Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Possibility to create RenderableTiledMap from TiledMap #1534

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

Hwan-seok
Copy link
Contributor

Description

Fixes #1533

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

spydon
spydon previously approved these changes Apr 11, 2022
Copy link
Contributor

@st-pasha st-pasha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a bit inconsistent that loadMap() returns a TiledMap, while fromMap() returns a RenderableTileMap?

@spydon
Copy link
Member

spydon commented Apr 11, 2022

Isn't it a bit inconsistent that loadMap() returns a TiledMap, while fromMap() returns a RenderableTileMap?

Good catch, maybe fromMap should be renamed to fromTiledMap, since this is in the RenderableTileMap class I think it makes sense that it returns that type. And maybe loadMap should actually be living in the TiledMap class?

@spydon
Copy link
Member

spydon commented Apr 11, 2022

Created: flame-engine/tiled.dart#42
This PR should be able to use that one instead of loadMap.

@spydon spydon dismissed their stale review April 11, 2022 20:46

It should use the tiled method instead

@spydon
Copy link
Member

spydon commented Apr 11, 2022

@Hwan-seok If you do a pub upgrade now tiled.dart should expose TiledMap.fromString that you can use instead of loadMap. You have to send in FlameTsxProvider.parse to the method as the second argument.

@Hwan-seok
Copy link
Contributor Author

@spydon
I got it, I will fix the PR.

@Hwan-seok Hwan-seok force-pushed the hwanseok.open-load-map branch from 22afcca to 7c190fc Compare April 12, 2022 02:31
@spydon
Copy link
Member

spydon commented Apr 12, 2022

@spydon
I got it, I will fix the PR.

Super, looks great! I'll do a release of this tonight. :)

@Hwan-seok
Copy link
Contributor Author

@spydon
Great!

By the way, Is the release includes the main package of flame? because it is not compatible with flame: ^1.1.0 because of #1522 but this has dependencies on it.

@spydon
Copy link
Member

spydon commented Apr 12, 2022

@spydon Great!

By the way, Is the release includes the main package of flame? because it is not compatible with flame: ^1.1.0 because of #1522 but this has dependencies on it.

Yup, melos calculates which packages that needs to be released, so at least Flame 1.1.1, flame_forge2d and flame_test will also be released.

Copy link
Contributor

@wolfenrain wolfenrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@spydon spydon merged commit 5ed0833 into flame-engine:main Apr 12, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[flame_tiled] add availability to loading TiledMap directly
4 participants