Feature: tiles/day velocity unit #8278

jostephd · 2020-07-21T22:04:53Z

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 · 2020-07-21T22:16:50Z

I like this idea.

jostephd · 2020-07-21T22:44:15Z

src/strings.cpp

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


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

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

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 · 2020-07-23T10:21:44Z

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

nielsmh · 2020-07-23T15:54:19Z

src/lang/english.txt

@@ -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 will make several strings much longer, potentially "breaking" the UI, maybe shorten it to dt/d?

nobody will understand this unit

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 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 · 2020-07-23T23:05:05Z

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 · 2020-07-24T16:15:18Z

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 · 2020-07-24T23:56:21Z

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.

Eddi-z · 2020-07-25T11:51:56Z

src/strings.cpp

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


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)

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;

OpenTTD/src/strings.cpp

Line 343 in a56bf35

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

ldpl · 2020-08-14T17:29:29Z

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

Eddi-z · 2020-08-14T23:36:02Z

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 · 2020-08-28T06:40:52Z

Anything else needed from me?

src/strings.cpp

LordAro · 2020-09-01T09:13:09Z

src/lang/english.txt

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


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

"Player's units"?

"In-game", maybe.

"Simulation"?

"Abstract"?

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

my choice would be "game units"

@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 · 2020-10-09T20:43:32Z

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

jostephd · 2020-10-10T21:56:57Z

@LordAro Rebased.

TrueBrain · 2020-12-14T23:17:42Z

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!

jostephd · 2020-12-16T20:22:27Z

@TrueBrain Thanks!

jostephd commented Jul 21, 2020

View reviewed changes

jostephd force-pushed the master branch from acfa78c to a8bdb6b Compare July 23, 2020 03:30

nielsmh reviewed Jul 23, 2020

View reviewed changes

jostephd force-pushed the master branch from a8bdb6b to 993ff57 Compare July 23, 2020 23:23

Eddi-z reviewed Jul 25, 2020

View reviewed changes

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

jostephd changed the title ~~Feature: decitiles/day velocity unit~~ Feature: tiles/day velocity unit Aug 28, 2020

LordAro reviewed Sep 1, 2020

View reviewed changes

jostephd force-pushed the master branch from 9f6ee34 to 91c9dc2 Compare October 10, 2020 21:56

TrueBrain added candidate: yes size: small labels Dec 14, 2020

TrueBrain force-pushed the master branch from 30dde00 to 8e286a7 Compare December 14, 2020 23:16

Feature: new velocity unit "tiles/day"

f6b8a82

TrueBrain force-pushed the master branch from 8e286a7 to f6b8a82 Compare December 14, 2020 23:21

TrueBrain approved these changes Dec 14, 2020

View reviewed changes

TrueBrain merged commit b1cf79d into OpenTTD:master Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: tiles/day velocity unit #8278

Feature: tiles/day velocity unit #8278

jostephd commented Jul 21, 2020 •

edited

Loading

nielsmh commented Jul 21, 2020

jostephd Jul 21, 2020 •

edited

Loading

jostephd Jul 23, 2020

jostephd Jul 23, 2020

Eddi-z commented Jul 23, 2020

nielsmh Jul 23, 2020

Eddi-z Jul 23, 2020

jostephd Jul 23, 2020

jostephd Aug 28, 2020

jostephd commented Jul 23, 2020 •

edited

Loading

Eddi-z commented Jul 24, 2020 •

edited

Loading

jostephd commented Jul 24, 2020

Eddi-z Jul 25, 2020

jostephd Jul 26, 2020

ldpl commented Aug 14, 2020

Eddi-z commented Aug 14, 2020

jostephd commented Aug 28, 2020

LordAro Sep 1, 2020

jostephd Sep 2, 2020

FLHerne Oct 7, 2020

jostephd Oct 8, 2020

jostephd Oct 9, 2020

andythenorth Oct 11, 2020

Eddi-z Oct 11, 2020

jostephd Oct 11, 2020

LordAro commented Oct 9, 2020

jostephd commented Oct 10, 2020

TrueBrain commented Dec 14, 2020

jostephd commented Dec 16, 2020

	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;

Feature: tiles/day velocity unit #8278

Feature: tiles/day velocity unit #8278

Conversation

jostephd commented Jul 21, 2020 • edited Loading

nielsmh commented Jul 21, 2020

jostephd Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eddi-z commented Jul 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jostephd commented Jul 23, 2020 • edited Loading

Eddi-z commented Jul 24, 2020 • edited Loading

jostephd commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldpl commented Aug 14, 2020

Eddi-z commented Aug 14, 2020

jostephd commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LordAro commented Oct 9, 2020

jostephd commented Oct 10, 2020

TrueBrain commented Dec 14, 2020

jostephd commented Dec 16, 2020

jostephd commented Jul 21, 2020 •

edited

Loading

jostephd Jul 21, 2020 •

edited

Loading

jostephd commented Jul 23, 2020 •

edited

Loading

Eddi-z commented Jul 24, 2020 •

edited

Loading