-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 ability to change position, spacing, and rotation of the positional snap grid in the editor #26309
Conversation
This caused issues in rendering the outline of the grid because the outline was getting masked at some resolutions.
UX-wise I'm missing a facility to reset settings to default here. Hitting the default values on everything but grid size is rather finicky and requires pixel perfect precision. 2024-05-24.14-45-03.mp4I guess this is not unique to this particular toolbox so I guess we can handle this as a follow-up, but I do think revert to default should be easily accessible somehow. The other weird kink in this is that grid size will be preserved if you save (because it's stored to the One thing related to the above that needs checking is what stable will do when it encounters a beatmap exported via lazer with such custom grid size specs (that isn't the default presets of 4/8/16/32). Will do a quick pass on code otherwise. @peppy would appreciate a quick look from you here too as to whether you wanna buy into this direction. |
osu.Game/Screens/Edit/Compose/Components/LinedPositionSnapGrid.cs
Outdated
Show resolved
Hide resolved
private bool isMovingTowardsBox(Vector2 currentPosition, Vector2 step, Vector2 box) | ||
{ | ||
return (currentPosition + step).LengthSquared < currentPosition.LengthSquared || | ||
(currentPosition + step - box).LengthSquared < (currentPosition - box).LengthSquared; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work? Why is checking distance against the top left and bottom right corner sufficient? I'd imagine that you'd wanna check all four corners (although I can't find quick counterexamples)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm 100% sure checking 2 corners is sufficient in this scenario where we've already checked if the current position would intersect the box.
A counterexample would be an instance where we have !lineDefinitelyIntersectsBox(currentPosition)
and !isMovingTowardsBox
and lineDefinitelyIntersectsBox(currentPosition + step)
. That would mean currentPosition
(p1) is outside the box, currentPosition + step
(p2) is at least as far away from both corners as p1, and the perpendicular line on p2 intersects the box.
The example I drew shows kind of a worst-case scenario where p1 is as close as possible to both corners to allow p2 to be as close as possible. However still you can see the perpendicular line on p2 doesnt intersect the box.
I have a proper proof showing that a counterexample is impossible but its quite long and complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to come up with at least an intuition as to why this is enough but I'm not sure I convinced myself so I guess I'll just have to take this at face value and fix it if it ever breaks.
{ | ||
var drawSize = DrawSize; | ||
var rot = Quaternion.FromAxisAngle(Vector3.UnitZ, MathHelper.DegreesToRadians(GridLineRotation.Value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using quaternions here feels a bit like shooting a fly with a cannon but sure I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid calculating the sin and cos more than necessary. If there was a way to multiply a Matrix2 and a Vector2 I would've went with that.
The slider bars reset to the default value when you double-click them. This is a general feature of all such sliders.
I'm reluctant to append stuff to the save file as long as we are using the legacy encoder, because I might break stuff with stable compatibility.
It seems like stable clamps the grid size between 0 and 32 and then actually allows you to use any integer grid size in that range. Decimal grid sizes cause it to not load the editor, but the grid size always gets truncated to integer before saving. |
I'm not sure whether this is an indictment of my ignorance or the lack of discoverability of the feature in question but I genuinely was not aware this was a thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this seems fine? will still need that signoff from @peppy on feature existence / general UX.
I think this feels good apart from the fact that it doens't save.
I wouldn't be against adjusting stable in order to support (ignoring) this. I think it's the first thing mappers are going to ask for when this feature hits, because it's a bit annoying to have to set the grid up each time you enter the editor. Two possible ways which will not break stable are either:
|
Can we get this in as is and worry about saving when users complain? There's a whole series of pulls after this one with various other grid types and I feel like before that is all done, blocking this on persistence woes seems not that helpful. Especially so that they entail changing stable. Users may well complain about saving but at least they'll get a new feature? |
I did some testing and it seems like stable gracefully ignored extra fields in under the [Editor] section, so maybe fixing persistence wouldn't be difficult. |
Sure. I'd hope we can get it in alongside this PR rather than waiting for complaints. I'll continue review of this change and look to merge it. |
Part of #26303
Adds just the ability to change the position, spacing, and rotation of the rectangular position snap grid.