-
-
Notifications
You must be signed in to change notification settings - Fork 565
Convert Cases of Hardcoded Light Unit #6321
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
base: master
Are you sure you want to change the base?
Convert Cases of Hardcoded Light Unit #6321
Conversation
- handling todo for small value display
|
I removed the mention of closing the issue that this only partially addresses. |
|
|
||
| sunlightChart.TooltipYAxisFormat = percentageFormat + " lx"; | ||
| var sunlight = SimulationParameters.Instance.GetCompoundDefinition(Compound.Sunlight); | ||
| sunlightChart.TooltipYAxisFormat = percentageFormat + sunlight.Unit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now missing a space between the % sign and the unit, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good to clean up these explicit uses of "lx" but I think this PR needs a bit more further tweaking before merging.
|
|
||
| numberPart = Localization.Translate("VALUE_WITH_UNIT") | ||
| .FormatSafe(Math.Round(amount, decimals), compoundDefinition.Unit); | ||
| numberPart = unitFormat.FormatSafe(percentageFormat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also have the "showEvenSmallValues" support and following the configured decimals value. Because right now if both ShowUnit and UsePercentageDisplay are defined, that stops many other features of this class from working, which seems very surprising from an API point of view to me.
|
We are currently in feature freeze until the next release. |
Brief Description of What This PR Does
Convert all places of the hardcoded "lx" unit for light in the code to instead use a Unit set in a config file.
I also fixed a TODO in the code so the showEvenSmallValues setting works regardless of if a value has a unit or not. I don't think this actually impacts any compounds that currently have units since it's just temperature and now light, but it's there now for future use.
NOTE: This does not solve the full original problem of the tagged issue of unifying all compound unit logic. I didn't go through and attempt to do that full fix for a few reasons.
Related Issues
partially addresses #4895
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.