Move judgement result revert logic to Playfield #22291

ekrctb · 2023-01-19T11:14:21Z

Previously, some judgement results were not reverted when the source DHO is not alive (e.g. frames skipped in editor).
Now, all results are reverted in the exact reverse order.
HitObjectContainer.RevertResult event is removed and Playfield.RevertResult event is changed to remove DrawableHitObject argument.

Previously, some judgement results were not reverted when the source DHO is not alive (e.g. frames skipped in editor). Now, all results are reverted in the exact reverse order.

peppy · 2023-01-23T05:03:50Z

I think it would be good to add a test here which fails on master, so we can better understand the specific fail case (and guard against it potentially regressing in the future).

osu.Game/Rulesets/UI/Playfield.cs

osu.Game/Rulesets/Judgements/JudgementResultEntry.cs

Calculating from TimeOffset is bad because it loses precision. The result time won't change anymore even If `HitObject.GetEndTime()` changes later.

osu.Game/Rulesets/Judgements/JudgementResult.cs

peppy · 2023-01-25T05:02:23Z

@smoogipoo interested in your high level thoughts on this change as well.

peppy · 2023-01-25T08:43:12Z

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

-        {
-            base.Update();
-
-            if (Result != null && Result.HasResult)


This is a huge win.

peppy · 2023-01-25T08:43:46Z

osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs

@@ -26,6 +27,8 @@ public class HitObjectLifetimeEntry : LifetimeEntry

        private readonly IBindable<double> startTimeBindable = new BindableDouble();

+        internal event Action? RevertResult;


This is a touch awkward in terms of event flow, but I can't immediately see a way out of it.

peppy

I think I am on board with this change.

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

smoogipoo · 2023-01-25T22:26:51Z

At a high level, this looks fine to me. The concerns above are quite valid though.

ekrctb · 2023-01-27T10:30:33Z

Separate question, should MaximumJudgementOffset be applied for the negative direction also? It makes more sense if this property is used for lifetime purpose #22292.
i.e. Clamp(TimeOffset, -MaximumJudgementOffset, MaximumJudgementOffset) instead of Min(TimeOffset, MaximumJudgementOffset)

It is not needed anymore as TimeAbsolute is stored raw.

bdach · 2023-01-28T09:57:53Z

Separate question, should MaximumJudgementOffset be applied for the negative direction also?

I had that thought too when reading the code and I'd probably say that it should, but refrained from pointing it out because I didn't want to scope creep this PR.

…ult-in-playfield

peppy · 2023-02-15T05:51:25Z

osu.Game/Rulesets/Judgements/JudgementResult.cs

+        /// <remarks>
+        /// This is used instead of <see cref="TimeAbsolute"/> to check whether this <see cref="JudgementResult"/> should be reverted.
+        /// </remarks>
+        internal double? RawTime { get; set; }


In terms of naming, my only alternative proposal here would be

RawTime -> Time
TimeAbsolute -> ClampedTime

Both have roughly the same number of usages, so I'm not sure about which should get a non-suffixed/prefixed name. But in general I dislike naming with words that have unclear meanings (like Raw or Base or Real).

ClampedTime is pretty ugly name for a public property to have. I'd argue the reverse and keep public TimeAbsolute but do internal UnclampedTime. Rationale is that the ugly name is contained to internal members I suppose?

It is not clear having clamped time in JugementResult should be the way forward (why do we have judgements with out-of-bound time offsets in the first place?) so let's not name it ClampedTime for now (go with RawTime and TimeAbsolute).

Sure, i'm okay with no change if that's deemed better. We can see how it plays out.

peppy

Apart from naming, I'm pretty happy with this direction now.

bdach · 2023-02-19T14:19:16Z

I can agree with what this pull is doing, I don't see much wrong in what it's doing, and it will also fix other issues in the process (like #17482). Doesn't make the change any less scary, mind, but yeah. Way I see it it'll all either go perfectly or we'll have to revert this in a week. But probably worth a go regardless?

ekrctb added 2 commits January 19, 2023 19:57

Move judgement result revert logic to Playfield

812a4b4

Previously, some judgement results were not reverted when the source DHO is not alive (e.g. frames skipped in editor). Now, all results are reverted in the exact reverse order.

Remove now-redundant code

32acaa4

ekrctb added area:results area:ruleset-api labels Jan 19, 2023

ekrctb requested a review from a team January 19, 2023 11:14

pull-request-size bot added the size/L label Jan 19, 2023

Add test for RevertResult

8405a3e

bdach reviewed Jan 23, 2023

View reviewed changes

osu.Game/Rulesets/UI/Playfield.cs Outdated Show resolved Hide resolved

osu.Game/Rulesets/UI/Playfield.cs Outdated Show resolved Hide resolved

osu.Game/Rulesets/Judgements/JudgementResultEntry.cs Outdated Show resolved Hide resolved

ekrctb added 4 commits January 24, 2023 14:15

Remove unused code

e66e43e

Fix OnRevertResult timing

cc87923

Store Result.TimeAbsolute separately from offset

efef97d

Calculating from TimeOffset is bad because it loses precision. The result time won't change anymore even If `HitObject.GetEndTime()` changes later.

Fix inspection issue

e1702a8

bdach reviewed Jan 24, 2023

View reviewed changes

osu.Game/Rulesets/Judgements/JudgementResult.cs Outdated Show resolved Hide resolved

peppy self-requested a review January 25, 2023 08:35

peppy added 2 commits January 25, 2023 17:35

Merge branch 'master' into revert-result-in-playfield

f0406c3

Update test to use newer assetion logic

e3a5c23

peppy reviewed Jan 25, 2023

View reviewed changes

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

{

base.Update();

if (Result != null && Result.HasResult)

Copy link

Member

peppy Jan 25, 2023

Choose a reason for hiding this comment

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

This is a huge win.

peppy reviewed Jan 25, 2023

View reviewed changes

peppy previously approved these changes Jan 25, 2023

View reviewed changes

peppy requested a review from smoogipoo January 25, 2023 08:44

bdach self-requested a review January 25, 2023 10:06

bdach reviewed Jan 25, 2023

View reviewed changes

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs Outdated Show resolved Hide resolved

Remove JudgementResultEntry

27578c4

It is not needed anymore as TimeAbsolute is stored raw.

ekrctb dismissed peppy’s stale review via 27578c4 January 27, 2023 10:35

ekrctb mentioned this pull request Jan 30, 2023

The combo adds +1 when rewinding to the beginning of the mania replay #21732

Open

ekrctb added 3 commits February 9, 2023 16:47

Merge branch 'maximum-judgement-offset-in-hit-object' into revert-res…

5f0636c

…ult-in-playfield

Store RawTime in JudgementResult

258de3b

Fix test setting invalid TimeOffset

5ddaf8e

peppy self-requested a review February 15, 2023 05:46

peppy reviewed Feb 15, 2023

View reviewed changes

Merge branch 'master' into revert-result-in-playfield

3ee0460

peppy approved these changes Feb 19, 2023

View reviewed changes

bdach self-requested a review February 19, 2023 13:02

bdach approved these changes Feb 19, 2023

View reviewed changes

bdach removed the request for review from smoogipoo February 19, 2023 15:32

Merge branch 'master' into revert-result-in-playfield

3183f5c

bdach enabled auto-merge February 19, 2023 15:37

bdach merged commit b5dda40 into ppy:master Feb 19, 2023

ekrctb deleted the revert-result-in-playfield branch February 20, 2023 09:04

peppy mentioned this pull request Feb 21, 2023

Increasing the number of slider repeats doesn't correctly extend the slider visuals in editor #22698

Closed

bdach mentioned this pull request Feb 21, 2023

Strict tracking mod sometimes allows to let go of the slider without penalty #17482

Closed

This was referenced Mar 31, 2023

Fix rewind visuals on Slides LumpBloom7/sentakki#450

Merged

Parent OnRevertResult is not invoked by nested HitObjects #23069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move judgement result revert logic to Playfield #22291

Move judgement result revert logic to Playfield #22291

ekrctb commented Jan 19, 2023 •

edited by bdach

Loading

peppy commented Jan 23, 2023

peppy commented Jan 25, 2023

peppy Jan 25, 2023

peppy Jan 25, 2023

peppy left a comment

smoogipoo commented Jan 25, 2023

ekrctb commented Jan 27, 2023 •

edited

Loading

bdach commented Jan 28, 2023 •

edited

Loading

peppy Feb 15, 2023

bdach Feb 16, 2023 •

edited

Loading

ekrctb Feb 19, 2023 •

edited

Loading

peppy Feb 19, 2023

peppy left a comment

bdach commented Feb 19, 2023

		@@ -26,6 +27,8 @@ public class HitObjectLifetimeEntry : LifetimeEntry

		private readonly IBindable<double> startTimeBindable = new BindableDouble();

		internal event Action? RevertResult;

Move judgement result revert logic to Playfield #22291

Move judgement result revert logic to Playfield #22291

Conversation

ekrctb commented Jan 19, 2023 • edited by bdach Loading

peppy commented Jan 23, 2023

peppy commented Jan 25, 2023

peppy Jan 25, 2023

Choose a reason for hiding this comment

peppy Jan 25, 2023

Choose a reason for hiding this comment

peppy left a comment

Choose a reason for hiding this comment

smoogipoo commented Jan 25, 2023

ekrctb commented Jan 27, 2023 • edited Loading

bdach commented Jan 28, 2023 • edited Loading

peppy Feb 15, 2023

Choose a reason for hiding this comment

bdach Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

ekrctb Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

peppy Feb 19, 2023

Choose a reason for hiding this comment

peppy left a comment

Choose a reason for hiding this comment

bdach commented Feb 19, 2023

ekrctb commented Jan 19, 2023 •

edited by bdach

Loading

ekrctb commented Jan 27, 2023 •

edited

Loading

bdach commented Jan 28, 2023 •

edited

Loading

bdach Feb 16, 2023 •

edited

Loading

ekrctb Feb 19, 2023 •

edited

Loading