Added object support to Godot exporter

[Skrapion]

Now supporting objects in the Godot 4 exporter initially added in pull #3550.

Video showing it off here: https://www.youtube.com/watch?v=3UfOOdepDDw

[bjorn]

Looks great, thanks for continuing work on this plugin!

I've left a number of comments.

[bjorn]

Since the object ID should always be unique for a given map, would it make sense to use it instead of an incremental uniqueifier?

[Skrapion]

The ID should be unique per res path, not per object. I've updated the comments to reflect that.

[bjorn]

Ah, ok. But in that case, this whole thing seems to be only a bit complicated due to the use of the base name derived from the path. Couldn't we just give these ext_resource entries unique IDs like "object_resource_1", "object_resource_2", etc.? Or is the current mechanism preferred for its human readability of the exported file?

I toyed a bit in Godot to see what it uses as IDs when using "Save Branch as Scene", and it appears to be quite random:

[ext_resource type="PackedScene" uid="uid://nbxmprynxcyd" path="res://collision_shape_2d.tscn" id="2_6ieet"]
[ext_resource type="PackedScene" uid="uid://cqy4y2ac8ub0n" path="res://collision_shape_2d_2.tscn" id="3_r05t1"]

[Skrapion]

I made this change in the last commit. I could take it or leave it. The pros:

• This reduces the line count from 14 to 8.
• This is more efficient.

The cons:

• The generated files are less human readable.
• It makes the files less git-friendly. For instance, given:

[node name="Object8" parent="." instance=ExtResource("object_resource_0")]
position = Vector2(320, 10)

[node name="Object9" parent="." instance=ExtResource("object_resource_1")]
position = Vector2(125, 29.5)

[node name="Object10" parent="." instance=ExtResource("object_resource_2")]
position = Vector2(172, 29.5)

If you delete the first object from the map, then every object afterwards will change, because object_resource_1 will be renamed to object_resource_0, etc.

With the old code, it would remove Object8 and leave it at that.

[bjorn]

I liked the simplification, but I think your concerns are valid and likely the previous approach will be preferred by some.

[Lexpeartha]

Also it might be useful to have custom properties to rename node added as object (which I really think should be default node's name by default).

[Lexpeartha]

@Skrapion @bjorn any news on this PR?

[Blucky87]

@bjorn is there any movement on this PR?

[bjorn]

@Lexpeartha @Blucky87 The original author hasn't responded to my feedback and I'm not sure if they are still interested in finishing this change. I could try to do it myself, but I am not very familiar with Godot.

If you need object support when using Tiled maps in Godot 4 now, I would suggest you give https://github.com/Kiamo2/YATI a try instead. If there is a reason to prefer Tiled's export plugin, please let me know!

[Skrapion]

Finally got back to this! Thanks for your review, and your patience, @bjorn.

[bjorn]

Ah, ok. But in that case, this whole thing seems to be only a bit complicated due to the use of the base name derived from the path. Couldn't we just give these ext_resource entries unique IDs like "object_resource_1", "object_resource_2", etc.? Or is the current mechanism preferred for its human readability of the exported file?

I toyed a bit in Godot to see what it uses as IDs when using "Save Branch as Scene", and it appears to be quite random:

[ext_resource type="PackedScene" uid="uid://nbxmprynxcyd" path="res://collision_shape_2d.tscn" id="2_6ieet"]
[ext_resource type="PackedScene" uid="uid://cqy4y2ac8ub0n" path="res://collision_shape_2d_2.tscn" id="3_r05t1"]

[bjorn]

@Skrapion I'm glad you've come back to life! I left some additional feedback. :-)

[Skrapion]

Okay, this should be good to go as soon as CI completes. But whether we use commit 6bde51c or dd6eb6d requires a judgement call. See the review

[bjorn]

@Skrapion Thanks a lot for finishing this nice addition!