Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: tiles/day velocity unit #8278

Merged
merged 1 commit into from Dec 14, 2020
Merged

Feature: tiles/day velocity unit #8278

merged 1 commit into from Dec 14, 2020

Conversation

jostephd
Copy link
Contributor

@jostephd jostephd commented Jul 21, 2020

Add a tiles/day velocity unit.

Why tiles/day? Because it's not intuitive how to correlate the existing units (km/h, m/s, mph) to distances on the game map. For example, if the coal mine is 100 tiles away from the power plant, produces 50 wagonfuls of coal a month, and trains move at 20 km/h, how many trains should I build? I have no idea. Change the question so the train speed is given in tiles/day and it becomes child's play to answer.

Why decitiles? The UI displays speeds as integers, and I think showing a whole number of tiles/day is too coarse. I didn't see an easy way to make the UI display speeds as fractions with one decimal place, so I compromised on showing tenths of a tile per day. Edit: Changed from decitiles/day to tiles/day, thanks @Eddi-z.

I used a version of this patch under 1.10.1 for a while.

@nielsmh
Copy link
Contributor

nielsmh commented Jul 21, 2020

I like this idea.

src/strings.cpp Outdated
{ { 1, 0}, STR_UNITS_VELOCITY_IMPERIAL },
{ { 103, 6}, STR_UNITS_VELOCITY_METRIC },
{ { 1831, 12}, STR_UNITS_VELOCITY_SI },
{ {37900, 16}, STR_UNITS_VELOCITY_PLAYORIENTED },
Copy link
Contributor Author

@jostephd jostephd Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose the magic numbers here based on the tile length information on the wiki: A tile is, for vehicle speed purposes 664.(216) km-ish, 668 km or 415 miles long.

A decitile a day = 66.8 km / 24 hours = (1/0.57820743) mph, equal to 37900 / (1<<16) to within 0.02%.

