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 ability to change position, spacing, and rotation of the positional snap grid in the editor #26309

Merged
merged 36 commits into from
Jun 5, 2024

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Jan 1, 2024

Part of #26303

Adds just the ability to change the position, spacing, and rotation of the rectangular position snap grid.

OliBomby and others added 3 commits January 1, 2024 16:21
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 May 24, 2024

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.mp4

I 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 .osu file), but everything else isn't. Note that I'm not necessarily saying that the other settings should be saved there too.

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.

Comment on lines +64 to +68
private bool isMovingTowardsBox(Vector2 currentPosition, Vector2 step, Vector2 box)
{
return (currentPosition + step).LengthSquared < currentPosition.LengthSquared ||
(currentPosition + step - box).LengthSquared < (currentPosition - box).LengthSquared;
}
Copy link
Collaborator

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)

Copy link
Contributor Author

@OliBomby OliBomby May 24, 2024

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.

paintdotnet_MSPN1Mj7zJ

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.

Copy link
Collaborator

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));
Copy link
Collaborator

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

Copy link
Contributor Author

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.

@OliBomby
Copy link
Contributor Author

OliBomby commented May 24, 2024

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.

The slider bars reset to the default value when you double-click them. This is a general feature of all such sliders.

The other weird kink in this is that grid size will be preserved if you save (because it's stored to the .osu file), but everything else isn't. Note that I'm not necessarily saying that the other settings should be saved there too.

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.

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).

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.

@bdach
Copy link
Collaborator

bdach commented Jun 3, 2024

The slider bars reset to the default value when you double-click them. This is a general feature of all such sliders.

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.

bdach
bdach previously approved these changes Jun 3, 2024
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 this seems fine? will still need that signoff from @peppy on feature existence / general UX.

@bdach bdach requested a review from peppy June 3, 2024 10:38
@peppy
Copy link
Sponsor Member

peppy commented Jun 5, 2024

I think this feels good apart from the fact that it doens't save.

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.

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:

  • Adding a new field to the .osu file with the extra data
  • Storing in GridSize using another colon for separator between new data (bit hacky but will work)

@bdach
Copy link
Collaborator

bdach commented Jun 5, 2024

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?

@OliBomby
Copy link
Contributor Author

OliBomby commented Jun 5, 2024

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.
Just to be safe though, I'd like to get this PR in first and fix persistence in a subsequent PR so we can make sure it doesnt break anything.

@peppy
Copy link
Sponsor Member

peppy commented Jun 5, 2024

Just to be safe though, I'd like to get this PR in first and fix persistence in a subsequent PR so we can make sure it doesnt break anything.

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.

@peppy peppy merged commit 3185987 into ppy:master Jun 5, 2024
12 of 17 checks passed
@OliBomby OliBomby deleted the grids-1 branch June 5, 2024 14:38
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.

3 participants