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

Replace half-pixel snapping with grid snapping in tile polygon editor #73495

Closed
wants to merge 1 commit into from

Conversation

ygypt
Copy link

@ygypt ygypt commented Feb 17, 2023

Closes godotengine/godot-proposals#5847

Similar to #70488, but uses subdivisions instead of raw pixel counts.

there's p much no instances where you would want to free-hand the collisions, as it results in very inconsistent movement. the half-pixel snapping doesn't really help in that regard, and is indistinguishable from free-handing when working with most sprites above 32x32

snap_to_grid

@MewPurPur
Copy link
Contributor

Could you show how it looks visually?

Also, since you didn't amend your commits, you need to follow https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase to squash them.

@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@ygypt
Copy link
Author

ygypt commented Feb 17, 2023

😅 oh my bad, ill follow the squashing guide tn when i get out of work. also weird, i coulda sworn i linked visual studio to my github account

@ygypt
Copy link
Author

ygypt commented Feb 18, 2023

ok that was hell but i did it somehow it took a million steps of tinkering around with files and opening another branch im sure it coulda been done in like 3 little commands if i was any good at git XD

in the future im gonna do changes on a dedicated branch i was just editing master the whole time lol

@anvilfolk
Copy link
Contributor

Hahaha, yeah, definitely use branches! For what it's worth, I had never done before I started contributing. There's definitely a startup cost to figuring it all out but it's really cool & useful to know it once you're past that initial hump :)

@groud
Copy link
Member

groud commented Feb 20, 2023

I am ok with the idea of implementing subdivisions. However:

  • half-pixel snapping should not be replaced. Subdivision should be an added mode, as, most of the time, you want half-pixel enabled anyway.
  • the subdivision SpinBox takes too much room there. It should be moved either into popup menu (likely in the three-dots menu), or in a popup window. Another solution, if we can't manage to integrate it in the popup menu, would be to simply have an "Increase subdivisions" and a "Decrease subdivisions" entries in the popup menu. That could be enough.
  • Maybe the half-pixel snapping button will be a bit useless, it could be moved to a check entry in the popup menu.
  • The subdivision amount should be saved as a project settings so that it is share with all other editors.

@ajreckof
Copy link
Member

ajreckof commented Feb 20, 2023

  • half-pixel snapping should not be replaced. Subdivision should be an added mode, as, most of the time, you want half-pixel enabled anyway.

I'm not sure of two things about that. First is why would we still need it since subdivisions can do the same if you just put the right number of subdivisions. The second is how they would interact with each other.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 20, 2023

The subdivision amount should be saved as a project settings

You probably mean project metadata.

Also I don't get how half-pixel snap is useful. Why would someone want to snap to less than 1 pixel?

@groud
Copy link
Member

groud commented Feb 21, 2023

You probably mean project metadata.

Yeah, my bad. That's likely better.

Also I don't get how half-pixel snap is useful. Why would someone want to snap to less than 1 pixel?

Because, otherwise, it would not snap at all, as polygons use Vector2 and not Vector2i. Half-pixel snapping is needed as you might want to place a polygon point in a middle of a pixel, if you have an odd number of pixels for example.

In general, it's something you want to keep enabled anyway, that's why I suggested to move it to a popup menu entry instead.

I'm not sure of two things about that. First is why would we still need it since subdivisions can do the same if you just put the right number of subdivisions. The second is how they would interact with each other.

Because it's super annoying to change the subdivisions when you only want to disable the grid for a little while and not have vertices in positions like (0.03632, 0.0042) instead of (0, 0).

Regarding how they will interact with each other, it's simple:

  • If grid snapping is enabled, ignore half-pixel snapping and snap to the grid
  • If grid snapping is disabled, snap to half-pixel if half-pixel snapping is enabled.

@ygypt
Copy link
Author

ygypt commented Feb 22, 2023

thanks for the suggestions guys! ill update the code when i have the free time. i appreciate the optimization @ajreckof!

on the topic of half-pixel vs grid, grid should certainly be the default, bc for the majority of use cases the collisions would just need to snap to 3x3 or 4x4. but having half-pixel be on as the default fall-back is also a great idea

ill bring the half-pixel back in, on by default, with a checkbox in the menu for if you want to disable it. snap to grid will take precedence over half-pixel (if elif else i reckon)

i dont think we should move the spinbox tho. it fits very snuggly into that bar, and the minimum width of the container already accommodates for it. putting it in a popup or using dropdown buttons would be cumbersome, it's far more intuitive to just have it available, and since implementing it for my own use ive found that it's the most frequently selected item in that bar (high rez 2d platformer)

@groud
Copy link
Member

groud commented Feb 22, 2023

on the topic of half-pixel vs grid, grid should certainly be the default, bc for the majority of use cases the collisions would just need to snap to 3x3 or 4x4. but having half-pixel be on as the default fall-back is also a great idea

Whether the grid is enabled should be stored as a global project metadata, as well as the subdivision count.
I don't think I would enable it by default though, as it requires some configuration (the subdivision) and will not work fine with isometric and hexagonal shapes. I prefer having is disabled at startup so it does not bother every isometric/hexagon tiles users.

ill bring the half-pixel back in, on by default, with a checkbox in the menu for if you want to disable it. snap to grid will take precedence over half-pixel (if elif else i reckon)

Sounds perfect!

i dont think we should move the spinbox tho. it fits very snuggly into that bar, and the minimum width of the container already accommodates for it. putting it in a popup or using dropdown buttons would be cumbersome, it's far more intuitive to just have it available, and since implementing it for my own use ive found that it's the most frequently selected item in that bar (high rez 2d platformer)

Yeah, but I would prefer to have that bar as small as possible. On small screens, the column already takes a lot of space, and I don't think subdivision is something most users change often. So I don't want to worsen most people's experience on small screens.

I get that, as a dropdown entries, it might be a bit weird. But I believe that, with keyboard shortcuts, it should not be a big issue for those who change it often. So for me that's the best compromise.

@nklbdev
Copy link
Contributor

nklbdev commented Feb 25, 2023

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

@svendixon
Copy link

Is there a reason why you chose subdivisions instead of pixels? Also having an option to display the grid so we can see where it's going to snap to, would be nice.

@MewPurPur
Copy link
Contributor

Are you still around? I'd really like to have this enhancement.

@akien-mga
Copy link
Member

Superseded by #70488.

@akien-mga akien-mga closed this May 8, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Dec 2, 2023
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
10 participants