osu icon indicating copy to clipboard operation
osu copied to clipboard

Fix spinner not restoring HP after bonus cap

Open substanc3-dev opened this issue 1 year ago • 2 comments

Closes #26261.

Implemented as talked about on discord.

Before:

https://github.com/ppy/osu/assets/6898693/484eb86e-8289-42f4-84df-23b2f8269359

After:

https://github.com/ppy/osu/assets/6898693/d77b3c2b-54a3-4aac-8057-72997ce86d94

substanc3-dev avatar Jan 02 '24 17:01 substanc3-dev

I get that this implementation was hinted at on discord, but I have severe concerns over how it will play out. Have you checked what this does for score statistics? Are maps with spinners now going to have 1000 of these results? Does it not tank gameplay performance from the number of extra judgements etc.?

From the oversights I've pointed out in review I have concerns this was tested thoroughly enough.

peppy avatar Jan 03 '24 03:01 peppy

I get that this implementation was hinted at on discord, but I have severe concerns over how it will play out. From the oversights I've pointed out in review I have concerns this was tested thoroughly enough.

While I do believe I've tested this semi-reasonably well, and I think this approach is viable (even if only after adjustments), I understand this position. As I don't think this is critical to be addressed prior to pp release (as these changes only make the game easier, so old scores would still be valid), I'm marking this as draft, so it hopefully doesn't waste any more bandwidth.

Have you checked what this does for score statistics?

To be completely honest not sure what exactly you mean by this, the HealthBonus hit result is implemented similarly to IgnoreHit. I've checked that it doesn't show on the result screen, and I've also considered submission, where this would indeed add 2 fields (one in statistics, one in maximum_statistics) but I don't think this is particularly a concern? Similarly in replays it also just emits a count of hit results, which I also didn't find very concerning.

Are maps with spinners now going to have 1000 of these results?

I don't think the vast majority of maps have 2 minutes worth of spinners, however if this is a concern, the count of the health ticks (and their effect) could easily be adjusted (currently at about 8 ticks per second of spinner, providing the same amount of health as corresponding combo ticks).

Does it not tank gameplay performance from the number of extra judgements etc.?

I'm not sure how exactly to test this, however I've looked into the numbers and on the test map linked in the issue they are as follows: At OD0 the rate is about 4 regular ticks per 8 hp ticks At OD10 the rate is about 7 regular ticks per 8 hp ticks As the numbers seemed quite comparable, I felt that the values I chose above made sense in context, and shouldn't cause significant issues (can be adjusted further if that is a concern).

I've looked into the possibility of doing this without creating HitObjects for it, however I was unable to find a convenient way to get the DrainingHealthProcessor in DrawableSpinner, so it didn't seem like a viable direction.

substanc3-dev avatar Jan 03 '24 04:01 substanc3-dev