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 grid snapping for box shape gizmos #82381

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

dervus
Copy link
Contributor

@dervus dervus commented Sep 26, 2023

Fixes #81916

I've rewritten Gizmo3DHelper logic for its box "extrusion" mode. In this version it applies snapping directly to box size and then adjusts box position by ensuring that opposite end of the box doesn't move as result of the operation. This way box size doesn't deviate from the snap grid.

Position (origin) adjustment logic is thus:

  1. Calculate initial offsets of box sides on selected axis.
  2. Adjust offset of the side we're extruding to accommodate new box size.
  3. Adjust box position to be in the middle between unmoved side and extruded side.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 27, 2023

Shouldn't the gizmos get snapped to the grid?

godot.windows.editor.dev.x86_64_QZiiSPkNMR.mp4

They move in correct increments, but not sure if this is intended.

@dervus
Copy link
Contributor Author

dervus commented Sep 27, 2023

I'm quite new to Godot, so I'm not sure myself if it's intended or not, but AFAIK all translation ops with grid snapping just force target property to change in integer increments (0.1 increments if shifted). I think that makes sense for box sizing, though because when I'm working with several CSG shapes I find it handy if they all snap like this, so they can fit together even then their origin is not on grid.

@dervus
Copy link
Contributor Author

dervus commented Sep 27, 2023

On the other hand, I think it would be quite simple to make it always snap to visual grid. This would cause size property to deviate from being integer, but it always could be corrected by pulling the other side of the box. However, this would be inconsistent with translation gizmo behavior, and also would be a change from original behavior of this specific gizmo, so I assume even if it is desired, it shouldn't be a part of this PR?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 27, 2023

I assume even if it is desired, it shouldn't be a part of this PR?

Yeah, I guess.

editor/plugins/gizmos/gizmo_3d_helper.cpp Outdated Show resolved Hide resolved
editor/plugins/gizmos/gizmo_3d_helper.cpp Outdated Show resolved Hide resolved
editor/plugins/gizmos/gizmo_3d_helper.cpp Outdated Show resolved Hide resolved
editor/plugins/gizmos/gizmo_3d_helper.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Seems fine.
CC @Calinou You might want to test the fixed behavior.

@akien-mga akien-mga requested a review from Calinou October 6, 2023 10:21
@akien-mga
Copy link
Member

Could you squash the commits? See PR workflow for instructions.

"Extruding" box face (i.e. moving it without holding Alt key) will now
always snap new box size exactly to the nearest snap step.
@akien-mga akien-mga merged commit fc6d6b2 into godotengine:master Oct 13, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@dervus
Copy link
Contributor Author

dervus commented Oct 13, 2023

Thanks for merging!

@JamesC01
Copy link

Thanks so much, just tested it and it seems to work well. This will make grey-boxing levels so much easier!

@dervus dervus deleted the issue-81916 branch October 20, 2023 02:56
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.

New 3D scaling gizmos not snapping to grid correctly
5 participants