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

Add grid placement tool #26313

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Add grid placement tool #26313

wants to merge 48 commits into from

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Jan 1, 2024

Part of #26303
closes #30118

Requires

Adds the 'Change grid' placement tool to the left tool box.

Mappers might want to align their grid with some hit objects they have already placed, so I also added a 'Grid from points' feature which allows the mapper to select two points on the playfield and then the grid aligns itself with those points. The origin is centered on the first point and then the spacing and rotation is determined by the difference between the two points. The two points will be on two neighbouring grid lines in the resulting grid.
This point selection has snapping to nearby objects so its easy to align it exactly with hit objects.
Also clicking with right click will end the point selection immediately so you can select just one point allowing you to change just the grid origin.

I repurposed the initial PR #26303 to add the remaining keybinds for cycling grid type.

osu._k6UmTIJN2n.mp4

This caused issues in rendering the outline of the grid because the outline was getting masked at some resolutions.
@bdach
Copy link
Collaborator

bdach commented Sep 20, 2024

Feature seems useful but as usual I have UX concerns.

2024-09-20.10-29-06.mp4

There is no guidance on how to use this feature. There is a button with no tooltip. You click on it and nothing visually happens. You click on it five times and maybe move it over the playfield and notice the grid starts moving. Only at that point you can probably figure out that you need to click the playfield twice to have it work. I'd still say both points should be visually indicated somehow rather than just relying on the grid but maybe that's optional.

The main problem is no indication on what you're supposed to do with the feature at all.

Also there is no way to cancel a grid change if you decide you don't want to change the grid anymore. Esc doesn't work.

Also I don't get what the criteria for the grid spacing subdividing is. It appears that it will decrease when the cursor moved too much between clicks 1 & 2, but I have no idea why it's doing what it's doing in particular.

There's also whatever this is:

2024-09-20.10-34-27.mp4

You can be in the "change grid" mode and still have things like object placement appear to work but they actually will not work and instead the grid change thing will take priority. At that point I'd start thinking if this shouldn't be a tool in the toolbox on the left.

@OliBomby OliBomby changed the title Add 'Grid from points' button Add grid changing placement tool Sep 23, 2024
@OliBomby
Copy link
Contributor Author

OliBomby commented Sep 23, 2024

I changed the 'grid from points' button into a placement tool like @bdach suggested. I think the UX is really nice now.

The diffstat became pretty big but most of it is from 1a81e12 which is mostly just refactoring name change. It should be pretty easy to review as that commit should not change any behaviours.

I'll need an icon for the new placement tool so that's still TODO

@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 1, 2024

I added a tooltip so it tells you what to do while you are hovering the button wondering what to do. From then on it should be pretty obvious how it works.

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.

I guess UX wise I'd be fine with this in its current state, but I'd want @peppy's reading of it too to proceed with merging this in.

@@ -107,5 +108,7 @@ public bool MatchingFilter
}

public bool FilteringActive { get; set; }

public virtual LocalisableString TooltipText { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This entire change with making TooltipText virtual is quite dodgy because some derived classes override the getter but leave the setter intact. Previously the intent was that the tooltip in those classes was derived from internal data and as such is immutable outside the class. But now with this you can freely overwrite the TooltipText of a ReadyButton or FavouriteButton or whatever else which is improper design.

The easiest way out of this would probably be to drop the virtual, and update the places that override the getter so that instead of it they use the setter (which will require a few bindable subscriptions or something to provide the same effect).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a good place to use the new keyword. By declaring a public new LocalisableString TooltipText in ReadyButton and FavouriteButton we can remove the setter entirely which seems more appropriate for these classes which want to manage their tooltip text internally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eh. I don't like shadowing because it makes things tricky anyway. Simple non-virtual non-shadowed public get; set; is best.

Copy link
Contributor Author

@OliBomby OliBomby Oct 3, 2024

Choose a reason for hiding this comment

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

I would agree with you that shadowing makes things tricky in most cases, but in this case using shadowing seems like the simplest solution. If we just considered ReadyButton and FavouriteButton it would be quite a simple change because they use only bindables for their tooltip. However we have to consider the tooltip of ReadyButton actually gets overriden by PlaylistsReadyButton and MultiplayerReadyButton which each use several non-bindable variables in determining their tooltip text.
I'd have to add new bindables in several classes and it would become a quite complicated chain of bindable updates.

That or make a new subclass of RoundedButton with tooltip text setter. That seems like the correct OOP approach.

@@ -71,6 +71,8 @@ public virtual void EndPlacement(bool commit)
PlacementActive = PlacementState.Finished;
}

