Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix editor blueprints being selectable for too long when hit markers are enabled #24288

Merged
merged 6 commits into from
Jul 25, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Jul 19, 2023

Addresses #24163.

@bdach
Copy link
Collaborator

bdach commented Jul 19, 2023

This fixes clicks, but drag box still sees the slider:

2023-07-19.21-19-44.mp4

The obvious would be to

diff --git a/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs b/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs
index 56a6b18433..604ee2139f 100644
--- a/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs
+++ b/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs
@@ -470,7 +470,7 @@ protected virtual void UpdateSelectionFromDragBox()
                         break;
 
                     case SelectionState.NotSelected:
-                        if (blueprint.IsAlive && blueprint.IsPresent && quad.Contains(blueprint.ScreenSpaceSelectionPoint))
+                        if (blueprint.IsAlive && blueprint.IsPresent && blueprint.HandlePositionalInput && quad.Contains(blueprint.ScreenSpaceSelectionPoint))
                             blueprint.Select();
                         break;
                 }

but I'm not entirely sure how to feel about that myself, so I'm not pushing that to this branch without discussion.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

see above

@peppy
Copy link
Member Author

peppy commented Jul 21, 2023

Good catch.

I was going to suggest that maybe we could null out the SelectionQuad in this situation, but it looks like that is being used for other purposes (ie. maintaining the correct selection area rectangle).

I'm also quite hesitant to place the check where you have it, as while it makes sense, it's some real inter-dependency logic that will likely get overlooked in other usages...

@bdach
Copy link
Collaborator

bdach commented Jul 21, 2023

Maybe something like this, then?

diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/OsuSelectionBlueprint.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/OsuSelectionBlueprint.cs
index 178b809d8b..8e8972a665 100644
--- a/osu.Game.Rulesets.Osu/Edit/Blueprints/OsuSelectionBlueprint.cs
+++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/OsuSelectionBlueprint.cs
@@ -25,12 +25,9 @@ public abstract partial class OsuSelectionBlueprint<T> : HitObjectSelectionBluep
                                                  || (DrawableObject is not DrawableSpinner && ShowHitMarkers.Value && editorClock.CurrentTime >= Item.StartTime
                                                      && editorClock.CurrentTime - Item.GetEndTime() < HitCircleOverlapMarker.FADE_OUT_EXTENSION);
 
-        public override bool HandlePositionalInput =>
-            // Bypass fade out extension from hit markers for input handling purposes.
+        public override bool IsSelectable =>
+            // Bypass fade out extension from hit markers for selection purposes.
             // This is to match stable, where even when the afterimage hit markers are still visible, objects are not selectable.
-            //
-            // Note that we are intentionally overriding HandlePositionalInput here and not ReceivePositionalInputAt
-            // as individual blueprint implementations override that.
             base.ShouldBeAlive;
 
         protected OsuSelectionBlueprint(T hitObject)
diff --git a/osu.Game/Rulesets/Edit/SelectionBlueprint.cs b/osu.Game/Rulesets/Edit/SelectionBlueprint.cs
index 3c878ffd33..158c3c102c 100644
--- a/osu.Game/Rulesets/Edit/SelectionBlueprint.cs
+++ b/osu.Game/Rulesets/Edit/SelectionBlueprint.cs
@@ -32,7 +32,7 @@ public abstract partial class SelectionBlueprint<T> : CompositeDrawable, IStatef
         /// </summary>
         public event Action<SelectionBlueprint<T>> Deselected;
 
-        public override bool HandlePositionalInput => ShouldBeAlive;
+        public override bool HandlePositionalInput => IsSelectable;
         public override bool RemoveWhenNotAlive => false;
 
         protected SelectionBlueprint(T item)
@@ -125,6 +125,11 @@ protected virtual void OnSelected()
         /// </summary>
         public virtual MenuItem[] ContextMenuItems => Array.Empty<MenuItem>();
 
+        /// <summary>
+        /// Whether the <see cref="SelectionBlueprint{T}"/> can be currently selected via a click or a drag box.
+        /// </summary>
+        public virtual bool IsSelectable => ShouldBeAlive && IsPresent;
+
         /// <summary>
         /// The screen-space main point that causes this <see cref="HitObjectSelectionBlueprint"/> to be selected via a drag.
         /// </summary>
diff --git a/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs b/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs
index 56a6b18433..790a224e3f 100644
--- a/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs
+++ b/osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs
@@ -470,7 +470,7 @@ protected virtual void UpdateSelectionFromDragBox()
                         break;
 
                     case SelectionState.NotSelected:
-                        if (blueprint.IsAlive && blueprint.IsPresent && quad.Contains(blueprint.ScreenSpaceSelectionPoint))
+                        if (blueprint.IsSelectable && quad.Contains(blueprint.ScreenSpaceSelectionPoint))
                             blueprint.Select();
                         break;
                 }

I also briefly considered making SelectionBlueprint.ScreenSpaceSelectionPoint nullable, wherein null means "don't select", but it is not easy to do as other places (cough skin editor cough) have already clung onto that for implementing transformations, and I like the above a bit more anyways.

Buyer beware, generally untested aside from checking that it solves the problem with osu!.

@peppy
Copy link
Member Author

peppy commented Jul 25, 2023

Yep, that solution looks solid, have applied.

As it turns out, the tightening of hitcircle lifetimes in editor caused
the test to fail, since the objects were too far apart and at the
starting time of the test, the first object was fully faded out and as
such not alive, therefore leading cyclic selection to fail to select it.

To fix, bring all three objects closer together time-wise so that this
does not happen and the test can continue to exercise its original
behaviour.
@bdach bdach enabled auto-merge July 25, 2023 17:31
@bdach bdach merged commit b86defb into ppy:master Jul 25, 2023
8 of 10 checks passed
@peppy peppy deleted the fix-editor-blueprint-input-extension branch July 29, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants