-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Expose TileMapLayer #89179
Expose TileMapLayer #89179
Conversation
Is this intended that layer dropdown is available when a TileMapLayer is selected? EDIT: |
I guess. I think it's something you mentioned you preferred to keep to avoid inspector updates. But it's true that it might be a bit weird, and you can use the parent TileMapLayerGroup node instead if you need fast updates. So well, I can remove it, I don't mind either way.
Edit: ah. Interesting. I guess we should disable the editor in that case. |
Yeah when you have a specific TileMapLayer selected, being able to switch to other layers might be confusing. That option should be available only for TileMap and TileMapLayerGroup. |
Another thing, which can be done in a follow-up and I could add it myself, is being able to convert TileMaps to TileMapLayerGroups. While I don't care that much about layer nodes, being able to hide them is useful and more convenient/efficient than disabling. Of course there is an option to add layer visibility to TileMap, but I think we shouldn't add more functionality to it anymore.
MultiNodeEdit solves that. |
Yes, that's definitely something we should add as soon as possible I agree. But it should be fairly straightforward to implement, so I didn't bother for now.
Oh yeah, I know it's possible, it's just a bit more cumbersome to edit I believe. But yeah, I don't know how users would feel about it. I feel like we could maybe remove TileMapLayerGroup completely in the end 🤔, not sure if it would help much though. |
btw I checked how slow is switching between layer nodes and it's not bad actually, even in dev build. So I guess inspector updates aren't really a problem (other than they exist and waste your CPU xd). I think TileMapLayerGroup still makes sense because layer switching is convenient and the dropdown doesn't make much sense in layers themselves (though maybe a shortcut for previous/next layer could work and select them in scene, idk). Still, it's a bit awkward that it has literally one property, I wonder if more settings could be delegated to the group and layers would set "overrides". |
The thing is that, I believe, we could easily keep all current behavior we have. Like, we could keep the shortcuts, the highliting and all (the only difference is that the highlighting will require the editor to store those values, instead of the parent node). That should be doable without this TileMapLayerGroup node. So yeah, that's kind of the whole question. Either we try to add more properties in the TileMapLayerGroup node (and have per-layer overrides), but it might end up more complicated to maintain and is likely more complex to understand. Or we ask a bit more work from users (copy-pasting the TileSet, editing multiple nodes at once for some properties, etc...) but it makes the whole system a lot simpler. Maybe we should do a poll or something ? |
Might be a good idea. I think having a single node that configures all layers is useful. But it should only have properties that normally you'd set to the same value for all layers. |
There's some bug with layer TileSet. If you set it to any value, even the same as parent group, selecting the layer is like 10x slower. Also happens when layer isn't under a group. EDIT: |
Another bug with TileMapLayer: stand-alone layers load without a TileSet. |
7e3468c
to
9062485
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this looks good, will take a proper look at some of the changed logic tomorrow but seems proper and safe
c66aecb
to
50979c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In its own docs, whenever TileMapLayer itself is mentioned exactly by name, it should be surrounded by square brackets ([TileMapLayer]
) for a self-reference (can't take a deeper look at the moment).
Alternatively, in my opinion, calling it "layer" instead of the whole class name would be sufficient and less "verbose".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was already very peeved at the TileMap/TileSet documentation in the past. But I can chime for a few things here, just to make TileMapLayer slightly more appealing.
doc/classes/TileMapLayer.xml
Outdated
<method name="get_cell_alternative_tile" qualifiers="const"> | ||
<return type="int" /> | ||
<param index="0" name="coords" type="Vector2i" /> | ||
<param index="1" name="use_proxies" type="bool" default="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of how you feel about it, another huge problem is proxies have never been properly documented in the past. Lack of documentation and no direct, "tangible" consequence when the argument is true
, and they may as well be regarded as "useless" . I had to skim through blog posts to figure out how they worked (and I still could not manage to have them working as intended either).
a9d16a6
to
d40c3c1
Compare
Alright, I've just pushed a change that does two thing:
|
5b36e3d
to
a6dae20
Compare
doc/classes/TileMapLayer.xml
Outdated
<signal name="changed"> | ||
<description> | ||
Emitted when this [TileMapLayer]'s properties changes. This includes modified cells, properties, or changes made to its assigned [TileSet]. | ||
This signal might be emitted very often when batch modifying a TileMap. Avoid executing complex processing in a connected function, and consider delaying it to the end of the frame instead (i.e. calling [method Object.call_deferred]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as I implied before, I'm concerned about this behavior, at least at project run-time. It makes generating TileMaps at runtime noticeably slower, as well as additional dynamic updates. It does not help that there's no way to know what changed in the tile map exactly.
May be worth reconsidering this separately later. What can be done in these situations is calling Object.set_block_signals
before massive TileMap changes.
This signal might be emitted very often when batch modifying a TileMap. Avoid executing complex processing in a connected function, and consider delaying it to the end of the frame instead (i.e. calling [method Object.call_deferred]). | |
[b]Note:[/b] This signal may be emitted very often when batch-modifying a [TileMapLayer]. Avoid executing complex processing in a connected function, and consider delaying it to the end of the frame instead (i.e. calling [method Object.call_deferred]) or preventing this signal from being emitted with [method Object.set_block_signals]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as I implied before, I'm concerned about this behavior, at least at project run-time. It makes generating TileMaps at runtime noticeably slower, as well as additional dynamic updates. It does not help that there's no way to know what changed in the tile map exactly.
What do you mean it makes things noticeably slower ? Have you tried it ? If you don't connect to it, the cost of emitting a signal is close to zero if I am not mistaking. It was already like that with TileMap and I don't think it was ever a bottleneck.
a6dae20
to
d4cc172
Compare
"Deprecating" means keeping the functionality working, but notifying that it might be removed. Keeping the API but without its effect is "obsoleting", and in that case it would be more useful to outright break compatibility than to let the API work but projects break at runtime due to silent removal of the functionality. So I would add back the code to make it correctly deprecated. |
d4cc172
to
96b4654
Compare
Done. I also re-added the documentation accordingly. I didn't bother mentioning the argument is deprecated, as the whole TileMap node is marked as deprecated anyway. |
96b4654
to
3cd4b28
Compare
Thanks! |
Is there any plan to match all the functionality we used to have on For me, this is the main reason to have an official we're missing all the methods that used to aggregate/merge cell usage across layers, like
and the simple helpers like
Doing "simple" actions like copying a map from one Yes, one could write a class of Node2D that does all of this, but why re-invent the wheel when |
Not yet. But I guess we could have a "group" node if there's a huge need for it. That might require a proposal though.
That's a fair point. combining those manually should not be too hard in general, but I agree it's a bit tedious.
You can just get the node's z-index now (like `$TileMapLayer.z_index), not sure how it would help.
You can just use a parent node and use |
Thanks for the response! I guess I'm just surprised that the proposal to remove What's the Godot procedure for getting these approved? Just PR-based, or is there a larger community discussion? I'll make sure I'm more up-to-date 😄 |
You can create a proposal in the godot-proposal repository, and see if there's traction for it. |
Alright. This PR finally exposes the TileMapLayer node.
It does multiple things:
All editor and tile map feature should more or less work now.
Edit: a video (a bit outdated though, I fixed most issues in this PR comments):
2024-03-27.14-34-26.mp4
Old info
I am still wondering about making some last moment changes though:
In general, I think this new node is quite an improvement. But my main concern is that it makes setting up layers that would have similar properties a bit more cumbersome. Things like "show debug collision" has to be set up per layer, which is a bit more complicated I believe.
A video:
Peek.05-03-2024.14-15.mp4