search

mapeditor / tiled-to-godot-export

Tiled plugins for exporting Tilemaps and Tilesets to Godot 3 format
149 stars 35 forks source link

Revert breaking changes to projectRoot and automatically determine projectRoot #46

Closed eleniums closed 2 years ago

eleniums commented 2 years ago

I updated Tiled and this plugin to the latest versions and found that my pre-existing TileMaps and TileSets no longer exported successfully. After some digging through the code and PR history, I found that this PR broke backwards compatibility with the projectRoot custom property: https://github.com/mapeditor/tiled-to-godot-export/pull/40

The original intent of projectRoot was to define the Godot project root (the location of the project.godot file), which is defined in Godot as res://: https://docs.godotengine.org/en/stable/tutorials/io/data_paths.html

All resource paths are relative to the projectRoot and the exporter created them automatically if projectRoot was properly set. The above PR changed the meaning of projectRoot to directly define the relative path to a resource. I think if this functionality is still desired, it should be introduced as another optional custom property, something like resPath maybe.

In order to maintain backwards compatibility, I reverted the change to projectRoot. Any projects using the existing tagged release can now upgrade without breaking. As a bonus, I added code to automatically determine the project root, so long as the Tiled files are stored alongside their exports within the Godot project structure as is recommended. I think this will reduce the headache new users have when trying to adopt this plugin. I know projectRoot was the most confusing thing to me when I first attempted an export.

I think the referenced issues the PR was trying to address may have stemmed around a lack of documentation regarding the projectRoot custom property. I've tried to update the readme to be a bit more clear about what projectRoot does.

I don't want to diminish the effort put into https://github.com/mapeditor/tiled-to-godot-export/pull/40. I really like the naming changes. I think if the different res path functionality is still desired, we can absolutely add it in another PR with a different custom property (or I could even add it to this one).

mcarpenter622 commented 2 years ago

Hello I was concerned with that change being breaking but I could never find a use-case where the previous design worked.

The main point of my change was so that the tiled map and tileset did not have to be in to godot path. Your change is breaking that.

My use case. Godot-project root ->

External-project root ->

In the current version when project root is set to /maps the Tilemap.tscn has the correct paths created. In your version it still has the path to the external-project.

I do like what you did with finding if the tile map is in the godot path. Perhaps we can have a best of both worlds where if it is in the godot path it follows what you are expecting, otherwise it replaces the path as in the current version?

For my own testing can you please provide a directory structure and projectRoot value for your use case?

Thanks, Mark Carpenter

eleniums commented 2 years ago

Hey, glad we could start a conversation up. So the exporter was designed with the idea that all Tiled files and exported Godot files live side by side in the Godot project: https://github.com/mapeditor/tiled-to-godot-export/blob/master/README.md?plain=1#L86-L87

This is the use-case where projectRoot makes sense and where it can automatically determine the project root. So, using something similar to your example: Godot-project root -> (let's say this is C:/git/godot-project)

Before the breaking changes, this worked if you set the projectRoot to the Godot project root. So: projectRoot : C:/git/godot-project. When exported, the exporter would correctly set the resource path as res://maps/Tileset.tres.

This was nice because you just needed to copy the same project root around to any tilemaps and tilesets. You could also make it a relative project root like ./.. which would evaluate to C:/git/godot-project. With my changes, this functionality works again, but also the project root will just be determined automatically if possible.

This doesn't cover the scenario where the Tiled files are stored in an external location and I agree we can address that. What did you think about my suggestion of a different custom property, something like resPath or relativePath maybe? That way we can support both scenarios.

What do you think? I can even make the changes to pull your code in for the new custom property.

mcarpenter622 commented 2 years ago

I think that is a great solution that solves both our use-cases! I am definitely on-board with "relativePath" parameter for both the tmx and tsx export while reverting projectRoot back to its original design like you have. Would you be willing to add that relativePath as part of your PR here?

eleniums commented 2 years ago

I think that is a great solution that solves both our use-cases! I am definitely on-board with "relativePath" parameter for both the tmx and tsx export while reverting projectRoot back to its original design like you have. Would you be willing to add that relativePath as part of your PR here?

Great! Yes, I can go ahead and add the new relativePath custom property as part of this PR. I should have some time tonight to work on it.

eleniums commented 2 years ago

Okay, I added the new relativePath custom property. I gave it precedence to projectRoot if both are set, which I think makes sense since it's a new property and projectRoot was the old default.

So relativePath works like this: If the location of an exported tileset is: C:/project/maps/level1/tileset.tres Then the relativePath custom property would be: /maps/level1

And projectRoot is as before: If the location of an exported tileset is: C:/project/maps/level1/tileset.tres Then the projectRoot custom property would be: C:/project

I updated the readme a bit to hopefully make it a bit more clear how this works.

@mcarpenter622 How do these change look to you? I ran multiple tests, but maybe you could test the changes with your setup just in case something is missing?

mcarpenter622 commented 2 years ago

I'll check it out in the morning and let you know!

On Fri, Jul 15, 2022, 12:38 AM Stan Nelson @.***> wrote:

Okay, I added the new relativePath custom property. I gave it precedence to projectRoot if both are set, which I think makes sense since it's a new property and projectRoot was the old default.

So relativePath works like this: If the location of an exported tileset is: C:/project/maps/level1/tileset.tres Then the relativePath custom property would be: /maps/level1

And projectRoot is as before: If the location of an exported tileset is: C:/project/maps/level1/tileset.tres Then the projectRoot custom property would be: C:/project

I updated the readme a bit to hopefully make it a bit more clear how this works.

@mcarpenter622 https://github.com/mcarpenter622 How do these change look to you? I ran multiple tests, but maybe you could test the changes with your setup just in case something is missing?

— Reply to this email directly, view it on GitHub https://github.com/mapeditor/tiled-to-godot-export/pull/46#issuecomment-1185166026, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJ7PVU6EMFFXV6CMELXURTVUDTOFANCNFSM53J2G42A . You are receiving this because you were mentioned.Message ID: @.***>

nerochiaro commented 2 years ago

FWIW I tested the branch and it does what it says on the tin, but can't claim to have done extensive testing

mcarpenter622 commented 2 years ago

I ran this through my test and I think it works quite well! Great improvement!

eleniums commented 2 years ago

Great, thanks @mcarpenter622!

@bjorn @MikeMnD Any feedback or are we good to go with this PR?

MikeMnD commented 2 years ago

Hi, looks fine. I've read the discussion and I like the solution. So it's a go for me!

I especially like the updates to the README.md Good job both of you @eleniums @mcarpenter622 I'll leave it for Bjorn to push the Merge button ;)

eleniums commented 2 years ago

I think it's good to go. It's been tested by several people at this point, so I'm going to go ahead and merge.