feat: add support for LDtk tilemap #4575

daiyam · 2022-11-23T19:04:36Z

This PR adds support for LDtk tilemap.

Caveats:

floating tiles' position are rounded to the lower integer.
it doesn't support levels with layers of different grid sizes, it will pick the first one.

Todo:

~~resources loading or importing, to be validated what to do.~~
~~support .ldtk files ~~(only .json for now)~~ (* there is any issue with existing tilemap)~~

closes #2434

4ian · 2022-11-25T08:48:05Z

Looks great! I'm reviewing this today :)

4ian

Just did a quick review but so far only a few minor comments.

SharedLibs/TileMapHelper/src/render/Manager.ts

SharedLibs/TileMapHelper/src/render/PixiHelper.ts

SharedLibs/TileMapHelper/src/render/ldtk/PixiHelper.ts

daiyam · 2022-11-25T15:02:37Z

@4ian From my todo, let's start with the resources loading.

When adding a LDtk map, here what I would like to do:

parse the map
locate all the files
import all those files to the project as resources
rewrite the map/json with the corresponding resource ids

What do you think about it?

The issue with that approach is external map (not copied into the project), we can't really refactor them to our need.

4ian · 2022-11-26T10:39:02Z

When adding a LDtk map, here what I would like to do:

That's a way to do it yes, parse the map and import as resources the other files... but as you said this implies to also update the map..

I wonder if there is something we could do that would work as a "mapping", stored in the object, that using as:

As key: the name of the file/resource used in the LDtk JSON
As a value: the name of the resource in GDevelop.

So when you import a LDtk file in the editor:

The LDtk map is parsed and every file referenced inside is imported as a new resource (unless it already exists, not sure how exactly it would work)
We keep inside the object a mapping of "name of the file in LDtk -> name of the resource".

This way, the LDtk map does not need to be modified. This will allow this approach to work for example with LDtk maps stored in URLs for the web-app.

Do you think this could work?

daiyam · 2022-11-26T11:07:58Z

Yes, I like that.
Will it be ok to add that mapping/translation layer in the metadata field of the resource? Since we can have several objects referencing the same map but using different a level.

4ian · 2022-11-26T16:22:52Z

Will it be ok to add that mapping/translation layer in the metadata field of the resource? Since we can have several objects referencing the same map but using different a level.

Sounds like an excellent idea (and if it does not work it can always go back in the object). Do this into a field in the metadata (so like the metadata looks like: { ldtkRelPathsMapping: {...} }, so other metadata can live next to it).

This will probably need some special work when the resource is imported, you can set it up at some place and we can then ensure it's applied everywhere resources can be imported in the editor.

SharedLibs/TileMapHelper/src/model/Model.ts

SharedLibs/TileMapHelper/src/render/TileTextureCache.ts

D8H · 2022-11-28T08:43:44Z

Thanks for working on this, it will make a lot of people happy 😃

daiyam · 2022-11-28T15:27:12Z

@4ian In the end, I've named metadata as { embeddedResourcesMapping: ... } since it's a generic structure and can be used by other resources.
So now, it can load LDtk maps that are inside or outside the project.

I have a issue with Remove unused resources, it would remove the embedded resources. I see exposeImage or exposeResources but I don't know if it's correct way to declare them as used.

daiyam · 2022-11-28T16:29:24Z

To add support for .ldtk file, I've added the new tilemap resource.
I'm having issue with old tilemaps which are using json resource. The tilemaps are displayed but can't be reused (if you remove the property from the object and re-add it, you get an error!).
Should they be transformed as tilemap resource?

4ian · 2022-11-28T21:46:08Z

I have a issue with Remove unused resources, it would remove the embedded resources. I see exposeImage or exposeResources but I don't know if it's correct way to declare them as used.

Indeed, we need to rework the way we iterate on the project resources so that resources can themselves notify that they are using other resources. Project::ExposeResources calls worker.ExposeResources(&GetResourcesManager()) which calls void ArbitraryResourceWorker::ExposeResources(gd::ResourcesManager* resourcesManager) which iterates on resources:

That's where we should call ExposeImage/Audio/Font/Json/Video/BitmapFont according to the content of embeddedResourcesMapping . This means we'll likely need to know in the mapping the type of the resources being pointed... or not? Because we'll get the resource name, so we can just check the resource type by looking at the resource itself.

So that should work (not tested, just wrote it to check if it made sense):

Not very pretty too, so we could maybe improve this (have a "ExposePointedResources" in the gd::Resource class, maybe making this mapping a first class field in the C++ class, with a nice getter/setter... but for now that will do the trick!)

The tilemaps are displayed but can't be reused (if you remove the property from the object and re-add it, you get an error!).

Which error is it? This sounds weird.

Should they be transformed as tilemap resource?

I think we could keep both but I've not thought a lot about it. Do you think we could keep the JSONs for now and have this in another PR?

daiyam · 2022-11-29T10:46:47Z

