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

Apply negative stat modifiers in player::suffer #28240

Conversation

paroid01
Copy link
Contributor

Summary

SUMMARY: Bugfixes "Applies stat penalties from power overload and solar sensitivity"

Purpose of change

The player::suffer function applies stat modifiers for a few effects but does not update the current stat variables. This causes the modifiers to not be used by anything.

Fixes #18147 - Power overload str penalty not showing up

Describe the solution

Updates the current stat variables after applying all suffer effects.

Describe alternatives you've considered

Considered only updating the stat variables after applying any individual stat change, but that would be a lot of code duplication.

Additional context

This save includes a character with the Power Overload bionic and the solar sensitivity line of mutations. Loading the save and waiting one turn before and after this fix should clearly demonstrate the fix on the stat panel.

TestTown.zip

The player::suffer function applies stat modifiers for a few effects but
does not update the current stat variables. This causes the modifiers to
not be used by anything.

fixes CleverRaven#18147
@paroid01
Copy link
Contributor Author

I think this fixes the negative stat modifiers from addictions as well.

str_cur = get_str();
int_cur = get_int();
dex_cur = get_dex();
per_cur = get_per();
Copy link
Contributor

Choose a reason for hiding this comment

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

This "fix afterwards" approach doesn't seem quite right. Furthermore, this chunk was copied from player::process_artifact() verbatim.

I'd suggest updating stats inside corresponding bonus setters: set_str_bonus() updates str_cur, set_int_bonus() updates int_cur, and so on. This would be more robust, enable better encapsulation, and some code deduplication as a bonus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that that would be a much better way to do it. I was trying for the minimal code impact here that would fix the bug. I'll make another PR that has more code changes and the maintainers can choose which fix they'd prefer.

Actually the whole str_cur variable is the non-encaspulated version, Character::get_str() is the correct function to call. It just seems like the refactoring to move away from str_cur is incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#28282 I've solved the bug a different way, only one of these PRs needs to be applied.

@ifreund ifreund added [C++] Changes (can be) made in C++. Previously named `Code` Mechanics: Effects / Skills / Stats Effects / Skills / Stats <Bugfix> This is a fix for a bug (or closes open issue) labels Feb 19, 2019
@kevingranade
Copy link
Member

Obsoleted by #28282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Mechanics: Effects / Skills / Stats Effects / Skills / Stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Power overload str penalty not showing up.
5 participants