godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Upgrades to the TileMapPattern System. Related to a current pull request. #10512

Open MendicantNinja opened 3 weeks ago

MendicantNinja commented 3 weeks ago

Describe the project you are working on

Related to this PR: https://github.com/godotengine/godot/pull/95826. I am making a traditional 2D Roguelike similar to TOME, Caves of Qud, Quasimorph, or Zorbus. I'd like to use prefabricated rooms like Zorbus or Quasimorph for dungeon generation. Or at least have it as one of my tools for procedurally generating the world. I'd like to be able to set down patterns that have furniture on a layer, walls on another layer, and floors on another.

Describe the problem or limitation you are having in your project

The current TileMapPattern (referred to as Patterns from now on) system is not adequate for organizing these prefabricated rooms. Patterns cannot even be reindexed once created. Nor can you place multi-layer patterns (E.g. a house with a floor, wall, furniture, and roof tilemaplayers.)

Describe the feature / enhancement and how it helps to overcome the problem or limitation

https://github.com/godotengine/godot/pull/95826 Here's a video of the system in action. I do a short demo of some super basic (North walls and rooms only!) procedural generation I made in a day. I use a graphically complex (3 tile high) tileset+furniture to show how fine-tuned dungeon generation is possible with the system.

-Patterns are now organized into pattern sets as opposed to 1 big list. Sets can be renamed, shuffled around, or deleted (see video). This is useful for organization as well as procedural generation (picking at random from a specific list of patterns, as opposed to a single mega list or range.) -Patterns are now able to be renamed, shuffled around, or deleted. Previously patterns could not be resorted and their index was fixed once created. (to my knowledge and those of others). -Patterns can now be multilayer patterns. So if you want to procedurally generate a village full of houses with a roof layer, wall layer, furniture layer, and floor layer. You can!

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

https://github.com/godotengine/godot/pull/95826 Link to the PR showing a video of how it works as well as the modifications to the source code in detail.

Follow up from this Issue proposal where I took Kobewi's suggestions to heart and scaled back some of the features (e.g. marks on the pattern.). While implementing the pattern sets and multilayer system for more organized storage. https://github.com/godotengine/godot-proposals/issues/7591

If this enhancement will not be used often, can it be worked around with a few lines of script?

You don't have to work with the new multi-layer TileMapPattern system at all if you don't need it. Or the TileMapPattern system in general. It is much more usable than the old system that doesn't allow reorganizing the patterns after creation and I don't know why someone would use the old system.

Is there a reason why this should be core and not an add-on in the asset library?

I also think it meets meets the basic guidelines set out here for merging something. https://godotengine.org/article/will-your-contribution-be-merged-heres-how-tell/

I will reply to some suggestions made in the comments of the PR below:

MendicantNinja commented 3 weeks ago

Also, as others have expressed, breaking compatibility like this is a big no-no. There needs to be a way for TileMapPatterns to seamlessly convert to the new format.

-To be clear, the only effect of upgrading to the new pattern system is that the old patterns one has are erased. It doesn't break projects or anything. -I'm not sure how many people actually use the patterns system as is because of how difficult it is to work with. You can't resort patterns after creating them, so I imagine it's somewhat rare to have a patterns list larger than 5 patterns. -Moreover there's a cheap fix that people will have to do anyway even if I made a conversion. you can set your patterns into a TileMapLayer in 4.3, save your project, then put them into a pattern set in 4.4. If someone wants to use multiple pattern sets they would have to perform this process anyway as all their set_pattern and get_pattern methods now require a pattern_set_index parameter. I'd argue for a rare exception to breaking backwards compatibility here due to how underutilized the system is, how converting could be difficult (depending on how they're using their patterns/if they want to take advantage of the new system). As well as the workaround for the (I suspect), very small number of people that make extensive use of the current patterns system. This is what the discussion is for though, I haven't seen anyone use the current patterns system at scale (many try, then they quit when they realize they can't reorganize patterns), but maybe they exist.

  • I'd rather not change the TileMapPattern class. Instead, you should create a dedicated class (likely called TileMapMultiLayerPattern) that uses single-layer TileMapPatterns internally.

-I don't agree with this (if I'm understanding the idea of having a new class correctly). Because it causes a few problems in addition to being a lot of work. -Whenever I need to do something, even something simple like get()ting or set()ting the name of a pattern during renaming as shown in the video, I would have to write two two different methods for each class or check which class it is. I would have to rewrite stuff like tile_set.get_pattern() for both classes, anything that returns or accepts a pattern as a parameter would need two seperate methods and I would have to check which one I'm dealing with. New documentation for the class? There's just a lot of work to do to create a new class. -Although I concede after writing all that and thinking that a lot of it could be mitigated by making "MultiLayerPattern" inherit from TileMapPattern, but I just don't think the benefits outweigh the costs. What are the advantages of doing it this way?

  • The reference is likely not the place to have that much information about how to use the feature. This should go into the docs IMO.

Agreed. Is it possible to write to the docs (the website?) from the XML files? I will look into doing this and remove the very long TileMapPattern description for the next/updated PR if there's a good chance this could be merged.

  • Compatibility should be kept (which is kind of 80% allowed by point 1, but it will need more work to port the sub-resources to the new organisation)

My thoughts on this are above.

  • I am not convinced with the multi-layer selection mode. Instead, this should be implement via the built-in multi-node selection: if you select multiple layers, the editor should adapt an allow you to select on multiple layers at the same time (and create a pattern).

I agree with you in principle that that's a slightly more intuitive/obvious way of doing things (although the ML selection mode button as is works and is functional, let's remember). I also agree there's an advantage of being able to select only a few layers for the pattern instead of selecting all of them. (Although why would you only want to get the furniture in a room/pattern you're creating haha.) But there is a potential issue I'd like advice/help on.