void ArbitraryResourceWorker::ExposeResources(gd::ResourcesManager* resourcesManager)

I've tried your modifs. Here my results:

the tileset/image is not removed when the tilemap is used.
the tileset/image is not removed when the tilemap is not used and still present in the resources.
the tileset/image is removed when the tilemap is not present in the resources.

I will search why...

Which error is it? This sounds weird.

It's an UI error:

I think we could keep both but I've not thought a lot about it. Do you think we could keep the JSONs for now and have this in another PR?

I can make the changes but:

.ldtk won't be able to be imported (.json will still be available)
the listTileMapEmbeddedFiles function, listing all the embedded tilesets, will have to moved on the json resource. It will be an extra process for those resources.

- Adapt property groups to show that the tilemap is all that is needed for LDtk - Add missing parameter after merge - Robustify instance renderer code against malformed metadata in a resource - Remove useless parameter in ExposeEmbeddeds - Code format JsExtension.js

…other resources + add test for it

Core/GDCore/IDE/Project/ArbitraryResourceWorker.cpp

4ian

I have to make a last pass on the IDE changes (newIDE/app/src/ResourcesList/EmbeddedResourceSources.js...) and we should be good.

Tested and it works well. Looks like magic to add a ldtk file and get the atlas image imported :) Also fixed renaming the resources.

I wonder if at some point we should make the "embedded resource mapping" a first class concept. For now we can keep it in metadata.

Silver-Streak · 2022-12-22T00:24:45Z

I wonder if at some point we should make the "embedded resource mapping" a first class concept. For now we can keep it in metadata.

If I understand correctly, this may be valuable later on if a Music Tracker is ever added, since I believe it can need/embed instrument audio files. Nothing crucial right now, but speaks to it being valuable long term.

4ian · 2022-12-22T09:48:51Z

Extensions/TileMap/JsExtension.js

@@ -364,10 +364,11 @@ const defineTileMap = function (
    .setFunctionName('getLayerIndex');

  object


Nicely done 👍

- Use a different data types to represent different part of the process of importing embedded resources - Remove useless properties - Avoid some branching - Rename functions and some properties to clarify the data being handled

4ian · 2022-12-22T17:45:57Z

Alright I did a few other changes and merged this! Thanks a lot for all the work done here @daiyam! 🎉 Awesome to see LDtk maps now direclty supported by GDevelop tilemaps. This will be appreciated by a large number of users and will make their life way easier :)
We'll need to update the documentation on the wiki and maybe then work on collisions or other polishing, but overall it's working 👍

daiyam · 2022-12-22T18:52:29Z

@4ian @D8H How do you want to proceed to support the .ldtk files?

4ian · 2022-12-22T21:12:36Z

@daiyam Was there any particular blocker for this? Feel free to go through the JsExtension.js file, setting up tilemap/tilemapResource instead of json/jsonResource when applicable and let's see how it's going?

daiyam · 2022-12-22T22:05:42Z

The issue is that when editing existing tilemaps, the existing resources are of type json instead of tilemap and so they can't be reused.
I think we should detect this case and convert them as tilemap resources.

Silver-Streak · 2022-12-23T05:21:25Z

@4ian @daiyam Just to confirm, is this something we think can be resolved easily?

I'd want to shore up as much of the support/functionality as possible before I put my approval on the bounty.

daiyam · 2022-12-23T08:20:39Z

@Silver-Streak I think it is doable but the decision on how to handle it should be made by the main developers.

4ian · 2022-12-23T09:32:31Z

Just to confirm, is this something we think can be resolved easily?

What is "this" exactly, to be sure of the scope?
We'll add support for the ldtk extension in another PR: #4733 and probably .tmj too at least.

the existing resources are of type json instead of tilemap and so they can't be reused.

For a few versions, we'll use a "fallback" allowing to display json in the selector of tilemap/tileset resources. Let's check on the other PR that this is working.

Silver-Streak · 2022-12-23T09:50:57Z

What is "this" exactly, to be sure of the scope?

Sorry, speaking specifically to the item daiyam mentioned directly above mine, as far as them being detected as json rather than tilemap resources.

daiyam · 2022-12-23T17:29:38Z

~~@4ian @D8H It seems there is a regression 😭 I'm not able to set the optional tileset for a tiled tilemap. I'm searching...~~

Edit: bad testing from my part... the json was already in the resources as a tilemap resource...

daiyam added 10 commits November 21, 2022 08:53

refactor: rename TiledMap as TiledTileMap and add LDtkTileMap

ab4e81d

feat: add level index UI

f6bc811

feat: load sample1

68222a9

feat: support stack tiles

39c170a

feat: select level

6169e69

test: pass all tests

eda42b3

refactor: regroup files by action

8a2dc67

style: reformat source files

b4d4177

fix: collision map & test

39daf85

Merge branch 'master' into feat-ldtk

7807eac

daiyam requested a review from 4ian as a code owner November 23, 2022 19:04

4ian reviewed Nov 25, 2022

