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

Scene-tiles for tilemaps #9180

Closed
bojidar-bg opened this issue Jun 14, 2017 · 14 comments
Closed

Scene-tiles for tilemaps #9180

bojidar-bg opened this issue Jun 14, 2017 · 14 comments

Comments

@bojidar-bg
Copy link
Contributor

Part of what was mentioned in #5965, though somewhat related to #3637 as well. Also, will obsolete all "Add scripts to tiles" issues if implemented.

Why put whole scenes in tilemaps? "We already have scene instancing for that!"

TL;DR - using instanced scenes instead of tiles in various places bloats the scene tree and makes for a bad workflow. Instancing scenes to replace certain tiles in _ready() is just a manual implementation of the feature which has been somewhat popular so far.


Unlike #9179, this time I can't say that scene instancing is hacky. Actually, what we currently have has worked for ages without having any problems. Sadly, the current workflow is a bit unwieldy...

As an example, here is how a typical Minilens level looks like, shown alongside the scene tree:
image

The typical problem here is that it is hard to find a specific box, flower, bomb pickup, etc., unless you select it from the viewport. And if you want to move a part of the level slightly... you have to move all the respective boxes/flowers/pickups along.

As an additional bugger, you have to remember to turn on "use snap" and then "configure snap" to use the proper tile size. That, or calculate the positions of the various nodes by hand.

Some people have solved this problem by having some tiles which get replaced in _ready() with their real versions. This makes a slight performance hit on load times as well as being too much boilerplate. I think having it in core is going to ease things for a lot of people.

Ok, let's say the workflow can be probably improved, what do you propose?

  • Some tiles of the TileSet should be able to have a PackedScene.
  • When a tile of such a scene type is encountered, we don't draw anything in its place, but add a separate node with the instance (which has owner = null, so that it won't be saved).
  • If the instanced child does not extend CanvasItem, just spam the console with errors. Or maybe check if it has a set_position method, and call it.

Optional features:

  • In case the child gets removed from the tree for whatever reason, we just do a set_cell(x,y, -1) behind the scenes.
  • Provide a move_cell(from_x, from_y, to_x, to_y) / move_cellv(from, to), which would move the instance, instead of recreating a new one at the other position. This might have an additional flag that would cause it not to call set_position on the child again.
  • Allow scene tiles to also have a texture, so that they can be free to move around without leaving holes (note that this can be done by just having a Sprite as the root of the scene, but then it won't benefit from Tilemap's batching). This might somewhat conflict with the first optional feature.
@Zylann
Copy link
Contributor

Zylann commented Jun 14, 2017

Should these scene tiles be visible in the tree? They will also influence node indexes and the list of what one expects as child of the tilemap, but that's to be expected with scene tiles.

@bojidar-bg
Copy link
Contributor Author

@Zylann They won't be visible in tree, since you want your tree to be kept clean for what really matters. Also, you aren't supposed to edit them.

@kubecz3k
Copy link
Contributor

kubecz3k commented Apr 3, 2018

What's the status of this proposal? Do you think it's still needed?

@bojidar-bg
Copy link
Contributor Author

bojidar-bg commented Apr 9, 2018

@kubecz3k Yes, I think it is still needed, for pretty much the same reasons as before.
I guess I will work on it some time "soon", if no one takes is by then.

@Jummit
Copy link
Contributor

Jummit commented Sep 26, 2018

This is what I thought the tilemap node would do when I first saw it. Sadly you can only make tiles have a sprite and a collision, so its not the best for level design where you need pickups, lamps, enemys and much more.

@willnationsdev
Copy link
Contributor

I'm kind of confused by all the content here. Seems like there are a number of problems that this issue brings up.

The typical problem here is that it is hard to find a specific box, flower, bomb pickup, etc., unless you select it from the viewport.

If we set these things as hidden children of the TileMap, that would de-clutter the SceneTree (yay?), but it would also make it impossible to find a specific box, flower, bomb pikcup, etc. because selecting it would select the TileMap node. Wouldn't this solution further complicate this issue?

And if you want to move a part of the level slightly... you have to move all the respective boxes/flowers/pickups along.

Shouldn't these assets then be positioned as children of the TileMap node, so that they move relative to its position?

As an additional bugger, you have to remember to turn on "use snap" and then "configure snap" to use the proper tile size. That, or calculate the positions of the various nodes by hand.

Good point. But wouldn't a simpler solution be to make the canvas_item_editor_plugin's snap configurations be able to remember the last selected or ancestor TileMap node and defer to its snap configurations, that way you can use the traditional 2D editor to assign things in the same snap settings without having to hide the content within the TileMap itself? Maybe not that exactly, but I feel there's more we could do with improving editor tooling around the existing TileMap node to give this feature rather than making the TileMap assume control of the placement outright.

Perhaps define a more generic GridPlacement node that can point to a TileMap or GridMap and position other nodes appropriately in relation to the targeted map node? TileMap's purpose is batched drawing with a tileset/tile editor, yes? Mostly for environmental stuff. If you start adding scenes, it makes it more complicated to duplicate the standard 2D editor's features just for the TileMap functionality.

@bojidar-bg
Copy link
Contributor Author

If we set these things as hidden children of the TileMap, that would de-clutter the SceneTree (yay?), but it would also make it impossible to find a specific box, flower, bomb pikcup, etc. because selecting it would select the TileMap node. Wouldn't this solution further complicate this issue?

Well, if it is part of the TileMap, it will be easy to delete/move, but probably it would be impossible to select it and change just certain properties.

Shouldn't these assets then be positioned as children of the TileMap node, so that they move relative to its position?

Note the wording: part of the level. Usually, I keep my whole level as one tilemap (simplifying code that way), so if I move the tilemap, I would have moved my whole level.

But wouldn't a simpler solution be to make the canvas_item_editor_plugin's snap configurations be able to remember the last selected or ancestor TileMap node and defer to its snap configurations

Remembering snapping, and probably keeping it when creating new scenes would be cool, indeed.
Deferring snapping to the tilemap is somewhat fishier as an idea, since it either makes canvas_item_editor depend on tilemap or node2d know about snapping.

Perhaps define a more generic GridPlacement node that can point to a TileMap or GridMap and position other nodes appropriately in relation to the targeted map node?

I think no. It might be better to have a GridPlacement resource which is used by Tilemaps to position their tiles.

Continuing that line of thought, it might be possible to assign a GridPlacement resource to a Node2D/Spatial, and have it snap itself automatically. Not sure how that would work out, but it might be worth discussing.

@willnationsdev
Copy link
Contributor

willnationsdev commented Nov 27, 2018

Edit:

