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

Add custom tile properties to Godot .tscn export format #3653

Merged
merged 13 commits into from Jun 9, 2023

Conversation

keharriso
Copy link
Contributor

This is my attempt at adding feature #3652.

Tile properties are exported as custom property layer 0, named "Properties".

Allows code like: get_custom_data("Properties").get(<Property>)

Feedback welcome.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks fine to me, but it would be good if @Skrapion would also have a look since he wrote this plugin.

Also, I personally can't really judge the Godot part since I don't know if there would be any other ways of associating custom data with tiles.

src/plugins/tscn/tscnplugin.cpp Outdated Show resolved Hide resolved
@keharriso
Copy link
Contributor Author

I implemented your feedback and added support for custom classes and enums.

@bjorn
Copy link
Member

bjorn commented Jun 7, 2023

Allows code like: get_custom_data("Properties").get(<Property>)

Just wondering, but this double call seems a bit superfluous. Could we also make get_custom_data(<Property>) work? Or would it be undesirable to add a layer for each property?

I had a look at Godot and I think the way this should work is to set up a Custom Data Layer for each individual property, rather than exporting all properties as a Variant. This allows nicely visualizing each property's value for all tiles in a tileset, for example a custom "Depth" float property on the tiles:

image

It is a little strange that the layer name doesn't show up here though, and instead it shows the layer ID:

image

But that looks to me like something that might eventually be improved in the Godot UI and not a reason to export the properties as nested in a Variant.

@bjorn bjorn self-assigned this Jun 7, 2023
@bjorn
Copy link
Member

bjorn commented Jun 7, 2023

Just for your information, I'm currently working on making the changes to achieve the above change in the export, but I don't know if I will be able to finish it today.

@keharriso
Copy link
Contributor Author

Sounds good 👍

Let me know if you want my assistance in any way.

Each different top-level property used on the tiles of a map's tilesets
will now get its own Custom Data Layer.
@bjorn
Copy link
Member

bjorn commented Jun 8, 2023

@keharriso I think I managed to get it to work. It would be great if you could review my change and do some testing as well. :-)

There are some todo's left in the code regarding support for QColor, FilePath and ObjectRef properties. It would be great to handle those in case they have any equivalent in Godot (QColor would certainly have, and FilePath could probably be written as string).

bjorn added 9 commits June 9, 2023 12:04
* Colors are written out like "Color(r, g, b, a)"

* Files are currently written out as strings and absolute paths. Would be
  good to change this to relative, assuming files within the Godot project
  are referenced.

* Object references are just written out as the object ID, though this
  isn't very useful at the moment since the exporter doesn't support
  exporting objects yet.
@bjorn bjorn merged commit c59d9f9 into mapeditor:master Jun 9, 2023
5 of 6 checks passed
@bjorn
Copy link
Member

bjorn commented Jun 9, 2023

@keharriso Thank you very much for getting this change started! I think it turned out quite well now so I've updated the news / docs and merged this for Tiled 1.10.2!

Since you've got some experience with this plugin and with Godot, maybe you could help get #3615 finished as well? @Skrapion has been silent since opening the PR so I'm not sure we can still expect feedback from them, but it's another important feature of the exporter that needs testing and polish.

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

Successfully merging this pull request may close these issues.

None yet

2 participants