Judgement container's existing judgement removal logic is incorrect with pooling

[bdach]

Originally spotted this in https://github.com/ppy/osu/pull/11970. I was going to fix it myself, but after writing a test case it turned out not to be as easy as I thought, and fixing it would actually mean a fair bit of reorganisation, so I figure it may be best to open an issue for now to discuss the way forward first.

Consider the following failing test scene:

// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using NUnit.Framework;
using osu.Framework.Graphics;
using osu.Framework.Testing;
using osu.Game.Beatmaps;
using osu.Game.Beatmaps.ControlPoints;
using osu.Game.Rulesets.Judgements;
using osu.Game.Rulesets.Objects.Drawables;
using osu.Game.Rulesets.Osu.Judgements;
using osu.Game.Rulesets.Osu.Objects;
using osu.Game.Rulesets.Osu.Objects.Drawables;
using osu.Game.Rulesets.Scoring;
using osu.Game.Rulesets.UI;

namespace osu.Game.Tests.Visual.Gameplay
{
    public class TestSceneJudgementContainer : OsuTestScene
    {
        private JudgementContainer<DrawableOsuJudgement> judgementContainer;

        [SetUpSteps]
        public void SetUp()
        {
            AddStep("create judgement container",
                () => Child = judgementContainer = new JudgementContainer<DrawableOsuJudgement>
                {
                    Anchor = Anchor.Centre,
                    Origin = Anchor.Centre
                });
        }

        [Test]
        public void TestJudgementFromDifferentHitObjectIsNotRemoved()
        {
            DrawableHitCircle drawableHitCircle = null;

            AddStep("create hit circle", () =>
            {
                drawableHitCircle = new DrawableHitCircle(createHitCircle());
                Add(drawableHitCircle);
            });

            AddStep("add two judgements for different objects", () =>
            {
                addDrawableJudgement(drawableHitCircle);
                drawableHitCircle.Apply(createHitCircle(), null);
                addDrawableJudgement(drawableHitCircle);
            });
            AddAssert("two judgements in container", () => judgementContainer.Count == 2);
        }

        private void addDrawableJudgement(DrawableHitObject drawableHitObject)
        {
            var judgement = new DrawableOsuJudgement();
            judgement.Apply(new JudgementResult(drawableHitObject.HitObject, new OsuJudgement())
            {
                Type = HitResult.Great,
                TimeOffset = Time.Current
            }, drawableHitObject);
            judgementContainer.Add(judgement);
        }

        private HitCircle createHitCircle()
        {
            var circle = new HitCircle();
            circle.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty());
            return circle;
        }
    }
}

The reason why it's failing is this check in JudgementContainer (note that JudgedObjects are DHOs in this context):

https://github.com/ppy/osu/blob/97879c3c98bcc19012c52e86287b2b8c8f0357ab/osu.Game/Rulesets/UI/JudgementContainer.cs#L17-L19

Before pooling, assuming that different DHOs map to different HOs was correct. Pooling however breaks that assumption, to the point that I'm no longer even sure whether it's correct to associate judgements with DHOs like that; you'd think that the fix is as simple as tacking .HitObject onto both sides of the equality, but that doesn't work, since that goes through the DHO itself, so the value of .HitObject changes immediately upon application of the new object.

For any fix to have a chance of working, I imagine that the judgements would need to hold a reference to the hitobject directly. Aside from possibly breaking existing API, the main roadblock here is osu! (the ruleset), which uses the DHO's colour for lighting logic.

A possible way forward to bypass that could be to transfer that colour in DrawableJudgement.Apply(), but from an API standpoint I find it risky to have the DHO passed in to Apply() at all, as ruleset creators could make the same mistake of storing that DHO reference in the judgement. Another possibility I can see is hard-coupling drawable judgements to DHOs, which ensures there's no possible option for this sort of mix-up. Them being decoupled (and also pooled separately) is what makes this possible in the first place.

It's a slight issue all things considered (possibly to the point of being unnoticeable with the naked eye, which is why I'm marking this as low priority), but it may indicate deeper issues in the structure of things.

[smoogipoo]

The important part to keep in mind is that comment - this existed only for rewinding.

I can think of several alternative ways to fix this off the top of my head:

1. Bind to Playfield.RevertResult and remove judgements.
2. Override RemoveWhenNotAlive => base.RemoveWhenNotAlive || Time.Current < LifetimeStart in DrawableJudgement (need to test when rewinding exactly to LifetimeStart).

But as you said, it should be tied to HOs and not DHOs.

[peppy]

Can this not have a visual impact? I guess the worst case is that the wrong judgements are removed during a rewind?

[bdach]

Yeah, that would be the worst case scenario. Not that I've actually noticed this ever happening, mind you.

[peppy]

Would probably consider this non-low-priority in that case. Possible even milestone since it involves actual gameplay.

[peppy]

Going to drop this in the next milestone for the time being. Can be removed if it's considered too high effort (although from what i understand it should be a non-involved change).