fix: TiledComponent now can be safely loaded regardless of the order

[Hwan-seok]

Description

Basically, the TiledAtlas must not be reused without clone() because it includes batch which is different for each TiledAtlas.
But the following snippets(before this PR) can lead to the reuse of the TiledAtlas if I have loaded the map with the same tilesets(same atlasKey).

   return atlasMap[key] ??= TiledAtlas._(...); 

Also, this PR includes the change of the TestAssetBundle.
I don't know about the exact intention of it.
But it can load only a single map and also it ignored the actual file name to the bundle's first file name.
This makes tests to be hard to debug.

This change is quite far from the original one. So please feel free to tell me to divide it to the separate PR.

Checklist

• I have followed the Contributor Guide when preparing my PR.
• 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 or docs.

Breaking Change?

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

Related Issues

Fixes #2390

[Hwan-seok]

This is the only production change.

[spydon]

Makes sense, lgtm!

[ufrshubham]

A question, if the atlas keys are same for 2 maps, wouldn't that case be handled by existing atlasMap.containsKey(key) few lines above?

Just to confirm this, I ran the new testcase with and without this fix. It passed in both the cases.

[Hwan-seok]

You are absolutely right, my test case was terrible. Thanks for pointing it out!

Also, my explanation of this issue also has some flaws.
As you seen, maps are handled by if(atlasMap.containsKey(key)) if they are loaded sequentially😂

But if they are loaded in the race condition, both of them would be stopped at:
final image = (await Flame.images.load(key)).clone();.
Then they keep going...
As a result, they reach ??= without if(atlasMap.containsKey(key)) check.

So the test case should be changed from:

final component1 = await TiledComponent.load(
  'single_tile_map_1.tmx',
  Vector2(16, 16),
);
final component2 = await TiledComponent.load(
  'single_tile_map_2.tmx',
  Vector2(16, 16),
);

to:

final components = await Future.wait([
  TiledComponent.load(
    'single_tile_map_1.tmx',
    Vector2(16, 16),
  ),
  TiledComponent.load(
    'single_tile_map_2.tmx',
    Vector2(16, 16),
  )
]);

The difference between these is racing or not.

But even after changing the test case, it passes with and without this fix.

This is because of here:

flame/packages/flame_tiled/lib/src/renderable_layers/renderable_layer.dart

Line 46 in f125593

atlas: atlas.clone(),

This line clones TiledAtlas one more time. So it was safe indeed.

You can remove this clone() and then see the (updated) test would be failed with ??=.

But still, I think it should be changed to ??= to = because of

1. Consistency with others like more than one tileset
2. It's safe to use without a thorough understanding of this class (maybe extended from 1 ). Without this change, anyone using this class should be aware of tiledAtlas should be cloned.

Probably, you have been curious about why I had an issue without any problem in the code.
That is because I am using the forked flame version with optimized TiledLayer without clone() one more time.
But regardless of my forked version, this would be better to be fixed.
I am thoroughly testing my patch in production and will show up to you guys

[ufrshubham]

Okay, that makes sense. So in that case, does something following be better?

Suggested change

	return atlasMap[key] = TiledAtlas._(
	return atlasMap.contains(key) ? atlasMap[key]!.clone() : atlasMap[key] = TiledAtlas._(

It is a bit verbose, but it avoids replacing the atlas in the map if it is already loaded by some other async call.

In any case, consider adding at least a comment at the return about this special case 🙂.

[Hwan-seok]

Sounds great!

This is another option.

if (imageList.length == 1) {
  // The map contains one image, so its either an atlas already, or a
  // really boring map.
  final image = (await Flame.images.load(key)).clone();

  if (atlasMap.containsKey(key)) {
    return atlasMap[key]!.clone();
  }

  return atlasMap[key] = TiledAtlas._(
    atlas: image,
    offsets: {key: Offset.zero},
    key: key,
  );
}

Which one do you prefer? I will follow your decision.

[ufrshubham]

Go ahead with your option. It looks more readable👍🏼

[Hwan-seok]

Final option:
just overwrite atlasMap[key] without checking if exists(no code change from mine).
IMHO, it has less code but leads to the same result.

[ufrshubham]

From the looks of it, the whole point of having atlasMap seems to be to avoid atlas recreations. But even after caching, we are anyways returning a new or cloned version. So I guess even overwriting without checking should technically be fine. Also, this part of code is not in critical path, so I'd say prefer whichever option is more readable.🙂

[ufrshubham]

LGTM!

[Hwan-seok]

I don't know the reason why the re-request is for unique 😂
Anyway, I added a comment for the review. Thanks to everyone.