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

Open
wants to merge 4 commits into
base: master
from
Open

Feature: tiles/day velocity unit #8278

wants to merge 4 commits into from

Conversation

@jostephd
Copy link

@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 nielsmh commented Jul 21, 2020

I like this idea.

{ { 1, 0}, STR_UNITS_VELOCITY_IMPERIAL },
{ { 103, 6}, STR_UNITS_VELOCITY_METRIC },
{ { 1831, 12}, STR_UNITS_VELOCITY_SI },
{ {37900, 16}, STR_UNITS_VELOCITY_PLAYORIENTED },

This comment has been minimized.

@jostephd

jostephd Jul 21, 2020
Author

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.)

This comment has been minimized.

@jostephd

jostephd Jul 23, 2020
Author

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

This comment has been minimized.

@jostephd

jostephd Jul 23, 2020
Author

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

@jostephd jostephd force-pushed the jostephd:master branch from acfa78c to a8bdb6b Jul 23, 2020
@Eddi-z
Copy link
Contributor

@Eddi-z 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

This comment has been minimized.

@nielsmh

nielsmh Jul 23, 2020
Contributor

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

This comment has been minimized.

@Eddi-z

Eddi-z Jul 23, 2020
Contributor

nobody will understand this unit

This comment has been minimized.

@jostephd

jostephd Jul 23, 2020
Author

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.

This comment has been minimized.

@jostephd

jostephd Aug 28, 2020
Author

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
Author

@jostephd 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.

@jostephd jostephd force-pushed the jostephd:master branch from a8bdb6b to 993ff57 Jul 23, 2020
@Eddi-z
Copy link
Contributor

@Eddi-z 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
Author

@jostephd jostephd 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

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.

@@ -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 */};

This comment has been minimized.

@Eddi-z

Eddi-z Jul 25, 2020
Contributor

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)

This comment has been minimized.

@jostephd

jostephd Jul 26, 2020
Author

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 jostephd:master branch 2 times, most recently from 4f3e884 to 2f7d15a Jul 26, 2020
@ldpl
Copy link
Contributor

@ldpl 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 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
Author

@jostephd jostephd commented Aug 28, 2020

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

This comment has been minimized.

@LordAro

LordAro Sep 1, 2020
Member

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

This comment has been minimized.

@jostephd

jostephd Sep 2, 2020
Author

"Player's units"?

This comment has been minimized.

@FLHerne

FLHerne Oct 7, 2020

"In-game", maybe.

This comment has been minimized.

@jostephd

jostephd Oct 8, 2020
Author

"Simulation"?

This comment has been minimized.

@jostephd

jostephd Oct 9, 2020
Author

"Abstract"?

This comment has been minimized.

@andythenorth

andythenorth Oct 11, 2020
Contributor

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

This comment has been minimized.

@Eddi-z

Eddi-z Oct 11, 2020
Contributor

my choice would be "game units"

This comment has been minimized.

@jostephd

jostephd Oct 11, 2020
Author

@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 LordAro commented Oct 9, 2020

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

@jostephd jostephd force-pushed the jostephd:master branch from 9f6ee34 to 91c9dc2 Oct 10, 2020
@jostephd
Copy link
Author

@jostephd jostephd commented Oct 10, 2020

@LordAro Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.