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.
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
Added difficulty-dependent "grace period" for crime and morale #1466
base: main
Are you sure you want to change the base?
Added difficulty-dependent "grace period" for crime and morale #1466
Changes from all commits
17403fa
ee10dba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Given the update to use
constants::ColonyShipOrbitTime
as the default value during loading of saved games, perhaps we should modify this to match. That way the default value on all code paths is the same.In a practical sense, the turn of landing will never exceed the colony ship orbit time, since if colonists aren't landed by then, they all die in a fiery crash, and it's game over.
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.
If you wanted to avoid the comment about potential negative values, maybe we could still partially use the idea from the last review, and have a local variable:
The checks would then be:
We wouldn't need to worry about negative values in that case. Additionally, assuming the default value for
mTurnNumberOfLanding
is updated toconstants::ColonyShipOrbitTime
we wouldn't need to worry about potential overflow either.As a small added bonus, it also means we avoid a double lookup of
gracePeriod
within the same method.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.
Was just thinking this suggestion could have gone further, and set a
bool
variable, rather than repeating a boolean expression in eachif
condition.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.
As per the comment on Discord, perhaps this should be moved to
MapViewState::onDeployColonistLander
.In that case, we know for sure there are going to be colonists landed at that point, so we don't need the population size check, which was really just a way of detecting when colonists have landed.
We would still need to retain the second check, since there can be multiple landers, and we probably want to activate on the first landing, and not reset the value upon a subsequent landing.
The second check should still work correctly with a default value for
mTurnNumberOfLanding
ofconstants::ColonyShipOrbitTime
.