Update Population format on Extinction Continue and Patch Details Panel Screens#6601
Update Population format on Extinction Continue and Patch Details Panel Screens#6601vmjauregui3 wants to merge 5 commits intoRevolutionary-Games:masterfrom
Conversation
|
Thank you for contributing to Thrive. Before your contribution can be accepted, you need to sign our CLA. Once your contribution has been accepted you can ask to be included in the credits (https://wiki.revolutionarygamesstudio.com/wiki/Team_Members) and you can apply to join the team and use this work as a sample: https://revolutionarygamesstudio.com/application/ |
|
The lead programmer for Thrive is currently on vacation until 2026-01-07. Until then other programmers will try to make pull request reviews, but please be patient if your PR is not getting reviewed. PRs may be merged after multiple programmers have approved the changes (especially making sure to ensure style guide conformance and gameplay testing are good). If there are no active experienced programmers who can perform merges, PRs may need to wait until the lead programmer is back to be merged. |
|
I would like to contribute if possible, so even if this commit is not proper, please inform me of the error and I will do my best to work on improving it. |
| continueText.ExtendedBbcode = Localization.Translate("CONTINUE_AS_SPECIES") | ||
| .FormatSafe(ShowContinueAs.FormattedNameBbCode, | ||
| StringUtils.ThreeDigitFormat(ShowContinueAs.Population)); | ||
| .FormatSafe(ShowContinueAs.FormattedNameBbCode, $"{ShowContinueAs.Population} {AmountSuffix}"); |
There was a problem hiding this comment.
You don't need to create an AmountSuffix field on the class, just use the constant value directly
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmmm interesting, tbh I have no idea what it's for, it seems there is no point in making it an exported value here. Maybe it's some left-over code? Git says our lead programmer wrote these lines, so we would need to wait until he's back from the break.
If you look in two other places where this value is used you can see that it is used directly, example:
predictionDetailsText.Append(new LocalizedString("ENERGY_SUMMARY_LINE", Round(energyResult.Value.TotalEnergyGathered), Round(energyResult.Value.IndividualCost), $"{energyResult.Value.UnadjustedPopulation} {Constants.MICROBE_POPULATION_SUFFIX}"));
I would propose to also do it like that
There was a problem hiding this comment.
The reason why the suffix is made configurable is that each species type will in the future need to have its own value, for example multicell and especially macroscopic species will not have trillions of members. So there should be enough configurability put in the code that deals with the suffises so that it isn't a major headache later to solve. Without reading my own older code, I bet that that was what I was doing but obviously as the different suffix constants aren't made yet, the design might not be fully "tested" so to speak.
The nonconfigurability un auto-evo is kind of fine for now as auto-evo is not implemented for any other species type than microbes. But yes there as well the suffix will need to be made configurable.
p.s. I'm still on break but I couldn't help seeing the notification with some reference being made to me so I ended up reading the notification and then writing this message.
There was a problem hiding this comment.
@hhyyrylainen I'm not sure when you'll be back, but I also just returned from a holiday break, so I wanted to ask a question about this. Right now, even in my example photos, there are >999T values still being represented with a T. I think when I was playing before, I saw numbers with the Q suffix at the end. If we're currently using the T for all microbes, are those values also using an older system, or are they using a newer suffix system than what I got approved here?
There was a problem hiding this comment.
StringUtils.FormatNumber is the right way to ask to format big numbers. I haven't yet looked at the code here as I've had over 100 notification emails to go through, but also you need to multiply the number by population *= Constants.MICROBE_POPULATION_MULTIPLIER; first if the species is a microbe.
|
Apparently, I forgot to indent the change that made a new line. I'll have to fix it when I get home. |
|
There is still an issue with the styling. You can use Btw in almost all cases our lead programmer is the one that merges pull requests, so you will need to wait patiently for him to come back from his break :D, he just started today so you got unlucky with the timing |
|
Code looks good now, I have tested it and it works as intended |
|
I feel like it would be really useful to make a generic "FormatSpeciesPopulationForDisplay" method (maybe even directly in the |
hhyyrylainen
left a comment
There was a problem hiding this comment.
I feel like these changes should take into account the fact that these also exist in the multicellular stage and can be used from there, so it's not fully correct to use a hardcoded assumption that any species is a microbe species.


Prefix
I wanted to start contributing to Thrive, so I looked at the application page https://www.revolutionarygamesstudio.com/application and was led to https://github.com/Revolutionary-Games/Thrive/issues. I haven't made a public push request before, so this is unlikely to be formatted correctly, and for that, I apologize.
I saw the issue "Update the population format on the extinction continue screen" https://github.com/Revolutionary-Games/Thrive/issues/6570 was marked as "easy" and "good first issue" so I took a crack at it; although, I'm unsure if it's really the "best practice" format that is used in other places in the code. I'd have to keep reading through to make sure it aligned with how you all do things.
Brief Description of What This PR Does
Updates the Extinction Continue Screen to use the "T" suffix

While I was looking for an example of how it was used in other places, I saw that the Patch Details Panel Screen also wasn't updated. I'm unsure if that was intentional, but I changed that too.
Updates the Patch Details Panel Screen to use the "T" suffix.

I based the changes off of how the populations were being displayed in src/microbe_stage/editor/SpeciesResultButton.cs
Let me know if there is a better way to do this and I can make the change.
Related Issues
Fixes #6570
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.