Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: Add convenience method for getting images in each layer #66

Merged
merged 11 commits into from
Mar 28, 2023

Conversation

Hwan-seok
Copy link
Contributor

Description

This PR adds the convenience method to the TiledMap that gets all TiledImage in the specific layer.

If the layer is Group, the method gets all images in the group recursively.

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

N/A

@Hwan-seok
Copy link
Contributor Author

Seems some dependencies like flame_lint are outdated😅
Should I make a separate PR?

@spydon
Copy link
Member

spydon commented Mar 27, 2023

Seems some dependencies like flame_lint are outdated😅 Should I make a separate PR?

You can do it in the same one if it is not too many changes :)

@Hwan-seok
Copy link
Contributor Author

Hwan-seok commented Mar 27, 2023

@spydon
Copy link
Member

spydon commented Mar 27, 2023

As of these issues,

  1. The flutter test command should require a flutter_test or test_api dependency (and check for it). flutter/flutter#91107
  2. fix(test_api): 0.5.0 breaks flutter test for projects that dont depend on flutter_test dart-lang/test#1977

the tests are failing(technically, they could not be loaded).

Hmm, why is this package even dependant on flutter (and flutter test)? 🤔

@Hwan-seok
Copy link
Contributor Author

Hwan-seok commented Mar 27, 2023

The test code action uses flutter test by the
https://github.com/flame-engine/flame-test-action/blob/fdf4cd9cf047b1b5e1359cf89ad0d88bca535523/scripts/test.sh#L10

Hmm, why is this package even dependant on flutter (and flutter test)? 🤔

Hmm..............🤣

@Hwan-seok
Copy link
Contributor Author

After removing flutter from the dependencies, it complains dart:ui could not be imported ...!

@spydon
Copy link
Member

spydon commented Mar 27, 2023

After removing flutter from the dependencies, it complains dart:ui could not be imported ...!

Yeah, it will be a much bigger project to get flutter out of there.
So probably just add the flutter_test dependency and the tests should run again.

@Hwan-seok
Copy link
Contributor Author

Hwan-seok commented Mar 27, 2023

OK, I replaced the test with flutter_test.

packages/tiled/lib/src/tiled_map.dart Outdated Show resolved Hide resolved
packages/tiled/lib/src/tiled_map.dart Outdated Show resolved Hide resolved
packages/tiled/lib/src/tiled_map.dart Outdated Show resolved Hide resolved
Comment on lines 231 to 243
for (final row in layer.tileData ?? <List<Gid>>[]) {
for (final gid in row) {
final tileGid = gid.tile;

if (tileGid == emptyTile || inspectedGids.contains(tileGid)) {
continue;
}
inspectedGids.add(tileGid);
usedTilesets.add(tilesetByTileGId(tileGid));
}
}

return usedTilesets
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be

  final rows = layer.tileData ?? <List<Gid>>[];
  final gids = rows.expand((row) => row.map((gid) => gid.tile)).where((gid) => gid != emptyTile).toSet();
  return gids.map(tilesetByTileGId) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such a compact solution it is!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying to decide whether or not to divide the cascading in the middle.
Would you kindly give some thoughts on this?

      return rows
          .expand((row) => row.map((gid) => gid.tile))
          .where((gid) => gid != emptyTile)
          .toSet()
          .map(tilesetByTileGId)
          .toSet()
          .expand(
            (tileset) =>
                [tileset.image, ...tileset.tiles.map((tile) => tile.image)],
          )
          .whereNotNull()
          .toList();
      final gids = rows
          .expand((row) => row.map((gid) => gid.tile))
          .where((gid) => gid != emptyTile)
          .toSet();

      return gids
          .map(tilesetByTileGId)
          .toSet()
          .expand(
            (tileset) =>
                [tileset.image, ...tileset.tiles.map((tile) => tile.image)],
          )
          .whereNotNull()
          .toList();

Copy link
Member

Choose a reason for hiding this comment

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

The second one might give slightly more readability.
You shouldn't have to do toSet again after map(tilesetByTileGId), since the gid list already is unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You shouldn't have to do toSet again after map(tilesetByTileGId), since the gid list already is unique.

The gid list is unique but they can be nonunique after map(tilesetByTileGId) because different gid can have the same tileset, doesn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, true :)

Copy link
Member

@luanpotter luanpotter left a comment

Choose a reason for hiding this comment

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

Nice!

@spydon spydon merged commit 1d3043f into flame-engine:main Mar 28, 2023
@Hwan-seok Hwan-seok deleted the hwanseok.tiled-layer-image branch March 28, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants