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 generic math-related changes #27965

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Apply generic math-related changes #27965

merged 5 commits into from
Apr 25, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Apr 22, 2024

To make game compile after ppy/osu-framework#6248.

  • Depends on framework package w/ the above

smoogipoo
smoogipoo previously approved these changes Apr 22, 2024
@peppy peppy self-requested a review April 23, 2024 14:04
peppy
peppy previously approved these changes Apr 23, 2024
@peppy peppy enabled auto-merge April 23, 2024 14:21
@bdach bdach disabled auto-merge April 23, 2024 16:05
@bdach
Copy link
Collaborator Author

bdach commented Apr 23, 2024

Well that's very red... I'll have to look into what's happened here.

@bdach
Copy link
Collaborator Author

bdach commented Apr 23, 2024

I think I've fixed the issues. They both stem from more stringent checking on the framework side.

cbbf2dd should be self-explanatory, mostly: the initial value DifficultyBindable.maxValue did not match the initial value of DifficultyBindable.CurrentNumber.MaxValue, and the method that updates the bounds of that thing updates them both at once, so when taiko sets MinValue = 0.25, it would attempt to also set MaxValue, to the desynced maxValue which should be 10 but was really 0 - and this would therefore explode due to attempting to set incorrect bounds of [0.25, 0].

787e60f is more complicated. In a few mania health processor tests the draining health processor would actually diverge to the point where drop would be basically double.Epsilon and HpMultiplierNormal would become infinite. This would backfire here:

return HpMultiplierNormal * increase;

If increase is 0, for e.g. mania hold note body, then the above expression becomes 0 * Infinity which is NaN. The previous implementation of clamping was a little dodgy in that it did a max(min(...)), which is fine for normal numbers, but will start doing random things with NaNs because they compare false to everything. The new generic math method refuses to play these games and instead returns NaN, which would then cause the health processor to also receive a NaN because NaNs are also viruses and infect everything, and thus everything catches on fire.

This is fixed by adding an escape hatch when a diverge is detected that just turns off hp drain and resets the multiplier to a neutral 1.

I think these are pretty okay but I am curious of your conclusions.

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@smoogipoo smoogipoo merged commit 52addc7 into ppy:master Apr 25, 2024
17 checks passed
@bdach bdach deleted the generic-math branch April 25, 2024 17:03
peppy added a commit that referenced this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants