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

gltf loader: add a temporary loader without compression support #9426

Closed

Conversation

mockersf
Copy link
Member

Objective

  • When loading gltf files during app creation (for example using a FromWorld impl and adding that as a resource), no loader was found.
  • As the gltf loader can load compressed formats, it needs to know what the GPU supports so it's not available at app creation time.

Solution

  • Add a temporary gltf loader without compressed format support. Once the GPU is available, it will be overwritten by a gltf loader with the correct compressed format support
  • This is needed for the 0.11.1 to still be able to load gltf files during app creation after Fix panic whilst loading UASTC encoded ktx2 textures #9158

@mockersf mockersf added A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds labels Aug 12, 2023
@mockersf mockersf added this to the 0.11.1 milestone Aug 12, 2023
@@ -53,6 +59,7 @@ impl Plugin for GltfPlugin {

None => CompressedImageFormats::NONE,
};
// Overwrite the gltf loader with support for compressed formats.
Copy link
Contributor

@Shatur Shatur Aug 12, 2023

Choose a reason for hiding this comment

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

Maybe use if let above and insert only if GPU is found?

@nicopap nicopap self-requested a review August 13, 2023 07:50
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

It's still a footgun. Suppose someone loads a compressed format in FromWorld?

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Aug 14, 2023
@cart
Copy link
Member

cart commented Aug 14, 2023

Closing in favor of #9429

@cart cart closed this Aug 14, 2023
github-merge-queue bot pushed a commit that referenced this pull request Aug 14, 2023
# Objective

- When loading gltf files during app creation (for example using a
FromWorld impl and adding that as a resource), no loader was found.
- As the gltf loader can load compressed formats, it needs to know what
the GPU supports so it's not available at app creation time.

## Solution

alternative to #9426

- add functionality to preregister the loader. loading assets with
matching extensions will block until a real loader is registered.
- preregister "gltf" and "glb".
- prereigster image formats.

the way this is set up, if a set of extensions are all registered with a
single preregistration call, then later a loader is added that matches
some of the extensions, assets using the remaining extensions will then
fail. i think that should work well for image formats that we don't know
are supported until later.
cart pushed a commit that referenced this pull request Aug 14, 2023
# Objective

- When loading gltf files during app creation (for example using a
FromWorld impl and adding that as a resource), no loader was found.
- As the gltf loader can load compressed formats, it needs to know what
the GPU supports so it's not available at app creation time.

## Solution

alternative to #9426

- add functionality to preregister the loader. loading assets with
matching extensions will block until a real loader is registered.
- preregister "gltf" and "glb".
- prereigster image formats.

the way this is set up, if a set of extensions are all registered with a
single preregistration call, then later a loader is added that matches
some of the extensions, assets using the remaining extensions will then
fail. i think that should work well for image formats that we don't know
are supported until later.
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 A-Rendering Drawing game state to the screen X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants