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

Latency message after streaming now reports with decimal point #1314

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kentyman23
Copy link

@kentyman23 kentyman23 commented Jan 14, 2024

The Show latency message after streaming toast previously reported truncated integers because integer division was used to calculate them (5.99 would be reported as 5). This led to inaccuracies reported here: https://docs.google.com/spreadsheets/d/1WSyOIq9Mn7uTd94PC_LXcFlUi9ceZHhRgk-Yld9rLKc/

Since we're using floating-point arithmetic, we no longer have to avoid divide-by-zero errors. Corner cases one might see are "∞ ms" (positive time divided by 0 frames) or "NaN ms" (0 time divided by 0 frames). I felt these were perfectly fine to output in these theoretical, degenerate cases. Because of this, I was able to simplify the logic in Game.onStop to no longer check for 0s. This simpler logic means we don't need both conn_client_latency_hw and conn_hardware_latency localized strings; I opted to remove the former and keep the latter and put a newline between them, as it was hard to quickly read both stats when the visual location of the values depended upon how the wrapping worked out. This is how it looks:

image

(Note that PR #1313 was abandoned after I realized I pushed to my fork's master instead of a branch.)

message = getResources().getString(R.string.conn_hardware_latency)+" "+averageDecoderLat+" ms";
}
// Will format to match `\d+\.\d` (formats 1f as "1.0", 0.12f as "0.1", and 12.34f as "12.3")
DecimalFormat decimalFormat = new DecimalFormat("0.0");
Copy link
Member

Choose a reason for hiding this comment

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

Will this do the right thing for locales that use other separators (such as ,) for decimal values?

Copy link
Author

Choose a reason for hiding this comment

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

Great catch. I had assumed it would inherit the same local as getResources().getString(), but that may be wrong according to the docs:

To obtain a NumberFormat for a specific locale, including the default locale, call one of NumberFormat's factory methods, such as getInstance(). In general, do not call the DecimalFormat constructors directly, since the NumberFormat factory methods may return subclasses other than DecimalFormat. If you need to customize the format object, do something like this:

NumberFormat f = NumberFormat.getInstance(loc);
 if (f instanceof DecimalFormat) {
     ((DecimalFormat) f).setDecimalSeparatorAlwaysShown(true);
 }

Stay tuned for a fix...

}
// Will format to match `\d+\.\d` (formats 1f as "1.0", 0.12f as "0.1", and 12.34f as "12.3")
DecimalFormat decimalFormat = new DecimalFormat("0.0");
final String millisecondSuffix = " ms";
Copy link
Author

Choose a reason for hiding this comment

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

Speaking of localization, I noticed in someone's Discord video that there are localized versions of the millisecond suffix that we sometimes use and sometimes don't:

image

I'll see if there is an existing string I can leverage instead of hardcoding it as we have been in this method.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, it appears I'd have to make a new localized string or edit the existing ones to be format strings. Am I allowed to change or add new strings, or is that discouraged because it breaks existing localization? Would it be better for me to surgically steal the localized string for the suffix (like that мс in the Russian(?) string above) so that we don't have to get the localizers to do another pass?

Copy link
Author

Choose a reason for hiding this comment

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

@cgutman, please let me know which way you'd prefer. The former seems more "correct", but the latter seems easier and probably safe.

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

Successfully merging this pull request may close these issues.

None yet

2 participants