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

Fix draining processor failing gameplay on bonus misses, tiny droplet misses, and ignore hits #27154

Merged
merged 9 commits into from Mar 4, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Feb 13, 2024

This closes:

The "ignore hit" portion of this is a small carve-out for another weird detail I found along the way wherein IgnoreHits would cause the health processor to invoke failure even though they're successful hits, and this is because they do not give any health gain. You could ask why that's not a MaxHealthIncrease > 0 check instead - it's not because it breaks the other added test cases, in particular things like a ComboBreak which has an IgnoreHit max judgement but also should cause failure at zero HP.

I'll readily admit that I didn't empirically test this in catch because I'm not good enough at the mode / couldn't find a good beatmap to test on. But I did test the osu! case manually.

For reference: stable code for osu! and for catch.

Please read the test cases for the desired behaviour here.

@bdach
Copy link
Collaborator Author

bdach commented Feb 13, 2024

blocking because apparently #27159 is a thing too and i don't think this pull fixes this (but it should)

@bdach bdach added the blocked Don't merge this. label Feb 13, 2024
@bdach bdach removed the blocked Don't merge this. label Feb 14, 2024
@bdach
Copy link
Collaborator Author

bdach commented Feb 14, 2024

This fixes #27159 now too. See inline comment for explanation.

Can be manually tested using test.zip (change extension to .osz to import). To test, miss the first fruit and two subsequent large droplets, then hit every large droplet / fruit until the end of the map but move out of the way inbetween so that small droplets get missed. You should be able to hit zero hp pretty easily.

@bdach
Copy link
Collaborator Author

bdach commented Feb 14, 2024

Of course perfect mod breaks this. Missing a tiny droplet with nomod can't fail on stable, but with perfect on it can.

I don't even know at this point.

Stable does this:

    https://github.com/peppy/osu-stable-reference/blob/46cd3a10af7cc6cc96f4eba92ef1812dc8c3a27e/osu!/GameplayElements/HitObjectManagerFruits.cs#L98-L102

I'd rather not say what I think about it doing that, since it's likely
to be unpublishable, but to approximate that, just make it so that
only the "default fail condition" is beholden to the weird ebbs
and flows of what the ruleset wants. This appears to fix the problem
case and I'm hoping it doesn't break something else but I'm like 50/50
on it happening anyway at this point. Just gotta add tests add nauseam.
@bdach
Copy link
Collaborator Author

bdach commented Feb 14, 2024

Maybe fixed. Rationale in last commit message. Now this has infinity percent more breaking ruleset API changes but I'm not sure how to fix otherwise.

@bdach bdach changed the title Fix draining processor failing gameplay on bonus misses and ignore hits Fix draining processor failing gameplay on bonus misses, tiny droplet misses, and ignore hits Feb 14, 2024
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.

lgtm

@smoogipoo smoogipoo merged commit e935c49 into ppy:master Mar 4, 2024
15 of 17 checks passed
@bdach bdach deleted the catch-fail-on-banana branch March 4, 2024 06:48
@smoogipoo
Copy link
Contributor

smoogipoo commented Mar 20, 2024

Do you remember if there any particular cases (aside from test failures) that informed your decision to not include IgnoreMiss here? Looking at #27121 (comment)

It sounds like there's two solutions:

  1. Make DrawableBananaShower indicate IgnoreHit at all times.
  2. Change the base condition to also include IgnoreMiss. At face value this doesn't sound too extreme to me. The alternative is ... || Type == IgnoreHit || (Judgement.Max == IgnoreHit && Type == IgnoreMiss) i.e. the special IgnoreJudgement.

@bdach
Copy link
Collaborator Author

bdach commented Mar 20, 2024

if there any particular cases (aside from test failures) that informed your decision to not include IgnoreMiss here?

Probably the biggest one is osu! slider tail circle which uses (SliderTailHit, IgnoreMiss) when classic mod is off. That should probably allow failure still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants