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.

Sign up for GitHub

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

Jump to bottom

Only allow seek to next/previous object via keybinding if there is no selection #29860

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Only allow seek to next/previous object via keybinding if there is no selection #29860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1292a34
Add failing test coverage for object nudging
bdach Sep 13, 2024
2ccb4a4
Add test coverage for seeking between objects in editor
bdach Sep 13, 2024
7f71ef4
Only allow seek to next/previous object via keybinding if there is no…
bdach Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 47 additions & 1 deletion osu.Game.Tests/Visual/Editing/TestSceneComposerSelection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using osu.Framework.Graphics.Cursor;
using osu.Framework.Graphics.UserInterface;
using osu.Framework.Testing;
using osu.Framework.Utils;
using osu.Game.Beatmaps;
using osu.Game.Graphics.UserInterface;
using osu.Game.Rulesets;
Expand Down Expand Up @@ -78,7 +79,7 @@ public void TestSelectAndShowContextMenuOutsideBounds()
}

[Test]
public void TestNudgeSelection()
public void TestNudgeSelectionTime()
{
HitCircle[] addedObjects = null!;

Expand All @@ -99,6 +100,51 @@ public void TestNudgeSelection()
AddAssert("objects reverted to original position", () => addedObjects[0].StartTime == 100);
}

[Test]
public void TestNudgeSelectionPosition()
{
HitCircle addedObject = null!;

AddStep("add hitobjects", () => EditorBeatmap.AddRange(new[]
{
addedObject = new HitCircle { StartTime = 200, Position = new Vector2(100) },
}));

AddStep("select object", () => EditorBeatmap.SelectedHitObjects.Add(addedObject));

AddStep("nudge up", () =>
{
InputManager.PressKey(Key.ControlLeft);
InputManager.Key(Key.Up);
InputManager.ReleaseKey(Key.ControlLeft);
});
AddAssert("object position moved up", () => addedObject.Position.Y, () => Is.EqualTo(99).Within(Precision.FLOAT_EPSILON));

AddStep("nudge down", () =>
{
InputManager.PressKey(Key.ControlLeft);
InputManager.Key(Key.Down);
InputManager.ReleaseKey(Key.ControlLeft);
});
AddAssert("object position moved down", () => addedObject.Position.Y, () => Is.EqualTo(100).Within(Precision.FLOAT_EPSILON));

AddStep("nudge left", () =>
{
InputManager.PressKey(Key.ControlLeft);
InputManager.Key(Key.Left);
InputManager.ReleaseKey(Key.ControlLeft);
});
AddAssert("object position moved left", () => addedObject.Position.X, () => Is.EqualTo(99).Within(Precision.FLOAT_EPSILON));

AddStep("nudge right", () =>
{
InputManager.PressKey(Key.ControlLeft);
InputManager.Key(Key.Right);
InputManager.ReleaseKey(Key.ControlLeft);
});
AddAssert("object position moved right", () => addedObject.Position.X, () => Is.EqualTo(100).Within(Precision.FLOAT_EPSILON));
}

[Test]
public void TestRotateHotkeys()
{
Expand Down
39 changes: 37 additions & 2 deletions osu.Game.Tests/Visual/Editing/TestSceneEditorSeeking.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
// 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 System.Linq;
using NUnit.Framework;
using osu.Game.Beatmaps;
using osu.Game.Beatmaps.ControlPoints;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Osu;
using osu.Game.Rulesets.Osu.Objects;
using osuTK.Input;

namespace osu.Game.Tests.Visual.Editing
Expand Down Expand Up @@ -135,9 +137,42 @@ public void TestSeekBetweenControlPoints()
pressAndCheckTime(Key.Up, 0);
}

private void pressAndCheckTime(Key key, double expectedTime)
[Test]
public void TestSeekBetweenObjects()
{
AddStep("add objects", () =>
{
EditorBeatmap.Clear();
EditorBeatmap.AddRange(new[]
{
new HitCircle { StartTime = 1000, },
new HitCircle { StartTime = 2250, },
new HitCircle { StartTime = 3600, },
});
});
AddStep("seek to 0", () => EditorClock.Seek(0));

pressAndCheckTime(Key.Right, 1000, Key.ControlLeft);
pressAndCheckTime(Key.Right, 2250, Key.ControlLeft);
pressAndCheckTime(Key.Right, 3600, Key.ControlLeft);
pressAndCheckTime(Key.Right, 3600, Key.ControlLeft);
pressAndCheckTime(Key.Left, 2250, Key.ControlLeft);
pressAndCheckTime(Key.Left, 1000, Key.ControlLeft);
pressAndCheckTime(Key.Left, 1000, Key.ControlLeft);
}

private void pressAndCheckTime(Key key, double expectedTime, params Key[] modifiers)
{
AddStep($"press {key}", () => InputManager.Key(key));
AddStep($"press {key} with {(modifiers.Any() ? string.Join(',', modifiers) : "no modifiers")}", () =>
{
foreach (var modifier in modifiers)
InputManager.PressKey(modifier);

InputManager.Key(key);

foreach (var modifier in modifiers)
InputManager.ReleaseKey(modifier);
});
AddUntilStep($"time is {expectedTime}", () => EditorClock.CurrentTime, () => Is.EqualTo(expectedTime).Within(1));
}
}
Expand Down
6 changes: 6 additions & 0 deletions osu.Game/Screens/Edit/Editor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,10 +721,16 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
switch (e.Action)
{
case GlobalAction.EditorSeekToPreviousHitObject:
if (editorBeatmap.SelectedHitObjects.Any())
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no way to catch if any event has already been handled by a selection? It'd be nice if this could leverage the input handling system in some way to handle this in a general way instead of making all these a conditionals.

Also I think it should be able to seek while there is a selection if you have bound the seek key to some unused key, which in this implementation is not possible.

Copy link
Collaborator Author

@bdach bdach Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there no way to catch if any event has already been handled by a selection

No. Or at least no sane way.

Also I think it should be able to seek while there is a selection if you have bound the seek key to some unused key

I refuse to do such checks because it's weird and means that we have failed and made everything too customisable. Now things work or don't work depending on how users' bindings are set up. I am not willing to be on the hook for diagnosing weird failure modes like that.

Copy link
Contributor

@OliBomby OliBomby Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see how more customisability amounts to a 'fail'. More customisability is often better.
I guess most of your issue lies with the implementation of my suggestion as it would probably be a pain.

Current implementation is still weird because this context awareness is only on the hitobject seek while there is none for the other seek types. The only distinction between the seek types is that the hitobject seek is a formerly conflicting keybind.
To me a makes a lot more sense if all the seek keybinds adhere to the same rules:

  • No conflicted keybind -> works in all contexts
  • Conflicted keybind -> only works if there is no selection

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To me a makes a lot more sense if all the seek keybinds adhere to the same rules:

  • No conflicted keybind -> works in all contexts
  • Conflicted keybind -> only works if there is no selection

No I do not accept this premise. Doing this is a failure of design.

There are two options:

  • You make things customisable but also make sure to put enough guardrails to ensure the user can't shoot their foot off with the customisation options.
  • You don't make things customisable at all.

I'm not having this halfway "you make things customisable but by doing so you allow other features to silently change behaviour for no observable reason" you're proposing here. If you insist you can try implementing it yourself but I will not put my hand to it by either trying to implement this or approving it during review.


seekHitObject(-1);
return true;

case GlobalAction.EditorSeekToNextHitObject:
if (editorBeatmap.SelectedHitObjects.Any())
return false;

seekHitObject(1);
return true;

Expand Down
Loading