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

AssetMetaMode #10623

Merged
merged 3 commits into from
Nov 27, 2023
Merged

AssetMetaMode #10623

merged 3 commits into from
Nov 27, 2023

Conversation

cart
Copy link
Member

@cart cart commented Nov 18, 2023

Objective

Fixes #10157

Solution

Add AssetMetaCheck resource which can configure when/if asset meta files will be read:

app
  // Never attempts to look up meta files. The default meta configuration will be used for each asset.
  .insert_resource(AssetMetaCheck::Never)
  .add_plugins(DefaultPlugins)

This serves as a band-aid fix for the issue with wasm's HttpWasmAssetReader creating a bunch of requests for non-existent meta, which can slow down asset loading (by waiting for the 404 response) and creates a bunch of noise in the logs. This also provides a band-aid fix for the more serious issue of itch.io deployments returning 403 responses, which results in full failure of asset loads.

If users don't want to include meta files for all deployed assets for web builds, and they aren't using meta files at all, they should set this to AssetMetaCheck::Never.

If users do want to include meta files for specific assets, they can use AssetMetaCheck::Paths, which will only look up meta for those paths.

Currently, this defaults to AssetMetaCheck::Always, which makes this fully non-breaking for the 0.12.1 release. However it is worth discussing making this AssetMetaCheck::Never by default, given that I doubt most people are using meta files without the Asset Processor enabled. This would be a breaking change, but it would make WASM / Itch deployments work by default, which is a pretty big win imo. The downside is that people using meta files without processing would need to manually enable AssetMetaCheck::Always, which is also suboptimal.

When in AssetMetaCheck::Processed, the meta check resource is ignored, as processing requires asset meta files to function.

In general, I don't love adding this knob as things should ideally "just work" in all cases. But this is the reality of the current situation.


Changelog

  • Added AssetMetaCheck resource, which can configure when/if asset meta files will be read

@cart cart added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds labels Nov 18, 2023
@cart cart added this to the 0.12.1 milestone Nov 18, 2023
@alice-i-cecile
Copy link
Member

Currently, this defaults to AssetMetaMode::Always, which makes this fully non-breaking for the 0.12.1 release. However it is worth discussing making this AssetMetaMode::Never by default, given that I doubt most people are using meta files without the Asset Processor enabled. This would be a breaking change, but it would make WASM / Itch deployments work by default, which is a pretty big win imo. The downside is that people using meta files without processing would need to manually enable AssetMetaMode::Always, which is also suboptimal.

IMO the strategy here is to put the correct config in the Bevy template used for the jam, but keep the existing default behavior. I don't think changing this is the right choice long-term, otherwise I'd be more okay with swapping the default now.

@DGriffin91
Copy link
Contributor

Will the AssetMetaMode::Paths also work on wasm / itch.io? Is it planned (or already the case) that meta files are usable on the web?

@cart
Copy link
Member Author

cart commented Nov 22, 2023

Will the AssetMetaMode::Paths also work on wasm / itch.io? Is it planned (or already the case) that meta files are usable on the web?

Yup this works on wasm / thats why it exists in the first place (as this workaround isn't needed if you aren't deploying to wasm / trying to prevent logspam / trying to avoid errors on platforms like itch that return non 404 error messages).

@cart
Copy link
Member Author

cart commented Nov 22, 2023

Per the discussion above, I have made this non breaking by switching the meta check AssetPlugin field to a resource. I've also added a new AssetServer constructor to avoid breaking changes to the existing constructor.

@P13L0v3r
Copy link

P13L0v3r commented Nov 27, 2023

Can you provide an example for how someone would use this?

Edit: I see this is an update to bevy's asset system so it should be the same as making any other changes to the plug-in, correct?

@cart
Copy link
Member Author

cart commented Nov 27, 2023

@P13L0v3r I've just added a usage example to the description. Hope that helps!

@cart cart added this pull request to the merge queue Nov 27, 2023
Merged via the queue into bevyengine:main with commit fa903a1 Nov 27, 2023
22 checks passed
cart added a commit that referenced this pull request Nov 30, 2023
# Objective

Fixes #10157

## Solution

Add `AssetMetaCheck` resource which can configure when/if asset meta
files will be read:

```rust
app
  // Never attempts to look up meta files. The default meta configuration will be used for each asset.
  .insert_resource(AssetMetaCheck::Never)
  .add_plugins(DefaultPlugins)
```


This serves as a band-aid fix for the issue with wasm's
`HttpWasmAssetReader` creating a bunch of requests for non-existent
meta, which can slow down asset loading (by waiting for the 404
response) and creates a bunch of noise in the logs. This also provides a
band-aid fix for the more serious issue of itch.io deployments returning
403 responses, which results in full failure of asset loads.

If users don't want to include meta files for all deployed assets for
web builds, and they aren't using meta files at all, they should set
this to `AssetMetaCheck::Never`.

If users do want to include meta files for specific assets, they can use
`AssetMetaCheck::Paths`, which will only look up meta for those paths.

Currently, this defaults to `AssetMetaCheck::Always`, which makes this
fully non-breaking for the `0.12.1` release. _**However it _is_ worth
discussing making this `AssetMetaCheck::Never` by default**_, given that
I doubt most people are using meta files without the Asset Processor
enabled. This would be a breaking change, but it would make WASM / Itch
deployments work by default, which is a pretty big win imo. The downside
is that people using meta files _without_ processing would need to
manually enable `AssetMetaCheck::Always`, which is also suboptimal.

When in `AssetMetaCheck::Processed`, the meta check resource is ignored,
as processing requires asset meta files to function.

In general, I don't love adding this knob as things should ideally "just
work" in all cases. But this is the reality of the current situation.

---

## Changelog

- Added `AssetMetaCheck` resource, which can configure when/if asset
meta files will be read
@miketwenty1
Copy link

For anyone else who is still getting 404's after this change, make sure you put app.insert_resource(AssetMetaCheck::Never) before the default plugins when configuring the app like in the description example. I didn't realize ordering mattered 🙃

@alice-i-cecile
Copy link
Member

#1255 strikes again :(

maaku added a commit to maaku/atomCAD that referenced this pull request Jan 20, 2024
As documented upstream in bevyengine/bevy#10623, bevy 0.12.1 can result in black screens on wasm if meta files are not available.  Setting the AssetMetaMode to AssetMetaChck::Never is a band-aid fix for this issue.
maaku added a commit to maaku/atomCAD that referenced this pull request Jan 20, 2024
As documented upstream in bevyengine/bevy#10623, bevy 0.12.1 can result in black screens on wasm if meta files are not available.  Setting the AssetMetaMode to AssetMetaChck::Never is a band-aid fix for this issue.
maaku added a commit to atomCAD/atomCAD that referenced this pull request Jan 21, 2024
As documented upstream in bevyengine/bevy#10623, bevy 0.12.1 can result in black screens on wasm if meta files are not available.  Setting the AssetMetaMode to AssetMetaChck::Never is a band-aid fix for this issue.
@janhohenheim
Copy link
Member

As someone who just lost quite a bit of time to this while trying to upload to itch.io, I second the suggestion to make this the default for 0.13 (if it isn't already).
The main issue for me was that the fix is currently hard to google. In the end, I had to search for "png.meta" in the Discord to find this PR :/

will-hart added a commit to will-hart/ld55-necromatcher that referenced this pull request Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset loading on the Web produces many 404 errors for .meta files
9 participants