TL;DR; I think we should go with the GridPlacement resource, make it available on Node2D/Spatial nodes as you suggested, but only make it dictate movement while in-editor, and ideally only when positioning/moving the node from the viewport (not during animations/physics movement, even in tools scripts - though idk if that's possible). With this, a simple Position/Position2D node should meet all of our needs since it can be edited independently of the TileMap/GridMap and then also enforce grid positioning rules that are shared by the TileMap/GridMap. We'd probably also need to add multi-select cell movement in the TileMap.


Well, if it is part of the TileMap, it will be easy to delete/move, but probably it would be impossible to select it and change just certain properties.

Well, we would be able to change properties, but those properties would have to be exposed in the TileMap's/TileSet's _get_property_list, _get, and _set methods, just like the TileSet::TileData and TileMap's selected cell do. This makes things much messier though, as we begin to re-implement the existing editor features (scene dock, inspector dock, node dock for signals/groups) all through the EditorInspector alone just to give full access to the nodes that are managed by the TileMap. I'd prefer to avoid this if at all possible and concentrate on solutions that reinforce people relying on the entire editor's existing tools to access and modify the nodes.

So, people shouldn't need to go through the TileMap to get to the nodes. Instead, it would be better to make the TileMap and created nodes rely on a shared third party that directs their positioning so that your latter two problems can be solved ("And if you want to move a part of the level slightly... you have to move all the respective boxes/flowers/pickups along." / "As an additional bugger, you have to remember to turn on "use snap" and then "configure snap" to use the proper tile size. That, or calculate the positions of the various nodes by hand.").

Note the wording: part of the level. Usually, I keep my whole level as one tilemap (simplifying code that way), so if I move the tilemap, I would have moved my whole level.

I thought the whole purpose of the scene system was to be able to build your level out in pieces though? Isn't this more of a usage problem rather than an API problem? You could simply create that portion of the level in its own scene and then instance it, problem solved.

I see now. You can't do that ^ while at the same time having only one TileMap. You'd end up with a hierarchy of TileMap sub-scenes. But I still think it feels closer to the intended Godot workflow than your suggestion. At least in that situation, you can fully design each piece independently. The thing that wouldn't work that'd be nice to fix is how you can't just "Create Scene from Branch" or "Merge from Scene" when you are dealing with TileMaps and the levels created by them. Nodes' fluidity between scenes is one of Godot's biggest advantages over other engines, but the TileMap really prevents you from doing that while at the same time giving you all of the nice, convenience "painting tile" features one looks for in a 2D tile game editor. Resolving that disparity seems to be the root issue here.

Remembering snapping, and probably keeping it when creating new scenes would be cool, indeed.

Yeah, having re-usable snapping rules as well as a way to specify custom rules for relative positioning seem to be what's needed here. TileMap could then define those custom rules and make external nodes follow its repositioning of selected tiles or something.

Deferring snapping to the tilemap is somewhat fishier as an idea, since it either makes canvas_item_editor depend on tilemap or node2d know about snapping.

Ah, good point. XD No need for them to need special knowledge of TileMap.

I think no. It might be better to have a GridPlacement resource which is used by Tilemaps to position their tiles....Continuing that line of thought, it might be possible to assign a GridPlacement resource to a Node2D/Spatial, and have it snap itself automatically.

The Node2D/Spatial thing might be cool, if it defaulted to inherit its parent's grid placement (similar to the pause functionality). On the other hand though, we often want animated or physical movement to move fluidly (or even discretely) across sub-points between grid positions, so locking a node's entire movement to only grid positions would not grant us our desired behavior. This could be great though if the GridPlacement resource only affected a node's positioning when placing it in the scene (so, not affecting other movement in-game).

In summary, I think...

  1. We want to expose a transform that can be placed/moved by the TileMap independently of the tiles themselves (so if you erase and paint a new tile, you won't delete the exposed transform, but can instead just move it).
  2. We want to be able to instance a scene positioned relative to this transform.
  3. We want the instanced scenes' contents to automatically position themselves according to the grid that the exposed transform is locked to.

If the above sounds correct, then, in combination with an editor-only child-snapping GridPlacement resource feature on Node2D/Spatial nodes, one could conceivably implement all of those features with the existing Position/Position2D nodes.

  1. They have a dedicated UI that displays them as a point.
  2. They aren't rendered in-game.
  3. They can be placed as a child of a TileMap/GridMap, but also can serve as the root of their own scenes (so one can design grid-positioned content outside of the TileMap's context).
  4. They aren't affected by a TileMap's painting operations (so they can't be accidentally deleted, nor would they require custom UI code in the TileMap editor to move them around or visualize them).
  5. All of their child nodes can be directly accessed through the existing editor tools with no additional work.

We would need to couple this with the ability to select and move a group of cells in the TileMap though. That's not currently a feature, but it would be needed if we wanted to move stuff in two steps: 1) move selected environment tiles to translate sub-section of TileMap, 2) move Position2D so that non-environment objects bound to that sub-section can also be translated.

@willnationsdev
Copy link
Contributor

@Jummit Also, to satisfy your perception (which I think is reasonable), we might try looking into a "node/script/scene painting" feature for the canvas_item_editor, some sort of insert mode that would let you define a palette of classes (#22181 could help there) and paint/erase with left/right mouse click just like a TileMap.

@jabcross
Copy link
Contributor

jabcross commented Jan 28, 2019

Some people have solved this problem by having some tiles which get replaced in _ready() with their real versions. This makes a slight performance hit on load times as well as being too much boilerplate. I think having it in core is going to ease things for a lot of people.

I personally don't mind the instancing lag, I'm usually just interested in the painting facilities. However, when you have dozens of different types of tiles, it gets cumbersome to create a different placeholder tile for each one.

About the difficulty with selection: we could have a image-editor-like toolbar for single tile selection, rectangle selection, lasso, and paint-and-erase. Those could automatically select all of the corresponding nodes in the tree. That way, if we want to move a bunch of tiles at once, we could just draw a box or ctrl-click them and then just drag them around.

Having a pipette tool would also be welcome.

@bojidar-bg
Copy link
Contributor Author

@jabcross Check the "Tilemap" menu, I think most features you are mentioning are already present there.

@Zylann
Copy link
Contributor

Zylann commented Jan 29, 2019

@jabcross for pipette, use Ctrl+Click on a tile ;)

@jabcross
Copy link
Contributor

jabcross commented Feb 4, 2019

@jabcross Check the "Tilemap" menu, I think most features you are mentioning are already present there.

So it does. Recognizing that was a menu was a bit hard, tho. Those tools are pretty hidden. Thanks

@KoBeWi
Copy link
Member

KoBeWi commented May 25, 2020

We are moving proposals to the proposal repo, so this has to be closed.

I looked briefly on the discussion here and this should be covered by my new TileMap proposal: godotengine/godot-proposals#896

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants