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

Fix TileMap layer reverts and defaults #83888

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Oct 24, 2023

I noticed that in beta2 my TileMaps suddenly started writing navigation_enabled = true property in the scene. Which is pointless, because it's a default value™.
Then I noticed that this actually applies to all TileMap layer properties. They write their default value, but only if the layer is higher than 0...

I fixed this inconsistency and all layers will now skip defaults. Before:

[node name="TileMap" type="TileMap" parent="."]
tile_set = SubResource("TileSet_mr1hv")
format = 2
layer_0/name = "Test"
layer_0/z_index = 5
layer_0/tile_data = PackedInt32Array(262148, 0, 0)
layer_1/name = "Fest"
layer_1/enabled = true
layer_1/modulate = Color(1, 1, 1, 1)
layer_1/y_sort_enabled = false
layer_1/y_sort_origin = 0
layer_1/z_index = 1
layer_1/navigation_enabled = true
layer_1/tile_data = PackedInt32Array()
script = SubResource("GDScript_j14su")

After:

[node name="TileMap" type="TileMap" parent="."]
tile_set = SubResource("TileSet_mr1hv")
format = 2
layer_0/name = "Test"
layer_0/z_index = 5
layer_0/tile_data = PackedInt32Array(262148, 0, 0)
layer_1/name = "Fest"
layer_1/tile_data = PackedInt32Array()
script = SubResource("GDScript_j14su")

(tile_data is always saved, though it can be easily changed too. I just wanted to avoid comparing big arrays)

As a bonus, you can also revert the layer properties now (previously it only worked for the first layer):

godot.windows.editor.dev.x86_64_18ULSnpU73.mp4

@KoBeWi KoBeWi added this to the 4.2 milestone Oct 24, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner October 24, 2023 11:44
@KoBeWi KoBeWi requested a review from groud October 24, 2023 11:48
@groud
Copy link
Member

groud commented Oct 24, 2023

I am not sure about the implementation. It seems a bit too complex for what it does.
I remember reduz mentioning a _validate_property callback that can be used for that, maybe that would be the way to go ?

Otherwise I guess it could be done that way but I kind of dislike having to add 80 lines of code just to define some properties defaults.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 24, 2023

_validate_property() is not called for properties added with _get_property_list().
And even if it was, you can't use it to define defaults and reverts. The code would be mostly the same, with an extra method that duplicates component check.

Dynamic properties not being revertable is actually a bigger issue and I'm thinking that it can be handled with a helper class (it would at least allow writing much less code). But it's totally out of scope of this PR.

@akien-mga
Copy link
Member

@groud I think you discussed this further on chat, is this good to approve now?

@akien-mga akien-mga merged commit 491160c into godotengine:master Nov 8, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the remove_all_properties_instead_of_adding_one branch November 8, 2023 19:00
@KoBeWi KoBeWi mentioned this pull request Nov 8, 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.

3 participants