ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.07k stars 2.23k forks source link

`IApplicableToDrawableHitObjects` is inconsistent between pooled and non-pooled hit objects #13301

Open ekrctb opened 3 years ago

ekrctb commented 3 years ago

The doc comment of the interface

https://github.com/ppy/osu/blob/8617aaa2a7a9eaf1438ad850b9bc7691b3d5c846/osu.Game/Rulesets/Mods/IApplicableToDrawableHitObject.cs#L14-L19

says it is only applied to top-level DHOs. However, when the hit object is pooled, the mod is applied to nested hit objects as well:

https://github.com/ppy/osu/blob/a31a6947bb4771472686cfd195d69747d1106c0d/osu.Game/Rulesets/UI/Playfield.cs#L355-L361

However, this behavior is already used. If I disable the mod application to nested DHOs, osu!standard hidden mod breaks (slider head remains visible). Also, the doc comment of IApplicableToDrawableHitObject mentions to use NestedHitObjects, but it cannot be used for pooled hit objects because the mod is applied before Apply, no nested hit objects are created for the DHO yet.

Proposed solution:

By the way, I think the interface should be defined as ApplyToDrawableHitObject(DrawableHitObject), as every mod is using a foreach for the IEnumerable<DrawableHitObject>.

peppy commented 3 years ago

At some point we did decide that it should be applied to nested hit objects as well, so the documentation is definitely out of date.

Can also agree with the change to the interface definition, as long as it works in all existing usages. May need to give consideration to the fact that other rulesets are likely implementing this interface, so the previous method will need to existing as an Obsoleted (and be called for both top-level and nested? for the time being).

ekrctb commented 3 years ago

If the mod registers ApplyCustomUpdateState (example: ModWithVisibilityAdjustment), it is propagated to nested DHOs.

It means, for nested pooled DHOs, ApplyCustomUpdateState will be registered multiple times. It has been unnoticed because transforms are idempotent.

By changing ApplyToDrawableHitObjects to be called for nested non-pooled hit objects as well, ApplyCustomUpdateState propagation can be removed, I think.