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

Axilotl17 commented 1 year ago

Type

Game behaviour

Bug description

when you create a slider and add a backtick, when played the slider disappear after the backtick gets hit.

Screenshots or videos

https://user-images.githubusercontent.com/78441708/220034779-43f24f54-895a-422c-922d-617f06f9a18a.mp4

Version

2023.207.0 & latest dev build

Logs

runtime.log performance.log network.log input.log database.log legacy-ipc.log

Axilotl17 commented 1 year ago

question: Is it okay to report bugs while using the dev build, on the latest commits? i mostly use that for mapping anyways and it's just more convenient.

peppy commented 1 year ago

By backtick, do you mean repeats? If possible, please use standard terminology when describing issues.

peppy commented 1 year ago

So this absolutely does not happen on the last release. You must have been mistaken when stating that.

@ekrctb this looks to have regressed with https://github.com/ppy/osu/pull/22291, could you take a quick look?

ekrctb commented 1 year ago

This issue also occurs when any hit object (not just sliders, all rulesets) is moved in the timeline.

Hit animation uses HitStateUpdateTime and it is the same as Result.TimeAbsolute. Results are preserved even if the hit object is modified and the end time is changed. Previously, only Result.TimeOffset is stored, and absolute time is computed as end time + offset. Now, the result's absolute time is stored and therefore it will be stale when the hit object is modified.

Storing absolute time instead of time offset was a deliberate decision. Storing time offset has floating-point precision issues and also complicates the revert logic.

Currently, there is no way to hook for hit object end time changes. HitObject.StartTime is a Bindable, but the duration and the end time are not.
One must be careful to not write Result.RawTime in place to not break the revert logic, which relies on the invariant of monotone time.
Hypothetical expression NewResultTime = OldResultTime + (NewEndTime - OldEndTime) has the floating-point precision issue.

Not immediately sure how to solve this issue. Could revert the PR if this issue is important.

bdach commented 1 year ago

I'm weakly opposed to reverting the PR. Sure this breakage is not great but the pull fixed other issues, including spurious rewinds in actual gameplay due to floating point error (https://github.com/ppy/osu/issues/17482).

Can we not do something like hook into HitObjectUpdated and clear all results of objects that have been touched in the editor? This is maybe a bit nuclear, but it has the potential to fix a whole lot of other issues like this.

bdach commented 1 year ago

So something like the following is extremely crude, but it also kinda works, so maybe there's a good idea in there somewhere...?

diff --git a/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs b/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs
index 20ee409937..1cf63cbfae 100644
--- a/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs
+++ b/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs
@@ -70,6 +70,7 @@ private void regenerateAutoplay()
         {
             var autoplayMod = drawableRuleset.Mods.OfType<ModAutoplay>().Single();
             drawableRuleset.SetReplayScore(autoplayMod.CreateScoreFromReplayData(drawableRuleset.Beatmap, drawableRuleset.Mods));
+            drawableRuleset.Playfield.ResetAllResults();
         }

         private void addHitObject(HitObject hitObject)
diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
index 0c50f8341a..af273ca0ed 100644
--- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
+++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
@@ -668,6 +668,8 @@ protected void ApplyResult(Action<JudgementResult> application)
             OnNewResult?.Invoke(this, Result);
         }

+        internal void RefreshState() => updateState(Result.IsHit ? ArmedState.Hit : ArmedState.Miss, true);
+
         /// <summary>
         /// Processes this <see cref="DrawableHitObject"/>, checking if a scoring result has occurred.
         /// </summary>
diff --git a/osu.Game/Rulesets/UI/Playfield.cs b/osu.Game/Rulesets/UI/Playfield.cs
index b1c3b78e67..c0c1b1733c 100644
--- a/osu.Game/Rulesets/UI/Playfield.cs
+++ b/osu.Game/Rulesets/UI/Playfield.cs
@@ -478,6 +478,21 @@ private void revertResult(HitObjectLifetimeEntry entry)
             result.Reset();
         }

+        internal void ResetAllResults()
+        {
+            foreach (var entry in judgedEntries)
+            {
+                if (entry.Result != null)
+                    entry.Result.TimeOffset = 0;
+            }
+
+            foreach (var dho in HitObjectContainer.AliveObjects)
+            {
+                if (dho.Result.HasResult)
+                    dho.RefreshState();
+            }
+        }
+
         #region Editor logic

         /// <summary>

Thoughts? Too ugly to live, or possibly passable?

peppy commented 1 year ago

Feels quite ugly to me. Like we've done something wrong at all levels and this is a hack to work around that :/

Cootz commented 1 year ago

Similar behavior for spinner

https://github.com/ppy/osu/assets/50776304/1602def1-d3f6-4960-a717-65f2fced7a1f

ppy / osu