Skip to content
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

Don't discard [[nodiscard]] result of clamping npc attributes #70421

Merged

Conversation

akrieger
Copy link
Member

@akrieger akrieger commented Dec 24, 2023

Summary

Bugfixes "Properly clamp npc personality traits"

Purpose of change

NPC personality traits are supposed to stay within a set range, but there was a bug in the clamp logic and so they were not. This is how to get nuclear Ghandi.

Describe the solution

Store the result of std::clamp to the variable being clamped.

Describe alternatives you've considered

Testing

CI.

Additional context

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership [C++] Changes (can be) made in C++. Previously named `Code` labels Dec 24, 2023
@akrieger akrieger force-pushed the no_unchecked_manifestations_of_anger branch from ade84fd to b5b293d Compare December 24, 2023 19:59
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Dec 24, 2023
@akrieger
Copy link
Member Author

Comes from #70314.

@inogenous
Copy link
Contributor

Maybe we should also annotate cata_utility.h clamp as [[nodiscard]], or just one day deprecate it in favor of std::clamp.

/**
* Clamp first argument so that it is no lower than second and no higher than third.
* Does not check if min is lower than max.
*/
template<typename T>
constexpr T clamp( const T &val, const T &min, const T &max )
{
return std::max( min, std::min( max, val ) );
}

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 24, 2023
Copy link
Member

@RenechCDDA RenechCDDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's embarrassing. Yeah this is what we want.

@kevingranade kevingranade merged commit 4ee780d into CleverRaven:master Dec 25, 2023
23 of 28 checks passed
@RenechCDDA
Copy link
Member

or just one day deprecate it in favor of std::clamp.

As far as I can see, cata::clamp only exists because std::clamp was not available at the time. We didn't move to C++17 (which introduced std::clamp) until #64152 (this year). So yes this is just infrastructure work that should be done at some point.

@akrieger akrieger deleted the no_unchecked_manifestations_of_anger branch December 25, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants