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.

Sign up for GitHub

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

fix!: Allow AssetCache to load json files with array at root #2688

Closed
wants to merge 5 commits into from
Closed

fix!: Allow AssetCache to load json files with array at root #2688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
14e47f9
Fixed loading JSON array files
projectitis Aug 27, 2023
03be710
Fix test
projectitis Aug 27, 2023
49806f1
Fix example
projectitis Aug 27, 2023
e152f6a
Linting example
projectitis Aug 27, 2023
5908f4a
Merge branch 'main' into fix/2682-readjson-return-type
projectitis Aug 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion examples/lib/stories/animations/aseprite_example.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ class AsepriteExample extends FlameGame {
@override
Future<void> onLoad() async {
final image = await images.load('animations/chopper.png');
final jsonData = await assets.readJson('images/animations/chopper.json');
final jsonData = await assets.readJson('images/animations/chopper.json')
as Map<String, dynamic>;
final animation = SpriteAnimation.fromAsepriteData(image, jsonData);
final spriteSize = Vector2.all(200);
final animationComponent = SpriteAnimationComponent(
Expand Down
4 changes: 2 additions & 2 deletions packages/flame/lib/src/cache/assets_cache.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ class AssetsCache {
}

/// Reads a json file from the assets folder.
Future<Map<String, dynamic>> readJson(String fileName) async {
Future<dynamic> readJson(String fileName) async {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... This is a clear degradation with having a completely dynamic type here (and a breaking change), maybe we should just have another method to load lists.

Copy link
Contributor Author

@projectitis projectitis Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would strongly argue against that. For a few reasons:

  1. JSON is allowed to be both an array or object at the root
  2. The native decodeJson implementation returns dynamic for this reason
  3. A user may not know what the JSON looks like before they load it, and will then not know which method to choose ahead of time (this is what I personally ran into)
  4. Implementations that use JSON already expect to cast the internal JSON structures to the correct format - see below

This example is from the "aseprite" animation example that I 'fixed' as part of this PR. In the PR, I cast the dynamic JSON to a Map<String, dynamic> before passing to fromAsepriteData. I believe this is the correct approach. As soon as you go into the method you can see that the internal dynamic structures are cast to Map as appropriate (and would be cast to List as well if there were any). So this is already something that consumers of JSON expect to have to do.

factory SpriteAnimation.fromAsepriteData(
    Image image,
    Map<String, dynamic> jsonData,
  ) {
    final jsonFrames = jsonData['frames'] as Map<String, dynamic>; // Projectitis: here
    return SpriteAnimation(
      jsonFrames.values.map((dynamic value) {
        final map = value as Map;
        final frameData = map['frame'] as Map<String, dynamic>; // Projectitis: here
        final x = frameData['x'] as int;
        final y = frameData['y'] as int;
        final width = frameData['w'] as int;
        final height = frameData['h'] as int;
        final stepTime = (map['duration'] as int) / 1000;
        final sprite = Sprite(
          image,
          srcPosition: Vector2Extension.fromInts(x, y),
          srcSize: Vector2Extension.fromInts(width, height),
        );
        return SpriteAnimationFrame(sprite, stepTime);
      }).toList(),
    );
  }

If a user requires a strongly typed JSON implementation, a better approach (for them) would be to look at JSON code generation.

If you did want to go ahead with separate methods for object and array based JSON files, I would suggest:

  1. Keep dynamic readJson as it is
  2. Add Map<String, dynamic> readJsonObject, and
  3. Add List<dynamic> readJsonArray

Thoughts?

Copy link
Member

@spydon spydon Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Even though it obviously is the preferred way to accept all valid json structures, we have to take into consideration that many have already done their implementations on top of the current function definition so we should really try to avoid creating a breaking change for this.

If you did want to go ahead with separate methods for object and array based JSON files, I would suggest:

This suggestion keeps the breaking change. I think only step 3 should be done here, and then the breaking change can be done for v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about introducing dynamic loadJson and marking readJson as deprecated?
I notice all the AssetCache methods are read and all the Images methods are load.
This could be a step towards standardising on load?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that could be a possibility.
What do you think @flame-engine/flame-admin?
I would introduce both 2 and 3 too, so that the user can do one less cast.

Copy link
Contributor Author

@projectitis projectitis Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I propose that we leave this one until v2 or there could be a bunch of confusion. JSON with an array at the root does not seem very common (or it would have come up before now). I am using jsonDecode directly to get around this issue myself.

But how do we mark something so it's not forgotten in v2?

A workaround for those who run into this issue and still want to use AssetsCache to take advantage of caching is:

AssetsCache _assets = AssetsCache():
 
@override
Future<void> onLoad() async {
    /// Instead of 
    final json = await _assets.readJson('data.json');

    // Do one of these
    final json = jsonDecode(await _assets.readFile('data.json'));
    final json = jsonDecode(await _assets.readFile('data.json')) as Map<String, dynamic>;
    final json = jsonDecode(await _assets.readFile('data.json')) as List<dynamic>;
}

Copy link
Member

Choose a reason for hiding this comment

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

But how do we mark something so it's not forgotten in v2?

We add it to the #1938 ticket, I added it now :)
Should we close this for now then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, go ahead :)

final content = await readFile(fileName);
return jsonDecode(content) as Map<String, dynamic>;
return jsonDecode(content);
}

Future<_StringAsset> _readFile(String fileName) async {
Expand Down
16 changes: 16 additions & 0 deletions packages/flame/test/_resources/test_1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[
1,
2.0,
true,
null,
"Hello",
{
"key": "value"
},
[
1,
2.0,
"three",
false
]
]
16 changes: 16 additions & 0 deletions packages/flame/test/_resources/test_2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"int": 1,
"double": 2.0,
"bool": true,
"null": null,
"string": "Hello",
"object": {
"key": "value"
},
"array": [
1,
2.0,
"three",
false
]
}
10 changes: 8 additions & 2 deletions packages/flame/test/cache/assets_cache_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@ void main() {
);
});

test('readJson', () async {
test('readJson with array root', () async {
final assetsCache = AssetsCache(prefix: '');
final file = await assetsCache.readJson(fixture('chopper.json').path);
final file = await assetsCache.readJson(fixture('test_1.json').path);
expect(file, isA<List<dynamic>>());
});

test('readJson with object root', () async {
final assetsCache = AssetsCache(prefix: '');
final file = await assetsCache.readJson(fixture('test_2.json').path);
expect(file, isA<Map<String, dynamic>>());
});

Expand Down