The way the code currently works is it checks if the multi-layer selection button is on then directs the selection code accordingly. For example, if it's on then "TileMapLayer get_pattern_multi_layer()" is called when selecting stuff and grabs all the tiledata on the all the layers. I use the tile_map_layers_in_scene_cache (sic?) variable to do that. How can we set it up in TileMapLayerEditor so that we know what TileMapLayers are selected when we call TileMapLayer get_pattern() or get_pattern_multi_layer? We have to able to pass that data between the two. I think it would be a good idea if the system was merged as is, then we both worked together to switch over to doing it by selecting layers rather than checking the multi_layer_selection_mode flag.

Aside from that it looks promising already. I think being able to sort/arrange patterns is a good idea. Though I wonder it is worth the added complexity. Maybe patterns should all be saved as resource files instead ? 🤔

Thank you. It's already pretty useful for my game so far. I definitely think it's worth the added complexity, a lot of people think patterns are unusable because they can't be reorganized after being created. Regarding the latter, patterns already inherit/are a resource. Do you mean making an overarching resource file that stores ALL the patterns? Or making pattern sets a resource? Or making patterns a resource (they already are I think?).

I'm curious what the experienced contributors/maintainers think are the odds of this being merged if I followed the suggestions that are agreed upon by the maintainers? If it were just for my own personal use, I'd stop here and be writing stuff for proc gen directly into the source code for my own use using the pattern system as a base.

RedMser commented 3 weeks ago

I'd argue for a rare exception to breaking backwards compatibility here due to how underutilized the system is,

How do you know it is underutilized? Even so, upgrading between minor versions should not cause data loss. I'd say it would have to wait until 5.0 if it should be a breaking change like such.

Is it possible to write to the docs (the website?) from the XML files?

There is a separate godot-docs repository which uses .rst files for documentation. Should probably go in the tutorials section https://github.com/godotengine/godot-docs/

MendicantNinja commented 3 weeks ago

How do you know it is underutilized? Even so, upgrading between minor versions should not cause data loss. I'd say it would have to wait until 5.0 if it should be a breaking change like such.

Whenever I would ask in the Godot discord if anybody used the pattern system/how they used it (when I was trying to figure out how to implement it into my game). They'd usually say "I didn't know such a thing existed." Or "I had trouble with deleting/organizing the patterns.". I never saw anyone using the pattern system. But I'll ask around again.

I'll rewrite the documentation to make the reference less verbose and the documentation more dense. Thank you for the information on .rst files.

MendicantNinja commented 3 weeks ago

I stand corrected! I did find someone who uses patterns as-is after the above post. In their words "I have 22 patterns, but 6 of them are unusable due to being unable to delete or reorder them.". They said they don't care about backwards compatibility a lot. But nonetheless if there's one person who does care, then it's worth doing. I'll see if I can add backwards compatibility this week along with updating the documentation.

groud commented 3 weeks ago

Adding a new class for multilayer patterns requires a bit more work on the editor side, but it makes the API easier to understand and easier to use for users. So even if that's duplicated code for some getters/setters, I truly believe it's worth it.

The multi-layer patterns are quite a different use case than single-layer patterns. Forcing everyone who wants to use a single layer (which is a ton of situations) to provide a layer number of 0 every time is annoying. The fact TileMap was divided into TileMapLayers, and that this change was nicely welcomed, kind of shows there's an issue in making APIs more complex with a layer number.

I think you probably believe that all patterns (whether they have one or several layers) should be handled the same in the editor, but I believe those are different use cases that can be handled in separate tabs / modes in the editor. It would also help with the issue that, with the multi-selection mode, the saved pattern might not fit the number of selected TileMapLayers that are currently selected. Separating those will make it clearer to users what they can/cannot do with each type of pattern.