JoshLee0915 / GodotLDtkImporter

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

Alternate approach: no incremental updates & more #18

Open keir opened 3 years ago

keir commented 3 years ago

I ended up forking the repo:

https://github.com/keir/GodotLDtkImporter

It's a bit of a different direction. I haven't added the README yet or merged in your changes. Maybe take a look and see what you think. I'm in the LDtk discord ("Keir") if you want to chat realtime.

JoshLee0915 commented 3 years ago

Interesting, I only had a brief moment to review your changes and while I do not disagree with removing some of the incremental updating the one area I know we can not is the tilesets. The main reason for this is we do not have the ability to define custom collision and occlusion shapes on the tileset in LDtk yet so those have to be done in engine.

I also think not just blowing away the entire scene might be a good call incase the end user wants to add to the scene but I do not know if there is a good enough use case there to justify not just blowing it away. I think I will take you up on your offer though to chat with you on this on the Discord as I would like to get more of your take on things.

keir commented 3 years ago

Regarding the use case for editing the level post-import in Godot: I don't think it's sustainable to support user edits of the scene. My suggestion to get around the collision issue is that the user should make their own scene for the level, then instance the auto-generated LDtk scene inside it. Custom collisions can be layered in on top; and they'll naturally update when the LDtk file is changed.

Looking forward to seeing you on the discord! In the interm, I also discovered that there is another project for LDtk Godot import, that the author must have worked on over the break like myself:

https://github.com/lrgilbert/godot-LDtk-import

This importer has a proper unit tests setup which our version does not, and arguably better split functions. I posit that it'd be best for the community if we could all work together and make the One True Importer.

JoshLee0915 commented 3 years ago

Ya, I saw that. Skimmed over their implementation a bit and its an interesting approach, and the unit tests are quite nice as well. I have been meaning to look at unit testing in godot but have not had time. I agree would be great to add some though. I also agree with your assessment that it may be better for us as a community to try to pool our skills to make one true importer.

I did not have time tonight but I do plan on trying to reach out to you tomorrow so we can chat in a bit more detail over discord.

levigilbert commented 3 years ago

Hey, just wanted to check in. I’m open to collaborating, I’ll try to get in touch via discord or email this evening. Also somewhat free this weekend to chat. I agree that having 3 LDtk importers might be a bit much lol.

On a side note: Unit testing is pretty easy to set up with the GUT addon. I haven’t stress tested it by any means, but haven’t had any issues.

Thanks for bringing me into the conversation. Looking forward to talking with you both more.

stefanmielke commented 3 years ago

Hey folks! Just getting in touch with everyone since I opened PRs for both libs :)

I do enjoy the other lib approach the most, and it was the one with the least amount of errors importing the sample files as well, so I ended up implementing a lot of things over there and not that much here.

Since I plan to use LDtk for games on Godot in the future, I'm curious on how you guys want to move forward (also open to talk somewhere else if needed).

JoshLee0915 commented 3 years ago

@keir, @lrgilbert, and I all talked at length about this. Doing a full reload from scratch is the easiest way to import the LDtk files and the most stable, but there are a few issues that prevent us from doing that fully at the moment. One of the largest issues and easiest to explain are the tilesets.

These have to be incrementally loaded as there is currently no way to represent collision shapes, occlusion shapes, and nav meshes in LDtk (at least when I last checked it out) so regenerating these resources would wipe out a large amount of work any time there is a change to the map.

What we are mainly doing right now is trying to set as much as we can to use LDtk as the source of truth and incrementally loading what needs to. We have some changes planned that will fix some of the issues you highlighted in your PR but I have not had the time to implement them yet.

stefanmielke commented 3 years ago

Do you have a board with them, besides the project? There aren't that many details there.

I also implemented collisions on the other lib using the same approach as the Unity lib (using IntGridCsv from 0.8.0), as well as importing custom objects.

My idea was to have everything set up on LDtk, to the point where I can just reimport and run the maps (my tests were with the large sample map, with the only customizations being to add my custom properties that indicate the class of the node in my project).

JoshLee0915 commented 3 years ago

No unfortunately, we have a small doc on the side but it is more or less a loose collection of notes on what we have discussed. I do need to open some issues for fixing some of the issues (mainly not clearing the tilemap for regen). It is just lack of time atm. You are welcome and I personally encourage opening any issues for improvements or bugs you find. If you can provide some test LDtk files with them as well that would greatly help us improve the importer.

I also considered using IntGrids at first as well, but outside of the fact IntGridCsv I do not think was in the specs when I started this importer, it does not work very well in the general case for the following reasons:

  1. It requires the end user to define all the collisions shapes, occlusion shapes, and nav meshes manually in LDtk which can be handled automatically by the engine
  2. If I remember, Godot does a fair amount of optimization of the generated collision/occlusion shapes and nav meshes from the tile set. Using an IntGrid layer would require us to have to manually optimize them ourselves or just implement them trivially which could cause issues on larger maps.
  3. Unless you do something like assigning a numeric value for every possible shape, IntGrids are going to generally be limited to a square shape. This would work fine in maybe 70-80% of cases but many tilemaps have items and tiles that are smaller then the grid and/or use an irregular shape and we can not represent that with IntGrids easily.

Using, custom objects is also feasible, and actually possible in the current importer, but if I remember last time I looked at LDtk you where limited to a defined set a primitive shapes (though that could have changed in more recent versions) and still has the same issue of why do the work when the engine can do it for you.

The idea to set everything up in LDtk is a good one, but, unlike tiled, to currently achieve that goal you would have to put restrictions on what can be created in LDtk to work around some of the features currently missing in LDtk. Once LDtk allows for us to define shapes on the tileset and allows for custom shapes doing that will become much more feasible without hurting the general case.

stefanmielke commented 3 years ago

Yeah, IntGridCsv was included on 0.8.0 (a fairly recent version), and what it does is just add a number based on each "type" in the layer you have. If you take a look at the current samples, the largest of them (the one with multiple levels as a 'world') it assigns "1" to walls and "2" to water, leaving "0" as everything else (including props from autotiles).

For the collisions, I optimized them by creating another layer with just the collisions (and only if the user sets a toggle), and then reduce the amount of collisions based on continuous collisions (basically 'lines' of collisions). You can import the sample using the other addon (https://github.com/lrgilbert/godot-LDtk-import) to see how I implemented there.

For shapes, I don't think we should even care about those, as they should be set in-engine inside the objects, so they can be represented by whatever in LDtk (even using the sprites from the game if you want, again take a look at the sample).

I'm not sure what you mean by 'tilemaps have items and tiles that are smaller then the grid', at least for items, you should still use custom objects for that (Entities in LDtk), not the tilemaps themselves.

For the doc, maybe just add the whole thing as an issue? Maybe that would give more context to some of these reasonings :)

levigilbert commented 3 years ago

My availability has been lacking so these side projects have been on hold for me.

With my importer I’d like to keep it pretty lean so other people can use it as a starting point. I’ll try to update more of the code to compliment the updates to LDtk.

I’ve also been kinda waiting to see if the collision and whatnot would be implemented with an LDtk update. Currently I’ve been just using godot tilemaps since I’ve found their implementation of bitmasks/autotiling to be far easier to use and they already have collision and pathfinding for tilemaps. Was also running into performance issues with LDtk when Working with multiple auto tiles.