godot icon indicating copy to clipboard operation
godot copied to clipboard

New TileMapPattern GUI

Open MendicantNinja opened this issue 1 year ago • 6 comments

Pull request with a cleaner commit history similar to PR #95826 https://github.com/godotengine/godot/pull/95826. The difference is that it doesn't contain the multilayer TileMapPattern system in favor of just adding a new TileMapPattern GUI that is backwards compatible with the old TileMapPattern system (A new pattern set is created and all old patterns are imported to it).

MendicantNinja avatar Sep 23 '24 03:09 MendicantNinja

Your rebase went wrong, you can fix this by:

git reset --hard 20750b6

And then performing your rebase and finally pushing with:

git push --force

AThousandShips avatar Sep 23 '24 20:09 AThousandShips

Please use a rebase, not a merge to update your branch

AThousandShips avatar Sep 23 '24 20:09 AThousandShips

I did some testing:

  • The default width of the left panel is too small and the label is trimmed. image
  • Context menu appears at wrong position. It's always above cursor when clicking empty space and at weird offset when clicking item: image image Although I'd completely remove the menu, as the UX is awkward. Add some "Add Pattern Set" button at the bottom and display sets with a delete button.
  • The rename popup for patterns always appears in the corner and can't be cancelled:

https://github.com/user-attachments/assets/f3678d07-6765-4a37-8364-3ea3a14dea18

  • When you drag a new pattern, the current pattern set will be deselected

https://github.com/user-attachments/assets/8948728a-464f-45b9-a122-64bd827ca9b7

  • When opening Patterns tab for the first time, I think the first set should be selected automatically. Currently you need extra click to see patterns
  • If a pattern has default name, the displayed name is changed when rearranging:

https://github.com/user-attachments/assets/b71af7a7-b286-47f0-af8c-5751e1892fdc

  • I think the pattern array should be called patterns not pattern_set.
pattern_set_0/name = "Set"
pattern_set_0/pattern_set = []
  • The help labels need improving. They mix with the list and disappear as content is added. image I know we have similar labels in Tiles tab, but these are much bigger. It would be better to have some sort of info button or foldable info panel (though I'm not sure if we have something similar already). If not, they should be at least fixed at the bottom and not disappearing.

Other than UX problems, the feature works correctly. You can create, rename and reorder sets and patterns. Also compatibility is handled properly.

KoBeWi avatar Oct 05 '24 19:10 KoBeWi

I did some testing:

  • The default width of the left panel is too small and the label is trimmed. image
  • Context menu appears at wrong position. It's always above cursor when clicking empty space and at weird offset when clicking item: image image Although I'd completely remove the menu, as the UX is awkward. Add some "Add Pattern Set" button at the bottom and display sets with a delete button.
  • The rename popup for patterns always appears in the corner and can't be cancelled:

godot.windows.editor.dev.x86_64_ngng4Jm8nU.mp4

  • When you drag a new pattern, the current pattern set will be deselected

godot.windows.editor.dev.x86_64_MS9lqQ6kAJ.mp4

  • When opening Patterns tab for the first time, I think the first set should be selected automatically. Currently you need extra click to see patterns
  • If a pattern has default name, the displayed name is changed when rearranging:

godot.windows.editor.dev.x86_64_Z8VNGYtLVc.mp4

  • I think the pattern array should be called patterns not pattern_set.
pattern_set_0/name = "Set"
pattern_set_0/pattern_set = []
  • The help labels need improving. They mix with the list and disappear as content is added. image I know we have similar labels in Tiles tab, but these are much bigger. It would be better to have some sort of info button or foldable info panel (though I'm not sure if we have something similar already). If not, they should be at least fixed at the bottom and not disappearing.

Other than UX problems, the feature works correctly. You can create, rename and reorder sets and patterns. Also compatibility is handled properly.

Noted. I will take this feedback and iterate upon it to improve the UX. Most of this feedback is reasonable.

MendicantNinja avatar Oct 07 '24 00:10 MendicantNinja

Have you considered implementing a path instead of pattern sets? I feel it would be more flexible to just let people create folders however they want in the list of patterns.

groud avatar Oct 07 '24 11:10 groud

Have you considered implementing a path instead of pattern sets? I feel it would be more flexible to just let people create folders however they want in the list of patterns.

I have a little bit. But decided it wasn't worth going to that level of complexity and wanted to avoid using strings/paths to access things wherever possible. Can you elaborate on what you mean? Rather than a pattern set>patterns you would do FileFolderName>InnerFileFolderName>Patterns+Folders containing patterns and stuff like that?

As with most other things in development I'm sure it can be done but it's a question of if it is needed and worth the effort. I think it would require a good deal of effort and a rewrite of the code as I have it. It would be a bit more fickle and complex with regards to how those patterns are accessed (e.g a path every time in order to get a pattern, if the path changes you're in trouble! Would patterns still have indexes as an additional parameter to access them? How would selecting a pattern between indexes 0-5 at random work if not?). Although, I would need to know exactly what you mean/want regarding the folder system to say for certain. The ultimate question much like any other feature added to Godot is "is there a situation where pattern sets would not suffice and a more in-depth/flexible system like folder paths would be needed?". In my personal experience working with patterns the answer is no.

MendicantNinja avatar Oct 07 '24 23:10 MendicantNinja