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 network assets package. #2314

Merged
merged 25 commits into from
Mar 18, 2023
Merged

Conversation

erickzanardo
Copy link
Member

@erickzanardo erickzanardo commented Feb 2, 2023

Description

Adds some documentation and a package that can be used to fetch assets from the network.

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

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.

I feel like this could be a great package for fetching files from the Internet and then caching them locally -- regardless of the type of the file. Currently it focuses on images, but what about audio files, fire atlas bundles, tiled levels, yarn scripts, custom json, etc?

The task of converting a file into an Image is quite trivial -- and we can add a helper function fromBytes into the core Flame Images class:

Future<Image> fromBytes(String key, Uint8List bytes) {
    return (_assets[key] ??= _ImageAsset.future(decodeImageFromList(bytes)))
        .retrieveAsync();
  }

doc/flame/rendering/images.md Outdated Show resolved Hide resolved
packages/flame_network_image/example/analysis_options.yaml Outdated Show resolved Hide resolved
@erickzanardo
Copy link
Member Author

I feel like this could be a great package for fetching files from the Internet and then caching them locally -- regardless of the type of the file. Currently it focuses on images, but what about audio files, fire atlas bundles, tiled levels, yarn scripts, custom json, etc?

The task of converting a file into an Image is quite trivial -- and we can add a helper function fromBytes into the core Flame Images class:

Future<Image> fromBytes(String key, Uint8List bytes) {
    return (_assets[key] ??= _ImageAsset.future(decodeImageFromList(bytes)))
        .retrieveAsync();
  }

That is a good idea tbh, I don't think we need to add a fromBytes helper on Flame since the decodeImageFromList is just simple call :)

How should we call this then? flame_network_assets?

@st-pasha
Copy link
Contributor

st-pasha commented Feb 3, 2023

How should we call this then? flame_network_assets?

Some more alternatives: flame_network_cache, flame_http_cache.

doc/flame/rendering/images.md Outdated Show resolved Hide resolved
packages/flame_networks_assets/.gitignore Outdated Show resolved Hide resolved
packages/flame_networks_assets/example/.gitignore Outdated Show resolved Hide resolved
Comment on lines 100 to 109
(
String url, {
Map<String, String>? headers,
}) =>
http.get(Uri.parse(url), headers: headers).then((response) {
return FlameAssetResponse(
statusCode: response.statusCode,
bytes: response.bodyBytes,
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a statue method instead of a function defined inline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean about statue method

/// Flag indicating if files will be cached in the local storage.
final bool cacheInStorage;

final bool _isWeb;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use kIsWeb instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is used actually, if you look on the constructor, it is the default value for this attribute.

I wrapped it in this attribute to make it easier to mock on tests.

@erickzanardo erickzanardo changed the title feat: Add network image package. feat: Add network assets package. Mar 6, 2023
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lookin' good! Mostly just grammar stuff.

@@ -33,3 +33,4 @@ yarnspinner
рушниці
рушниць
рушниця
unawaited
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should go in the gamedev_dictionary.txt?

doc/bridge_packages/bridge_packages.md Outdated Show resolved Hide resolved
doc/bridge_packages/bridge_packages.md Outdated Show resolved Hide resolved
packages/flame_network_assets/pubspec.yaml Outdated Show resolved Hide resolved
@erickzanardo erickzanardo requested review from spydon and Hwan-seok and removed request for Hwan-seok March 18, 2023 19:11
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm!

@erickzanardo erickzanardo enabled auto-merge (squash) March 18, 2023 20:04
@erickzanardo erickzanardo merged commit 61d6965 into main Mar 18, 2023
@erickzanardo erickzanardo deleted the erick.flame-network-image branch March 18, 2023 20:12
@@ -0,0 +1,27 @@
name: flame_network_assets
description: Network assets support for Flame.
Copy link
Member

Choose a reason for hiding this comment

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

nit: this will trigger the "package description too short" pub penalty. I think the min is 60 characters. not endorsing unnecessary verbosity but losing points is also bad

Copy link
Member

Choose a reason for hiding this comment

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

do we need an empty file at all?

Comment on lines +15 to +16
<img src="https://github.com/flame-engine/flame_network_image/workflows/Lint/badge.svg?branch=master&event=push" alt="Test" />
<a title="Discord" href="https://discord.gg/pxrBmy4" ><img src="https://img.shields.io/discord/509714518008528896.svg" /></a>
Copy link
Member

Choose a reason for hiding this comment

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

why not the 4 standard badges? (I know the package hasnt been released yet if you add as soon as it is it will work)

sdk: flutter

dev_dependencies:
flame_lint: ^0.1.3
Copy link
Member

Choose a reason for hiding this comment

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

use 0.2.0 ?

@@ -0,0 +1,35 @@
# FlameNetworkAssets

`FlameNetworkAssets` is a bridge package focused on providing a solution to fetch, and cache assets
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`FlameNetworkAssets` is a bridge package focused on providing a solution to fetch, and cache assets
`FlameNetworkAssets` is a bridge package focused on providing a solution to fetch and cache assets

Comment on lines +9 to +11
By default, the package relies on the `http` package to make http requests, and `path_provider`
to get the place to store the local cache, to use a different approach for those, use the
optional arguments in the constructor.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By default, the package relies on the `http` package to make http requests, and `path_provider`
to get the place to store the local cache, to use a different approach for those, use the
optional arguments in the constructor.
By default, the package relies on the `http` package to make http requests, and `path_provider`
to get the place to store the local cache; to use a different approach for those, use the
optional arguments in the constructor.

Comment on lines +143 to +146
```{note}
Check [`flame_network_assets`](https://pub.dev/packages/flame_network_assets)
for a ready to use network assets solution that provides a built in cache.
```
Copy link
Member

Choose a reason for hiding this comment

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

we could probably be even more persuasive about using the flame_network_assets, and frame importing http "by hand" as an alternative

/// Flag indicating if files will be cached in the local storage.
final bool cacheInStorage;

final bool _isWeb;
Copy link
Member

Choose a reason for hiding this comment

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

can we just set the default value of cacheInStorage to be kIsWeb instead of having two flags?

}

final response = await _get(url, headers: headers);
if (response.statusCode >= 200 && response.statusCode < 400) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: invert the if for an early-exit

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.

5 participants