Add new visuals and fixes to environmental tolerances #6323
Conversation
This commit is messy and probably doesn't build.
IMPORTANT: Auto-evo has no MP calculation for this, and many other systems are also probably buggy
And add tolerances to the explorer's csv export.
ender-titan1
changed the title
|
This PR is now feature complete and ready for testing and review. |
|
|
||
| /// <summary> | ||
| /// How wide a temperature range this species can stay in effectively. The range of temperatures is | ||
| /// <c>PreferredTemperature - TemperatureTolerance</c> to <c>PreferredTemperature + TemperatureTolerance</c> |
There was a problem hiding this comment.
Now it is only PreferredTemperature + TemperatureTolerance, right?
There was a problem hiding this comment.
Temperature has not been changed, it is still Preferred +/- Tolerance, this is indicated in the UI by the line connecting the range to the grabber being drawn from the center of the range as opposed to pressure where it is drawn from the minimum.
|
I have tested the functionality and did a code review, for me it looks fine :) |
edge-case with the way the pressure tolerance was saved.
hhyyrylainen
left a comment
hhyyrylainen
left a comment
There was a problem hiding this comment.
It took a while, but this should be a full review of all the code here. I didn't see any major architecture problems, but I did spot quite many potential code cleanup and code uniformity with other Thrive code changes that could be done. I didn't playtest yet, but I think the latest screenshot that was shown looked good of the result of this PR.
| public float OxygenResistance; | ||
|
|
||
| public EnvironmentalTolerances() | ||
| : this(0, 0, 0, 0, 0, 0) |
There was a problem hiding this comment.
Why are the default tolerances now basically unusable values?
There was a problem hiding this comment.
I might have missed something, but I don't think there was any point having default values here in the first place, as any time this constructor is used, it's followed by CopyFrom.
The only case where that doesn't happen is in the unit tests, but in my opinion it would make more sense to assign test values there then have a bunch of default values in the class that almost always get immediately overwritten.
There was a problem hiding this comment.
Well, even if it is not currently used, I think it's still a useful thing to have. I know this is kind of a struct, but in OOP one key concept is that the constructor makes classes valid to use, so it should ensure that all values have sensible data in them after construction. So even if not currently used, I think the default values were useful at least in illustrating what kind of data the class might hold for future readers of the code.
| /// Temperature (in C) that this species likes to be in | ||
| /// </summary> | ||
| public float PreferredTemperature = 15; | ||
| public float PreferredTemperature; |
There was a problem hiding this comment.
So why was this default value removed?
|
|
||
| // Show in red the conditions that are not matching to make them easier to notice | ||
| if (Math.Abs(patchTemperature - preferredTemperatureWithOrganelles) > | ||
| if (Math.Abs(patchTemperature - CurrentTolerances.PreferredTemperature) > |
There was a problem hiding this comment.
So now only conditions that are without modifiers bad are shown?
Wouldn't this make the GUI less helpful? Or does something compensate to show this? I haven't playtested yet so it might be the case that like the other modifier place turns red in this case and that helps?
There was a problem hiding this comment.
I think this is because PreferredTemperature isn't actually affected by any modifiers, just the temperature range is, so there is no point in calculating preferredTemperatureWithOrganelles.
There was a problem hiding this comment.
I'm going to ask that you make it so. We should account for future flexibility if some organelle, for example, in the future wants to reduce the temperature a cell can be in.
I just think that cutting out a part of the feature is going to make a potential future change so much harder, that we ought to do things fully now to support whatever tweaks someone doing balance changes might want to make.
|
We are currently in feature freeze until the next release. |
| #endif | ||
|
|
||
| var change = Math.Max(score.PerfectOxygenAdjustment, maxChange); | ||
| var change = Math.Max(score.PressureRangeSizeAdjustment, maxChange); |
There was a problem hiding this comment.
I'm preparing a separate PR to get this quite important auto-evo fix in, and I noticed that even this fix still seems to have a typo, because this is used to modify temperature but the variable here refers to pressure range.
Brief Description of What This PR Does
This PR reworks the environmental tolerances tab to be more player friendly and intuitive.
For this PR to be feature complete, the following tasks have to be finished:
Additionally, the following tasks have been completed in the process of developing this:
Related Issues
N/A
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.