public virtual SnapType SnapType => SnapType.All;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs xmldoc

@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Oct 7, 2024
@peppy
Copy link
Sponsor Member

peppy commented Oct 7, 2024

No idea about the tooltip changes I just wangs some shit to make it compile so I could comment on the UX.

  • I don't think the right-click method needs to exist. You can just left-click twice to get the same behaviour right?
  • I preferred this being on the left, as it clears the user's selection and behaves like a modal operation but now has no indication that it will do that. Can we move it back?
  • It seems to maintain a spacing under 64 while the limit is 128, is there a reason for that?

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Updated UX feedback.

@peppy peppy removed the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Oct 7, 2024
@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 7, 2024

  • I don't think the right-click method needs to exist. You can just left-click twice to get the same behaviour right?

Yes left-click twice has the same behaviour, but I prefer to use a single right-click instead. It doesn't hurt to have a little redundancy to make it a bit easier to use right?

  • I preferred this being on the left, as it clears the user's selection and behaves like a modal operation but now has no indication that it will do that. Can we move it back?

It already clears the user's selection, but if you prefer it being on the left I'll move it there.

  • It seems to maintain a spacing under 64 while the limit is 128, is there a reason for that?

There's a good reason for that. I made the spacing match the range of the grid spacing cycle hotkey which is [4,64), so you can always cycle the spacing value back to the original value you got from the grid placement tool. I think these are all reasonable spacing values, beyond that there are so few snappable points on the screen that it is barely of any utility. The limit on the spacing slider is intentionally bigger for the off-chance you want to use a big spacing, so you can set that manually.

The grid placement tool and the spacing cycling hotkey are tools that set the spacing automatically so I think it makes sense to restrict them to a range of more reasonable values.

I hope to get this in the next release, so I'll be available to reply to any feedback ASAP

@peppy
Copy link
Sponsor Member

peppy commented Oct 7, 2024

It already clears the user's selection, but if you prefer it being on the left I'll move it there.

Let me rephrase: because it clears the selection, I think it should be on the left. Users are understanding that their selection will be lost when changing tools here. A button on the right randomly nuking a selection is unexpected.

@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 7, 2024

Let me rephrase: because it clears the selection, I think it should be on the left. Users are understanding that their selection will be lost when changing tools here. A button on the right randomly nuking a selection is unexpected.

That makes sense. I've moved it back to the left toolbox.

@peppy peppy self-requested a review October 7, 2024 15:44
@peppy
Copy link
Sponsor Member

peppy commented Oct 7, 2024

I'd either call the toolbox item "Grid" or "Place grid". The first may fit better with the other tools? @bdach thoughts?

Also I think the FA icon looks nicer:

diff --git a/osu.Game.Rulesets.Osu/Edit/GridFromPointsTool.cs b/osu.Game.Rulesets.Osu/Edit/GridFromPointsTool.cs
index bc143886ce..11c97d78d9 100644
--- a/osu.Game.Rulesets.Osu/Edit/GridFromPointsTool.cs
+++ b/osu.Game.Rulesets.Osu/Edit/GridFromPointsTool.cs
@@ -3,7 +3,6 @@
 
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Sprites;
-using osu.Game.Graphics;
 using osu.Game.Rulesets.Edit;
 using osu.Game.Rulesets.Edit.Tools;
 using osu.Game.Rulesets.Osu.Edit.Blueprints;
@@ -13,7 +12,7 @@ namespace osu.Game.Rulesets.Osu.Edit
     public partial class GridFromPointsTool : CompositionTool
     {
         public GridFromPointsTool()
-            : base("Change grid")
+            : base("Place grid")
         {
             TooltipText = """
                           Left click to set the origin.
@@ -23,7 +22,7 @@ Right click to only set the origin.
                           """;
         }
 
-        public override Drawable CreateIcon() => new SpriteIcon { Icon = OsuIcon.EditorPlaceGrid };
+        public override Drawable CreateIcon() => new SpriteIcon { Icon = FontAwesome.Solid.DraftingCompass };
 
         public override PlacementBlueprint CreatePlacementBlueprint() => new GridPlacementBlueprint();
     }

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

As above

@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 7, 2024

I'd either call the toolbox item "Grid" or "Place grid". The first may fit better with the other tools? @bdach thoughts?

Also I think the FA icon looks nicer:

I agree. "Grid" fits better in the theme of the other placement tools.

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.

"Center on selected object" doesn't change grid coordinates
4 participants