View reviewed changes

refactor: improve variable names and comments

891fd84

daiyam added 2 commits November 27, 2022 09:13

feat: add tilemap resource

ffb1f93

feat: add background

8cba0c0

D8H reviewed Nov 28, 2022

View reviewed changes

feat: add embedded resources mapping

15231d9

daiyam added 3 commits November 28, 2022 16:59

Merge branch 'master' into feat-ldtk

9f4edbc

fix: improve background and alpha

15b4bd9

fix: correctly updating alpha

ed4cdc4

4ian added 3 commits December 22, 2022 00:08

Fix renaming a resource not renaming the mapping to this resource in …

69bb489

…other resources + add test for it

Remove leftover debug code

bea66c8

daiyam commented Dec 21, 2022

View reviewed changes

Core/GDCore/IDE/Project/ArbitraryResourceWorker.cpp Show resolved Hide resolved

4ian approved these changes Dec 22, 2022

View reviewed changes

4ian and others added 6 commits December 22, 2022 10:00

Fix flow

d65f67c

Revert some files renaming.

7ef3ac2

Rebuild the library

0291e59

Move back some files.

57d7ca5

Rebuild the library.

04b10c8

Scope the LevelIndex condition.

0f77574

4ian reviewed Dec 22, 2022

View reviewed changes

D8H and others added 3 commits December 22, 2022 10:55

Fix type

4f2bf21

Fix useless parameter

cb2349d

Simplify various data types

ca3042a

- Use a different data types to represent different part of the process of importing embedded resources - Remove useless properties - Avoid some branching - Rename functions and some properties to clarify the data being handled

4ian merged commit 77320ce into 4ian:master Dec 22, 2022

daiyam deleted the feat-ldtk branch December 23, 2022 07:31

daiyam mentioned this pull request Dec 23, 2022

feat: enable .ldtk files #4733

Merged

daiyam mentioned this pull request Dec 26, 2022

Can't Find Level Index Action on Tilemap #4745

Open

D8H mentioned this pull request Feb 10, 2023

Fix tile map loading for collision mask objects #4916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for LDtk tilemap #4575

feat: add support for LDtk tilemap #4575

daiyam commented Nov 23, 2022 •

edited

Loading

4ian commented Nov 25, 2022

4ian left a comment

daiyam commented Nov 25, 2022

4ian commented Nov 26, 2022

daiyam commented Nov 26, 2022

4ian commented Nov 26, 2022

D8H commented Nov 28, 2022 •

edited

Loading

daiyam commented Nov 28, 2022 •

edited

Loading

daiyam commented Nov 28, 2022

4ian commented Nov 28, 2022 •

edited

Loading

daiyam commented Nov 29, 2022

4ian left a comment

Silver-Streak commented Dec 22, 2022

4ian Dec 22, 2022

4ian commented Dec 22, 2022

daiyam commented Dec 22, 2022

4ian commented Dec 22, 2022

daiyam commented Dec 22, 2022

Silver-Streak commented Dec 23, 2022 •

edited

Loading

daiyam commented Dec 23, 2022

4ian commented Dec 23, 2022

Silver-Streak commented Dec 23, 2022

daiyam commented Dec 23, 2022 •

edited

Loading

		@@ -364,10 +364,11 @@ const defineTileMap = function (
		.setFunctionName('getLayerIndex');

		object

feat: add support for LDtk tilemap #4575

feat: add support for LDtk tilemap #4575

Conversation

daiyam commented Nov 23, 2022 • edited Loading

4ian commented Nov 25, 2022

4ian left a comment

Choose a reason for hiding this comment

daiyam commented Nov 25, 2022

4ian commented Nov 26, 2022

daiyam commented Nov 26, 2022

4ian commented Nov 26, 2022

D8H commented Nov 28, 2022 • edited Loading

daiyam commented Nov 28, 2022 • edited Loading

daiyam commented Nov 28, 2022

4ian commented Nov 28, 2022 • edited Loading

daiyam commented Nov 29, 2022

4ian left a comment

Choose a reason for hiding this comment

Silver-Streak commented Dec 22, 2022

4ian Dec 22, 2022

Choose a reason for hiding this comment

4ian commented Dec 22, 2022

daiyam commented Dec 22, 2022

4ian commented Dec 22, 2022

daiyam commented Dec 22, 2022

Silver-Streak commented Dec 23, 2022 • edited Loading

daiyam commented Dec 23, 2022

4ian commented Dec 23, 2022

Silver-Streak commented Dec 23, 2022

daiyam commented Dec 23, 2022 • edited Loading

daiyam commented Nov 23, 2022 •

edited

Loading

D8H commented Nov 28, 2022 •

edited

Loading

daiyam commented Nov 28, 2022 •

edited

Loading

4ian commented Nov 28, 2022 •

edited

Loading

Silver-Streak commented Dec 23, 2022 •

edited

Loading

daiyam commented Dec 23, 2022 •

edited

Loading