I confirmed this by testing. (Start a new game, build a straight and level track section of known length, and see how many in-game days a train going max speed needs to pass that section.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the constant to 37893 as that's even more accurate. (0.57820743 * 65536 == 37893.40213248)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the constant again to 37888 after doing the math again with the 664.(216) km-ish value rather than the 668 km value.

@Eddi-z
Copy link
Contributor

Eddi-z commented Jul 23, 2020

there's a fractional tile display in the depot view for train length, maybe you can reuse that

@@ -194,6 +194,7 @@ STR_COLOUR_DEFAULT :Default
STR_UNITS_VELOCITY_IMPERIAL :{COMMA}{NBSP}mph
STR_UNITS_VELOCITY_METRIC :{COMMA}{NBSP}km/h
STR_UNITS_VELOCITY_SI :{COMMA}{NBSP}m/s
STR_UNITS_VELOCITY_PLAYORIENTED :{COMMA}{NBSP}decitiles/day
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make several strings much longer, potentially "breaking" the UI, maybe shorten it to dt/d?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nobody will understand this unit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't think it's a big problem if the string "decitiles/day" itself gets truncated; only if the length of that string causes stuff following the "100 mph" string to be truncated.

We might get away with "dt/d" (or "deci-tpd" or "tiles/10d" or whatever) if we also put it in a parenthetical in the drop-down menu in the settings dialog where people choose this unit in the first place, something like Play-oriented (tiles per 10 days: dt/d). But these wouldn't be nearly as self-explanatory as "decitiles/day" is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also somewhat mitigated by the change from "decitiles/day" to "tiles/day", although that's admittedly still longer than "mph" and "km/h".

@jostephd
Copy link
Contributor Author

jostephd commented Jul 23, 2020

there's a fractional tile display in the depot view for train length, maybe you can reuse that

The problem isn't formatting a double into a string with given precision. The problem is that UnitConversion::ToDisplay returns an integer. To display a fraction, that function and all of its callers would have to be changed. I don't have the time to make this kind of change, sorry.

@Eddi-z
Copy link
Contributor

Eddi-z commented Jul 24, 2020

no, the value doesn't need to be a double, you treat the integer as a fixed point decimal, the game provides {DECIMAL} instead of {COMMA} for that

see

OpenTTD/src/depot_gui.cpp

Lines 329 to 331 in 663c301

SetDParam(0, CeilDiv(u->gcache.cached_total_length * 10, TILE_SIZE));
SetDParam(1, 1);
DrawString(rtl ? left + WD_FRAMERECT_LEFT : right - this->count_width, rtl ? left + this->count_width : right - WD_FRAMERECT_RIGHT, y + (this->resize.step_height - FONT_HEIGHT_SMALL) / 2, STR_TINY_BLACK_DECIMAL, TC_FROMSTRING, SA_RIGHT); // Draw the counter
for the mentioned example of depot gui

@jostephd
Copy link
Contributor Author

no, the value doesn't need to be a double, you treat the integer as a fixed point decimal, the game provides {DECIMAL} instead of {COMMA} for that

Ah, thanks. Pushed that. That changes "144 decitiles/day" to "14.4 tiles/day" when that unit is selected, and doesn't change the UI when mph, km/h, or m/s is selected. However, I'm not sure why there is no change for those three units; I expected there to be. 🤔

Thanks also for the pointer to the depot GUI code.

src/strings.cpp Outdated
@@ -1228,7 +1229,7 @@ static char *FormatString(char *buff, const char *str_arg, StringParameters *arg

case SCC_VELOCITY: { // {VELOCITY}
assert(_settings_game.locale.units_velocity < lengthof(_units_velocity));
int64 args_array[] = {ConvertKmhishSpeedToDisplaySpeed(args->GetInt64(SCC_VELOCITY))};
int64 args_array[] = {ConvertKmhishSpeedToDisplaySpeed(args->GetInt64(SCC_VELOCITY)), 1 /* decimal places */};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this appears to be working, it is not technically correct, in the sense that for the old units you now have this extra string parameter, which will get ignored in this case, but might cause problems in the future.

You should better take care of this now, instead of hoping it won't fall apart.

Options could be:

  • branching the code so you have 1 or 2 parameters, depending on the unit used
  • converting the other units from {COMMA} to {DECIMAL} as well, with second parameter of 0 (no idea how that gets displayed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should better take care of this now, instead of hoping it won't fall apart.

To be clear, I wasn't "hoping it won't fall apart". I knew that commit was incomplete, but I didn't see how it was incomplete, so I pushed it and indicated which part of it I suspected to be incorrect. I never expected the PR to be merged in that state.

branching the code so you have 1 or 2 parameters, depending on the unit used

I took a shot at this, using another StringParameters constructor so I can pass the array length explicitly. Seems good enough for now, and can be upgraded to std::vector in the future if needed.

converting the other units from {COMMA} to {DECIMAL} as well, with second parameter of 0 (no idea how that gets displayed)

{DECIMAL} with a second parameter of 0 is synonymous with {COMMA}, if I'm reading the code right:

OpenTTD/src/strings.cpp

Lines 1045 to 1053 in a56bf35

case SCC_COMMA: // {COMMA}
buff = FormatCommaNumber(buff, args->GetInt64(SCC_COMMA), last);
break;
case SCC_DECIMAL: {// {DECIMAL}
int64 number = args->GetInt64(SCC_DECIMAL);
int digits = args->GetInt32(SCC_DECIMAL);
buff = FormatCommaNumber(buff, number, last, digits);
break;

static char *FormatCommaNumber(char *buff, int64 number, const char *last, int fractional_digits = 0)

Let me know if you prefer this approach. (How are translations handled, though? Should I change {COMMA} to {DECIMAL} in all translations, or leave that to translators?)

@jostephd jostephd force-pushed the master branch 2 times, most recently from 4f3e884 to 2f7d15a Compare July 26, 2020 00:34
@ldpl
Copy link
Contributor

ldpl commented Aug 14, 2020

I just realized a huge problem with this idea - tile/d is different for traveling straight and diagonally...

@Eddi-z
Copy link
Contributor

Eddi-z commented Aug 14, 2020

I just realized a huge problem with this idea - tile/d is different for traveling straight and diagonally...

Honestly, i wouldn't worry about that in this context. if at all, we should be working on fixing this difference, but that would be out of scope here.

@jostephd jostephd changed the title Feature: decitiles/day velocity unit Feature: tiles/day velocity unit Aug 28, 2020
@jostephd
Copy link
Contributor Author

Anything else needed from me?

src/strings.cpp Outdated Show resolved Hide resolved
src/strings.cpp Outdated Show resolved Hide resolved
@@ -194,6 +194,7 @@ STR_COLOUR_DEFAULT :Default
STR_UNITS_VELOCITY_IMPERIAL :{COMMA}{NBSP}mph
STR_UNITS_VELOCITY_METRIC :{COMMA}{NBSP}km/h
STR_UNITS_VELOCITY_SI :{COMMA}{NBSP}m/s
STR_UNITS_VELOCITY_PLAYORIENTED :{DECIMAL}{NBSP}tiles/day
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of "play oriented" as a string, but I can't think of any good alternatives. "Friendly", perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Player's units"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"In-game", maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Simulation"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Abstract"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Tiles per day"? Name the thing for the thing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my choice would be "game units"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andythenorth That would be inconsistent with the other strings in the same drop-down (e.g., "Imperial" rather than "miles per hour").

I've gone ahead and made the change to "Game units".

@LordAro
Copy link
Member

LordAro commented Oct 9, 2020

CI changed recently - you'll need to revise to pull in the changes

@jostephd
Copy link
Contributor Author

@LordAro Rebased.

@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Dec 14, 2020
@TrueBrain
Copy link
Member

Honestly, I did not expect to like this, but it is pretty useful :D I might use it as my new default :D Thank you so much for this PR, and sorry it took so long.

I just rebased your code, and made some minor fixes (a variable called places is not the best name, so I changed it in decimal_places). If it passes CI, we will approve and merge :) No action needed from your side!

@TrueBrain TrueBrain merged commit b1cf79d into OpenTTD:master Dec 14, 2020
@jostephd
Copy link
Contributor Author

@TrueBrain Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants