Ldtk file support for tilemaps in Gdevelop

blurymind commented 2 years ago

This adds support for ldtk parsing in gdevelop.

It also makes an attempt at loading resources from relative paths to the tilemap file (auto - loading textures from paths in the tilemap file) - this is required since ldtk can have a different atlas for each layer/map and can have many maps and atlases in a single file

Todo

[ ] add tests
[ ] add some examples
[x] update to latest pixi-tilemap and implement layer opacity in parser
[x] add rotated tiles support for ldtk (rework the existing one to work on both
[x] add layer opacity for tiled files too
[x] register pixi tilemap as a renderer plugin in the runtime + fix other regressions
[ ] ~remove all arrow functions from the helper module. Note from 4ian: no need for this~
[ ] fix regressions from merging dev (changing map at runtime no longer works)
[ ] implement a better approach for handling loading of multiple resources found in the json file's relative path entries

closes https://github.com/4ian/GDevelop/issues/2434

blurymind commented 2 years ago

It looks like I need to get to pixi renderer that gdevelop uses in order to register this now pixi>core> Renderer.registerPlugin('tilemap', TileRenderer as any); https://github.com/pixijs/tilemap/issues/124#issuecomment-884005578

@4ian what would be the correct way to do that in gdevelop? Should this be done from the extension files?

blurymind commented 2 years ago

I think I can use this to get to the renderer in the runtime.Now how to do it in the ide

      const pixiRenderer = runtimeScene
        .getGame()
        .getRenderer()
        .getPIXIRenderer();

might have to add a getter

blurymind commented 2 years ago

InstancesEditor has this line

    this.pixiRenderer = PIXI.autoDetectRenderer(
      {
        width: this.props.width,
        height: this.props.height,
        preserveDrawingBuffer: true,
        antialias: false,
      }
      // Disable anti-aliasing(default) to avoid rendering issue (1px width line of extra pixels) when rendering pixel perfect tiled sprites.
    );

I need to somehow pass this.pixiRenderer to the extension so I can do what was suggested by pixi-tilemap devs renderer.plugins.tilemap = new TileRenderer(renderer);

EDIT: I think I can do it now via InstancesEditor>LayerRenderer>ObjectsRenderingService

blurymind commented 2 years ago

I managed to pass it through and register this as a pixi plugin. Now ther is another issue however

PIXI.utils method is missing? I do seem to have it

EDIT: Ah I got it, there is a bug in pixi-tilemap.its in utils.utils!

Silver-Streak commented 2 years ago

@blurymind question on this:

Would it be possible to have it surface int grid and entity data in the parser, even if it's not utilized in the engine currently?

My thought is that would allow a separate PR down the road that anyone could take up that could allow for people to build logic based off Int Grid and Entity stuff from LDTK files. (I think?)

Or would passing data that's not used break stuff?

blurymind commented 2 years ago

@blurymind question on this:

Would it be possible to have it surface int grid and entity data in the parser, even if it's not utilized in the engine currently?

My thought is that would allow a separate PR down the road that anyone could take up that could allow for people to build logic based off Int Grid and Entity stuff from LDTK files. (I think?)

Or would passing data that's not used break stuff?

This pr adds support for both autolayers and intGrid already! https://github.com/4ian/GDevelop/pull/2828/files#diff-230804730aae7eef4ce89228e9120570e8e383b295c728892644045c056baa23R458

Entities however are for another pr as they require some careful planning.

I can add an expression to gdevelop, which lets you get data from ldtk maps in the event sheet. From there on you can use it to set things. That would be potentially simple to do, but it will leave you with the task of manually setting things up with the event sheet

blurymind commented 2 years ago

@4ian for some reason I now get this crash when clicking on an instance, I wonder if its missing something on the c++ engine still

The tilemap properties are still accessible if you select it from the object

4ian commented 2 years ago

@blurymind This means you have an outdated libGD.js, can you try to npm install and npm start again in newIDE/app? If you have compiled the C++ sources (in GDevelop.js) yourself, you need to recompile - otherwise it will download the latest version online when you run npm start :)

blurymind commented 2 years ago

@blurymind This means you have an outdated libGD.js, can you try to npm install and npm start again in newIDE/app? If you have compiled the C++ sources (in GDevelop.js) yourself, you need to recompile - otherwise it will download the latest version online when you run npm start :)

thanks, that sorted it :) I pasted the code twice from my other repository by mistake which was causing this issue. Fixed now

blurymind commented 2 years ago

@4ian I added support for flipped ldtk tiles.

Note that we can now do this to flip them (rotate option,corresponds to pixi's rotate):

 const pixiTilemapFrame = pixiTileMap.tile(
                tileTexture,
                xPos,
                yPos,
                { alpha: layer.opacity, rotate }
              );

so there is no need for caching and complicated code. Should I remove that from how tiled does it? I am having trouble understanding how exactly you are getting the rotation for it.

EDIT: I will try today to remove the need to cache flipped tiles when parsing tiled.

blurymind commented 2 years ago

@4ian where should I add my examples to? I remember the demo projects got moved to another repository.

Also about tests - by that do you mean just example projects testing if specific features work? What is a good example of an extension test?

blurymind commented 2 years ago

@4ian I got the ldtk resource to automatically pull atlases and add them via the resource manager, but there is one problem.

If the user deletes the atlas from resources that ldtk added and then hits the play button - because the update method that checks these depepndencies and adds the atlases is not ran again - the game starts with no atlas.

Do we have a life cycle method we can use in JsExtension.js that is triggered right before the game is playtested? I think that if I trigger my update method whenever the user playtest - then ldtk will always start with its dependency atlas resources, regardless if someone deletes them.

If we can get this to work well, it can also be done for tiled json files which also contain relative paths to any atlases and even tileset jsons. The user will no longer be limited to one atlas,one tileset and have to manually select them

4ian commented 2 years ago

@blurymind can you describe how this automatic importing works? (Sorry I'm on holidays so no time to dive in the code at the moment :)) I can then check where it's best to do that.

blurymind commented 2 years ago

@blurymind can you describe how this automatic importing works? (Sorry I'm on holidays so no time to dive in the code at the moment :)) I can then check where it's best to do that.

Ah no worries, hope its going well :)

The way it works is very basic atm, but it does the trick of automatically adding image resources when an ldtk resource is added.

When parsing the ldtk file, the tilemap helper passes the ldtk files's path as well as the texture's relative path https://github.com/4ian/GDevelop/pull/2828/files#diff-230804730aae7eef4ce89228e9120570e8e383b295c728892644045c056baa23R190

to

https://github.com/4ian/GDevelop/pull/2828/files#diff-7a3c239ffb464bda6f26203444781052cf8022fa89f851ded73b03077215000cR659

to this._pixiResourcesLoader.getPixiTextureRelativeToFile

Then with the ldtk file's path and the relative path to the texture from it, this is passed to https://github.com/4ian/GDevelop/pull/2828/files#diff-8aa3ba6d7875a6046f13fd522a758e611022d74ad44af14b7c503b5d489fdf63R143

which uses the data to figure out the absolute path of the texture which the ldtk file is trying to add. Then finally the resource is added by gdevelop's resources manager.

That is of course, if it didnt exist already. So when you load an ldtk file, you dont need to also specify any atlas textures like tiled does. Just point it to the ldtk file and the ldtk file will point gdevelop to all the atlases it needs.

There is a problem from this, which is that it is not a pure dependency being tracked and the user can easily break it by manually deleting the resource which ldtk pulled when it was parsed, then playtest the game.

I was wondering if I should trigger this somehow every time before playtesting or exporting a game, but I'm not sure if we have a JsExtension lifecycle that does that or if this is the best way to go.

Having a mechanic to add new resources directly and automatically like that can be very very helpful in also improving the UX for tiled - as that also contains information about resources it uses and their relative paths and we are still limiting users to only one atlas there.

In this PR I am overcoming that limitation and its much faster to set up a map by selecting a single file, but it comes with some caveats which needs to be figured out. This happens to be the simplest thing I could come up with to do it, but it might not be the right way to go or at a minimum requires at least being triggered before the game runs or is exported from the extension and I dont know how to do that

4ian commented 2 years ago

I see! So currently it's a bit hacky indeed, I think we could have something like:

a JsExtension.js file can "hook" a function called whenever a resource is added. This allows you to add a new resource or more if there are references to other files.
the editor will call this function when a resource is added or when the user ask to "refresh" it

Nothing fairly obvious though to do, can be postoponed, but we'll need to clean that up before committing this.

Silver-Streak commented 2 years ago

@blurymind Just seeing where this one is at. Are we waiting on other input from 4ian, or are the changes mentioned above things he is meant to be doing?

I've been putting off any more level design stuff until I can just outright do it directly in LDtk without the migration steps. 😆

blurymind commented 2 years ago

@blurymind Just seeing where this one is at. Are we waiting on other input from 4ian, or are the changes mentioned above things he is meant to be doing?

I've been putting off any more level design stuff until I can just outright do it directly in LDtk without the migration steps. 😆

Yes it needs a formal review :) he's on holiday atm I believe.

The rendering functionality is all there now. I can start making a demo project, but don't know to which repository. The examples have been moved to another tracker I believe

Silver-Streak commented 2 years ago

Examples are now over here: https://github.com/GDevelopApp/GDevelop-examples/

Silver-Streak commented 2 years ago

Just pinging @4ian as unless I misunderstand, this is just awaiting review. This will likely give folks a workaround for https://github.com/4ian/GDevelop/pull/2296, as well.

4ian commented 2 years ago

Added to my list, hope to have a first review done early this week!

blurymind commented 2 years ago

Thanks for reviewing this @4ian :) I will have a look over the weekend

Silver-Streak commented 2 years ago

Just a general check in on how we're feeling on current state/next steps, @blurymind

blurymind commented 2 years ago

I have some review notes to address on the pr, but might be away for a bit due to travel the next few days.

There is one overarching problem which I'm not sure how we will address - which is the ability to dynamically load resources dependent to a resource. The way it's done at the pr works, but is a bit hacky.

On Tue, Aug 31, 2021 at 4:53 PM Silver-Streak @.***> wrote:

Just a general check in on how we're feeling on current state/next steps, @blurymind https://github.com/blurymind

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/2828#issuecomment-909362903, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRRWVM3C3U6LBXIXBQJ7MDT7T3GDANCNFSM5AXPB23A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Silver-Streak commented 2 years ago

Thanks for the update! Also, have a good (and safe) trip.

Not sure if @4ian has any guidance on the resource loading. I'd imagine this would also be beneficial for a bunch of different items long term (one being if a music editor is ever integrated).

blurymind commented 2 years ago

@Silver-Streak thank you! :)

The biggest other potential feature that could benefit from this is dragonbones animation resources imo. They work on the same principle - you load a json file, which pulls a bunch of other files relative to it.

Silver-Streak commented 2 years ago

As I'm working on migrating the other bounty (LDtk bundling) over to Rysolv, I was reminded of this one. Just wanted to check in with @4ian and @blurymind on this one to see if a solution had been found for the resource loading/next steps?

Silver-Streak commented 2 years ago

Checking in on here again for @4ian and @blurymind as a partial heads up: Bountysource seems to be nearing abandonment. I'm seeing more and more posts on their github where people are waiting months to get paid out, if at all. Their Github login integration has broken, so if you don't have a valid cookie you cannot sign in to your account via Github anylonger.

So rather than just interest, we probably want to finish this one out as soon as feasible so that the funds don't get lost in the aether.

blurymind commented 2 years ago

Hi, I am sorry for the delayed replies. I'm on a holiday atm, so time is limited. I can still get to the laptop and work on it during the week, but the problem with the resource dependencies will still be there.

My understanding from @4ian is that we still need to come up with a better solution for that than what is in the pr.

The other review notes are simple to address and shouldn't take me long. I also need to upload my demo project to another git repo I think.

On Sun, 19 Sep 2021, 12:33 Silver-Streak, @.***> wrote:

Checking in on here again for @4ian https://github.com/4ian and @blurymind https://github.com/blurymind as a partial heads up: Bountysource seems to be nearing abandonment. I'm seeing more and more posts on their github where people are waiting months to get paid out, if at all. Their Github login integration has broken, so if you don't have a valid cookie you cannot sign in to your account via Github anylonger.

So rather than just interest, we probably want to finish this one out as soon as feasible so that the funds don't get lost in the aether.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/2828#issuecomment-922443940, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRRWVKKHERJCQDVO23UXVDUCWVABANCNFSM5AXPB23A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Silver-Streak commented 2 years ago

Hi, I am sorry for the delayed replies. I'm on a holiday atm, so time is limited. I can still get to the laptop and work on it during the week, but the problem with the resource dependencies will still be there. My understanding from @4ian is that we still need to come up with a better solution for that than what is in the pr. The other review notes are simple to address and shouldn't take me long. I also need to upload my demo project to another git repo I think.

No worried at all. I just wanted to make folks aware of the current state of Bountysource. Any future bounties I post will be through Rysolv, and I am working towards trying to pull the full ldtk integration/bundling funds from bountysource to move there.

I will not be able to do so with the funds for this current effort's bounty, so I wanted to ensure we kept awareness up/kept an eye out. Please enjoy your holiday!

Silver-Streak commented 2 years ago

Hey @blurymind, it's been ~a month since the last update. Just checking in if you're still on holiday or if there's an update?

blurymind commented 2 years ago

Sorry still on holiday. I'm flying back to the UK tomorrow actually, so should be able to get back on track soon

On Fri, 15 Oct 2021, 18:07 Silver-Streak, @.***> wrote:

Hey @blurymind https://github.com/blurymind, it's been ~a month since the last update. Just checking in if you're still on holiday or if there's an update?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/2828#issuecomment-944376749, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRRWVKU6F3UD7ADLI7RGFDUHA7RLANCNFSM5AXPB23A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Silver-Streak commented 2 years ago

Thanks for the update! I hope you had a great time.

Silver-Streak commented 2 years ago

Hey @blurymind, I hope you had a great holiday!

Just doing another check-in on this. Any updates?

blurymind commented 2 years ago

Hi, sorry haven't had a chance to pick it up yet, but I intend to. I have to fix merge conflicts on the pr yet again and also sort out florians comments.

I am still unsure how to deal with the resource dependency part. If this is something that needs to be addressed on the c++ engine, I would be happy to split the bounty 50/50 with anyone more proficient who knows how to implement that part.

Btw did all the boutysource sum manage to be transferred to that new site? I am confused what the bounty on this is now

On Tue, 2 Nov 2021, 07:30 Silver-Streak, @.***> wrote:

Hey @blurymind https://github.com/blurymind, I hope you had a great holiday!

Just doing another check-in on this. Any updates?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/2828#issuecomment-957164603, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRRWVPZGGJW7NJJCZVC2ITUJ6OSHANCNFSM5AXPB23A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Silver-Streak commented 2 years ago

Hi, sorry haven't had a chance to pick it up yet, but I intend to.

No worries! Conversation around tilemaps has been popping up more and more on the discord, and newer users are frustrated with Tiled Map Maker, so getting native support for LDTK files would help solve that and it reminded me to follow back up on this.

I have to fix merge conflicts on the pr yet again and also sort out florians comments. I am still unsure how to deal with the resource dependency part. If this is something that needs to be addressed on the c++ engine, I would be happy to split the bounty 50/50 with anyone more proficient who knows how to implement that part.

