JoshLee0915 / GodotLDtkImporter

A plugin for importing LDtk files into Godot
MIT License
26 stars 4 forks source link

Add explanations and custom image location #2

Closed deranjer closed 3 years ago

deranjer commented 3 years ago

Could not figure out exactly how you had your images loading since you are somehow using the path specified in the LDtk file, so wrote a UI component to allow user to specify a path. Also added some explanatory text, so let me know how that looks.

JoshLee0915 commented 3 years ago

To answer your question quick, as specified in the LDtk docs the image path for each tileset is stored as a relative path relative to the LDtk file. To get an absolute path for the images we simply just append the relative path from the LDtk file to the absolute path of the LDtk minus the file name.

I do have some concerns with this PR though. While it could be useful there are a few issues I could see that could be problematic.

1st, if this allows a user to specify the root directory for images anywhere on their system there could be issues when building distributions as this resources would not be in the res:// directory and may not be packed or copied over correctly.

2nd, if the directories you can choose is restricted to the res:// directories then the system is redundant as the importer restricts loading from the res:// directory already.

3rd, your changes do not search the directories recursively and always expects the images to be in the directory specified. This is quite rigid and limits how users can choose to organize that directory since they can not have sub directories.

JoshLee0915 commented 3 years ago

I should state the PR is getting me thinking about how the system may react if the relative path points to outside the res:// directory. I will need to test that as I kind of assume it will just crash but not sure.

deranjer commented 3 years ago

as specified in the LDtk docs the image path for each tileset is stored as a relative path relative to the LDtk file.

Is this some sort of setting you need to set somewhere in LDtk? It does not at ALL store things this way for me. If we look at my .lktd file I get this:

"tilesets": [
    {
        "identifier": "Full_tileset",
        "uid": 1,
        "relPath": "E:/TacticalRPG_Godot/Tiles/full_tileset.png",
        "pxWid": 256,
        "pxHei": 256,
        "tileGridSize": 32,
        "spacing": 0,
        "padding": 0,
        "opaqueTiles": [0,1,2,3,11,12,19,20,24,25,32,33],
        "savedSelections": [{ "ids": [24,32], "mode": "Stamp" }]
    }

That is certainly not a relative path, and when I save from LDTK it only saves the LDtk file it does not pull the image file and store it anywhere else in a relative path that I can see.

I built the select images folder selection since I literally was unable to use or test the plugin without that feature unless I manually modified the .ldtk file.

JoshLee0915 commented 3 years ago

Interesting, mine are all relative. Now that you are mentioning it there may be a setting for absolute paths. Let me double check, otherwise if thats the main issue it may be better to check if the path it absolute or relative then adjust accordingly

JoshLee0915 commented 3 years ago

Oh may I ask what version of LDtk are you using? 0.5.2?

deranjer commented 3 years ago

@JoshLee0915 I am using 0.5.2-beta. I'll try and dig around in the settings as well.

deranjer commented 3 years ago

@JoshLee0915 There is no setting. If you select an image that is stored at a location that is ALREADY relative the .ldtk save location it makes it a relative path. If not, it makes it absolute. Might have to take that up with deepknight to figure out a way to make it consistent or we will need to do some parsing of the path to figure it out.

JoshLee0915 commented 3 years ago

Ah, I see. You are trying to load an image from a different drive so it can not do it relatively, it has to use an absolute path. I have also confirmed this on my side.

I feel its not the standard use case but I can see it happening like how it happened with you.

With this info I think a absolute path check will need to be added. Also, it may be a good idea to add an option to copy the assets over into the res:// directory on import.

I will open two new issues for this, but I will close this PR only because I do not think this is the way we want to handle that issue.

deranjer commented 3 years ago

Sounds good to me. I did also try a directory above the ldtk and realized it referenced it with ..\ so only a different drive would trigger this. This does make it a pretty far edge case but certainly is something that needs to be checked for.

JoshLee0915 commented 3 years ago

I have created issues #9 & #10. I plan on trying to do some more work on this tonight and #9 should be fairly simple (though I need to make sure I do it in a platform independent way) so I may have that knocked out tonight. If you want to try your hand at it feel welcome to.

Thank you for bringing this to my attention!