-
-
Notifications
You must be signed in to change notification settings - Fork 588
Update Population format on Extinction Continue and Patch Details Panel Screens #6601
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
Open
vmjauregui3
wants to merge
5
commits into
Revolutionary-Games:master
Choose a base branch
from
vmjauregui3:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+3
−2
Open
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8b7cdb0
Updated the population format on the extinction continue screen to us…
eb472c1
Updated the population format on the Patch Details Panel screen to us…
be223ee
Updating my commit so that no C# exceeds the maximum column width of …
bfd478c
Updated the changes to use the constant value directly
366faa1
Updated the indentation of my last edit to ExtinctionBox.cs
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I thought, but it's how it was done in already existing code in this project.


It is exported here though. Is that why it's supposed to be done in this case or should it be changed here as well?
Regardless, I have made the changes and am adding those to this pull request.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringUtils.FormatNumberis 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 bypopulation *= Constants.MICROBE_POPULATION_MULTIPLIER;first if the species is a microbe.