@4ian can you comment on the dependency questions above?

Btw did all the boutysource sum manage to be transferred to that new site? I am confused what the bounty on this is now

This bounty, since it has funds from deepknight themself, is not transferred from Bountysource as I cannot cover replacing the fee costs bountysource takes if I were to migrate it at this time. (you would basically be double dinged for fees in that case) This specific bounty is still here: https://www.bountysource.com/issues/97132654-500-bounty-add-support-for-tilemaps-made-in-ldtk-build-a-parser-for-ldtk-files

Bountysource still exists right now, and I was able to extract the other bounties after hounding their support, so it at least seems like they're showing signs of life, but they are definitely far too unreliable for new bounties.

The other bounty you're thinking of is the bounty for integrating/bundling LDTK itself ($200), which is migrated to Rysolv and is here: https://rysolv.com/issues/detail/d62c42cc-1a30-445a-a0ed-e19a62478005

If folks still want to migrate this PR's bounty to Rysolv as well, I can, but we'd be reducing the total bounty by the fees twice, which I'd rather avoid. Let me know.

blurymind commented 2 years ago

@Silver-Streak can you link us to the bounty page for the ldtk parsing (this pr) on rysolv?

so to be clear on this bounty: https://www.bountysource.com/issues/97132654-500-bounty-add-support-for-tilemaps-made-in-ldtk-build-a-parser-for-ldtk-files

My understanding is that the 200 has been transfered to the other site, @deepnight 's 300$ hasnt, But bountysource still shows the total as 500 - which is wrong? Does that mean that his 300 will not be awarded once this ticket is closed - or they will be awarded by bounty source separately?

shouldnt we update the ticket's title to reflect that? If @deepnight moves his contribution to rysolv, can we close the one on bountysource completely so as to clear the confusion?

Silver-Streak commented 2 years ago

@blurymind there appears to be some confusion.

Unless something has changed, This PR was just for the LDTK file support in GD5. deepnight contributed to this bounty bringing its total to $500. It was and remains entirely on Bountysource. ( https://www.bountysource.com/issues/97132654-500-bounty-add-support-for-tilemaps-made-in-ldtk-build-a-parser-for-ldtk-files ) This PR/bounty originated from this issue here. https://github.com/4ian/GDevelop/issues/2434. This is the bounty you've posted a screenshot of above, which I've contributed ~$150 to. (We'll call this Newest bounty)

There is a completely separate bounty (We'll call this Oldest bounty) for integrating/bundling LDTk into GDevelop, for $200. This bounty was moved to Rysolv ( https://rysolv.com/issues/detail/d62c42cc-1a30-445a-a0ed-e19a62478005 ). This bounty originated from this discussion here: https://github.com/4ian/GDevelop/discussions/2352. It was originally another bounty on Bountysource, of which I had also contributed $150 to.

And this recreated issue here: https://github.com/4ian/GDevelop/issues/2991

In the discussion (around March 2021), 4ian/yourself/I discussed and agreed it would make the most sense to split off a separate bounty entirely for adapting the TileMap Object in GD5 to support .LDTK files/format. That split became the "New Bounty". This would make it easier to eventually bundle LDTK entirely, in the "Old Bounty".

Let me know if there's anymore confusion, as I know it has been a long road. 😄

bjorn commented 2 years ago

No worries! Conversation around tilemaps has been popping up more and more on the discord, and newer users are frustrated with Tiled Map Maker

Hey @Silver-Streak, I'm not currently on the GDevelop Discord, but I'd be very interested to hear what frustrations these newer users are having. Maybe there's something I can do to help reduce the frustration? Not wanting to further hijack this PR, please feel free to join the Tiled Discord or just open an issue on mapeditor/tiled for giving feedback.

Silver-Streak commented 2 years ago

@bjorn A lot of our GD5 users skew much younger than other engines. Most of the feedback is around difficulty exporting with embedded tilesets (I'm not sure how you could improve this), and inability to comprehend or set up automapping. Many of them have come from Unity or other engines, and LDTKs automapping is similar to those solutions, so it is very much a comfort level thing.

I'll try to hop on your discord later to sum up stuff, but I really don't think there is anything inherently wrong with Tiled as an experienced user, its just daunting for these newer/younger game devs.

Edit: Just to close the loop on this, Bjorn and I chatted on Discord to get him more details.

blurymind commented 2 years ago

@Silver-Streak ah I see, so the bountysource one is still valid :) Thank you for clarifying this. :+1:

Yeah I agree they should be separate tasks/bounties, unfortunately they do depend on each other. I have to finish this one in order to even start doing the editor one

Silver-Streak commented 2 years ago

Yeah I agree they should be separate tasks/bounties, unfortunately they do depend on each other. I have to finish this one in order to even start doing the editor one

Yep! Absolutely. I think that was our last conversation on this, that this one would need to be completed first. No worries!

blurymind commented 2 years ago

@4ian I resolved conflicts on this after merging dev and now it is crashing Not sure why, seems unrelated to this pr

Could it be a case of having to recompile gd?

Bouh commented 2 years ago

Could it be a case of having to recompile gd?

Yes, a recompile is required, there were a few changes on the core and code generation.

Silver-Streak commented 2 years ago

@blurymind Any success on the recompile?

blurymind commented 2 years ago

@Silver-Streak unfortunately it wont let me compile it

what is emconfigure?

@Bouh any ideas?

Edit: I think I managed to recompile it now - it required updating emsdk too, switching it to its master branch

blurymind commented 2 years ago

@Silver-Streak I am back on track now, will address the review notes this week

@Bouh thanks for the assistance

blurymind commented 2 years ago

@4ian I have now applied all review notes (apart of the resource dependency one, since Im not sure how else we will solve that without implementing something on the c++ side)

After merging dev I am now experiencing a new problem - incrementing the level index during runtime gives me some sort of a gdjs error

"TypeError: gdjs.New_32sceneCode.GDNewObjectObjects1[i].getLevelndex is not a function
    at Object.gdjs.New_32sceneCode.eventsList0 (file:///tmp/GDTMP-1000/preview/code0.js:45:107)
    at u.gdjs.New_32sceneCode.func [as _eventsFunction] (file:///tmp/GDTMP-1000/preview/code0.js:60:22)
    at u.renderAndStep (file:///tmp/GDTMP-1000/preview/runtimescene.js:1:6149)
    at a.step (file:///tmp/GDTMP-1000/preview/scenestack.js:1:409)
    at file:///tmp/GDTMP-1000/preview/runtimegame.js:1:5665
    at i (file:///tmp/GDTMP-1000/preview/pixi-renderers/runtimegame-pixi-renderer.js:1:7000)"

"gdjs.New_32sceneCode.GDNewObjectObjects1[i].getLevelndex is not a function"

perhaps I broke it in some commit when resolving conflicts?

Silver-Streak commented 2 years ago

@blurymind Based off the other thread, it sounds like Davy believes his change is no longer blocking this. Is that accurate (and if so are you running into any other blockers), or is it still impactful?

Silver-Streak commented 2 years ago

(ignore me hitting close with comment instead of comment. Apologies)

blurymind commented 2 years ago

Happy new year guys :partying_face:

@Silver-Streak @4ian as mentioned merging dev into it and converting it to the new api broke a feature - still need to fix.

Other than that I am not sure how to proceed with it without using the current approach to load dependent resources, which is "hacky". I need some input from @4ian about what to do on that front to get it moving to a realistic resolution.

In terms of https://github.com/4ian/GDevelop/issues/2775 I dont mind it either way. If he merges his first, I am gonna have to refactor this entire pr - might be easier to close it and create a new pr or something. But I will do it depending on what @4ian thinks is the better way of moving forward :)

Btw I updated it to include the two new todo entries

4ian / GDevelop

Ldtk file support for tilemaps in Gdevelop #2828