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

Allow Scout trait to work with other traits #42923

Merged
merged 1 commit into from
Aug 15, 2020

Conversation

wapcaplet
Copy link
Contributor

@wapcaplet wapcaplet commented Aug 15, 2020

Summary

SUMMARY: Bugfixes "Allow Scout trait to work with other traits"

Purpose of change

For reasons I do not understand, using "multiplicative" in the "overmap_sight" mutation value map caused the "Self-Aware" trait to nullify the effects of both the "Scout" and "Topographagnosia" traits.

Fix #42853

Describe the solution

Correct the "overmap_sight" mutation value map to be additive, rather than multiplicative (since it is). Include a regression test that failed before this change, and passes afterward.

Describe alternatives you've considered

Trying to understand why Self-Aware has anything whatsoever to do with this. I do not know, but there is now a test case to catch it if it happens again.

Testing

Added a regression test to tests/mutation_test.cpp which can be run with tests/cata_test [mutations][overmap]

Playtest with the "Scout" and "Self-Aware" traits together to be sure "Scout" still worked.

Additional context

But why?

Correct the "overmap_sight" mutation value map to be additive, rather
than multiplicative. Include a regression test that failed before this
change, and passes afterward.

Fix CleverRaven#42853

For reasons I do not understand, using "multiplicative" here caused the
"Self-Aware" trait to nullify the overmap sight adjustments from both
the "Scout" and "Topographagnosia" traits.
@mqrause
Copy link
Contributor

mqrause commented Aug 15, 2020

"overmap_sight" will be 0 for every mutation that does not have it set to anything else. It will always be set to 0 if you have any other mutation, because every mutation will have that member and it will be 0 in almost all cases. As far as I'm aware, this will be the case for all mutation multipliers that get initialized with 0.

So if you still want it to be multiplicative, the better option to solve this would be to initialize it with 1.

@wapcaplet
Copy link
Contributor Author

wapcaplet commented Aug 15, 2020

"overmap_sight" will be 0 for every mutation that does not have it set to anything else. It will always be set to 0 if you have any other mutation, because every mutation will have that member and it will be 0 in almost all cases. As far as I'm aware, this will be the case for all mutation multipliers that get initialized with 0.

@mqrause Aha, sure enough. In other words, the "Scout" trait basically only works if you have no other traits. That is totally busted. Looking through the revision history, I wonder how it ever worked. Good explanation though, thank you for clarifying!

So if you still want it to be multiplicative, the better option to solve this would be to initialize it with 1.

The values that currently exist for Scout (5) and Topographagnosia (-10) are clearly intended to be additive, and their respective values are added to the accumulated sight range in the overmap_sight_range function, so I don't see how it could work any other way.

Edit: I suspect (but have not confirmed) this has been broken since the overmap sight modifier was JSONized in #29560 last year.

@wapcaplet wapcaplet changed the title Allow Scout trait to work with Self-Aware trait Allow Scout trait to work with other traits Aug 15, 2020
@ZhilkinSerg ZhilkinSerg added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies labels Aug 15, 2020
@ZhilkinSerg ZhilkinSerg merged commit baa4216 into CleverRaven:master Aug 15, 2020
@mqrause
Copy link
Contributor

mqrause commented Aug 15, 2020

The values that currently exist for Scout (5) and Topographagnosia (-10) are clearly intended to be additive, and their respective values are added to the accumulated sight range in the overmap_sight_range function, so I don't see how it could work any other way.

Yeah, sorry, seems I didn't really read your PR. Had a few beers and it was late at night. Introducing more mutations with values like that would make overmap sight explode if it were multiplicative. Currently, with just one possible trait with overmap sight, it technically wouldn't matter if it's additive or multiplicative, except for the initialization value having to be 0 or 1. (Or making it an optional value and ignoring it if it's not set.)

Edit: I suspect (but have not confirmed) this has been broken since the overmap sight modifier was JSONized in #29560 last year.

Looks like that broke it. It might be worth it to check for more broken multipliers.

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` Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scout trait not working properly
3 participants