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 proper snapping to tile polygon editor #70488

Merged
merged 1 commit into from
May 8, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Dec 23, 2022

Closes godotengine/godot-proposals#5847
godot windows editor dev x86_64_2jQS4NjZQ1
The grid does not draw, but I could add it if it's important.

@kleonc
Copy link
Member

kleonc commented Dec 23, 2022

Made like that it's not possible to snap to subpixel values. So e.g. for odd sized tiles you won't be able to snap to the tile's center. Meaning being able to snap to half pixels is a must. So at least min/step for the spin boxes should be set to 0.5 (maybe even more granular step customization should be allowed?:thinking:).

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 23, 2022

Odd-sized tiles are a weird use-case and I never understood why it was half-pixel in the first place, but setting step to 0.5 should be enough.

@groud
Copy link
Member

groud commented Jan 13, 2023

I believe the feature is fine, but I think it takes too much space as it is and is not the best to configure IMO.
What I would do:

  • Replace "grid step" in pixels by a subdivision value (defaulting to 3 or 4 I guess ?), as I don't think there are uses cases where you would need the grid with a size that's not a division of the tile size. It is also easier to setup IMO.
  • Move this subdivision settings to a popup, likely accessible from the option menu dropdown.
  • Add a single toggle button to enable/disable grid snapping.
  • If we consider the bar is getting too large, the "snap to pixel" option might be moved inside the dropdown menu.

@ygypt
Copy link

ygypt commented Feb 15, 2023

I believe the feature is fine, but I think it takes too much space as it is and is not the best to configure IMO. What I would do:

  • Replace "grid step" in pixels by a subdivision value (defaulting to 3 or 4 I guess ?), as I don't think there are uses cases where you would need the grid with a size that's not a division of the tile size. It is also easier to setup IMO.
  • Move this subdivision settings to a popup, likely accessible from the option menu dropdown.
  • Add a single toggle button to enable/disable grid snapping.
  • If we consider the bar is getting too large, the "snap to pixel" option might be moved inside the dropdown menu.

i really like your idea of a simple one-line subdivision value, although imo putting it into a dropdown would be unnecessary. i guess its time to learn c++, i'd love to contribute to this PR :)

@nklbdev
Copy link
Contributor

nklbdev commented Feb 26, 2023

I created small editor plugin to help with this problem. Maybe it helps to add custom vertex snapping in future

@KoBeWi KoBeWi requested a review from a team as a code owner May 1, 2023 19:29
@KoBeWi
Copy link
Member Author

KoBeWi commented May 1, 2023

Updated based on feedback (including #73495 feedback)

  • changed grid size to subdivisions
  • the subdivision SpinBox is made smaller, so it doesn't take much space
    • it can be even smaller if we only allow 1-digit subdivisions
  • snap setting and subdivision is stored in project metadata
  • the grid is drawn

The original feedback was to move half-pixel to popup and make it separate from grid snap, with grid taking priority. This has a problem though: if grid is enabled, toggling half-pixel snap has no effect. So there is awkward setup where there are 4 states, but 2 are effectively the same. So instead of weird toggle/button combo, I went with 3-state popup.

godot.windows.editor.dev.x86_64_JxVow5t71w.mp4

You can either disable snap, enable half-pixel snap or enable grid snap.

@akien-mga akien-mga modified the milestones: 4.x, 4.1 May 8, 2023
@akien-mga akien-mga merged commit b791a7a into godotengine:master May 8, 2023
@akien-mga
Copy link
Member

Thanks!

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.

Add a snapping grid to polygon editors in the TileSet editor
6 participants