Fix crash when loading textures from gltf. #7005

AllenDang · 2022-12-22T06:24:27Z

Refine image texture conversion to fix crash when loading specified textures from gltf.

Objective

Solution

Unify format conversion by using ConvertBuffer from image crate and set correct format.

…exture format from gltf

AllenDang · 2022-12-26T02:58:26Z

@nicopap I've tested it according all the models I have without problem, what now? Waiting for more test result or code review?

nicopap

Why remove support

nicopap · 2022-12-26T08:04:22Z

crates/bevy_render/src/texture/image_texture_conversion.rs

-
-                data = i.into_raw();
-            }
-            DynamicImage::ImageLuma16(i) => {


Could you explain why you removed support for the following image formats: Luma16, LumaA16, Rgb16, Rgba16, and 32F variants?

Sorry for not providing enough context and information at the first time.

I'm creating a 3D asset management tool based on bevy, it should be able to load and preview gltf 2.0 model at runtime, so the stability and compatibility comes to first priority.

I've tested the compatibility with all kinds of gltf, including all models from (https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0) and models I collected which will cause the crash issue #7005, and current code will provide maximum stability and compatibility, means all the textures could be loaded and rendered without any problem.

I think, we could build a solid ground without any crash at first, and extend the compatibility case-by-case in future, rather than support as much as possible.

What about custom shaders? Bevy users might want to load any image formats for their own shaders. Even if it doesn't work with the default PBR shader

janhohenheim · 2023-02-10T02:42:46Z

Any updates on this? If I understood @AllenDang's answer correctly, the version he proposes only keeps de jure support for the formats Bevy de facto supports right now. That seems reasonable to me, is it not?
For context, I stumbled upon this PR when wondering why my normal maps on GLTF imports kept crashing Bevy.

nicopap · 2023-02-11T08:34:31Z

@janhohenheim Could you drop a review? The changes are quite trivial and if you understand the implications of the bug, your input will be greatly appreciated. I personally barely understand them and feel like I don't have the authority to review it.

nicopap

To me it seems reasonable to remove handling of image formats that invariably crash bevy. I'm on edge because of Chesterton's fence. A lot of removed code seem like they do duplicate or useless work, but who knows if it actually serves a purpose?

I'm worried about flexibility though. Maybe some texture formats do not work with PBR, but they might work with user-defined shaders.

nicopap · 2023-02-11T08:38:21Z

crates/bevy_render/src/texture/image_texture_conversion.rs

            }
-            // DynamicImage is now non exhaustive, catch future variants and convert them


Why remove this line? This is still true as far as I am aware.

nicopap · 2023-02-11T08:39:17Z

crates/bevy_render/src/texture/image_texture_conversion.rs

-
-                data = i.into_raw();
-            }
-            DynamicImage::ImageLuma16(i) => {


What about custom shaders? Bevy users might want to load any image formats for their own shaders. Even if it doesn't work with the default PBR shader

janhohenheim · 2023-02-15T00:01:29Z

@nicopap sure, I can review this tomorrow :)

AllenDang · 2023-02-26T01:45:48Z

Any progress?

janhohenheim · 2023-02-26T02:02:16Z

@AllenDang thanks for the reminder, I forgot to get back to you!
The argument about shaders @nicopap brought up seems quite important. From my own experience, the data types you removed really do only crash on StandardMaterials and not when just loading them as assets in general, right? Could you verify what happens when you simply pass one of them to a shader? You should be able to easily adapt https://github.com/bevyengine/bevy/blob/main/examples/shader/shader_material.rs. Bonus points if you test this on Wasm as well.
If I am right about this, I also advise to keep the data types but add a little doc comment so that others don't need to scratch their heads in vain in the future when stuff crashes. Something like the following perhaps:

/// "This type is currently not compatible with [`StandardMaterial`]"

In any case, thanks for fixing the PR so far and fixing some crashes I had :)

AllenDang · 2023-02-26T02:28:28Z

@janhohenheim The crash happens on a critical and major workflow about 3D game making.

I got a glb file, I previewed it via gltf's official viewer, everything seems fine.
I import it into bevy, boom, crash occurred.

If some image types are known incompatible with StandardMaterial, which will lead to a runtime crash, why are we still trying to use a universal image loader? Anyone could add a new image type for some reason and introduce a runtime crash at anytime.

My suggestion is, not just add some comments, but to create a new image loader only for StandardMaterial.

janhohenheim · 2023-02-26T02:52:47Z

@AllenDang I see what you mean. How would you go about passing the data to other shaders that use these formats?

AllenDang · 2023-02-26T14:05:33Z

@janhohenheim After a double thoughts, I think we might need to go back to check the root cause about the crash... I'm not familiar about wlsl and web gpu, do you have any clue?

janhohenheim · 2023-02-26T15:29:04Z

I'm not knowledgeable enough, sorry. Maybe a rendering expert can help out: summoning @superdump, @robtfm

nicopap · 2023-02-26T16:46:24Z

This surfaced recently:

wgpu validation error when using 16bits precision PNGs #4532

A discussion with a user resulted in finding out that AsBindGroup's derive macro attributes #[sampler] and #[texture] can only bind to RGBA textures. and you would need to derive AsBindGroup manually to (so it's hard to make custom materials with custom shaders that use a different format from RGBA)

robtfm · 2023-02-27T00:33:21Z

this seems to work:

diff --git a/crates/bevy_render/src/texture/image_texture_conversion.rs b/crates/bevy_render/src/texture/image_texture_conversion.rs
index 71eeff23e..5c372995d 100644
--- a/crates/bevy_render/src/texture/image_texture_conversion.rs
+++ b/crates/bevy_render/src/texture/image_texture_conversion.rs
@@ -82,7 +82,7 @@ impl Image {
             DynamicImage::ImageRgb16(image) => {
                 width = image.width();
                 height = image.height();
-                format = TextureFormat::Rgba16Uint;
+                format = TextureFormat::Rgba16Unorm;
 
                 let mut local_data =
                     Vec::with_capacity(width as usize * height as usize * format.pixel_size());
@@ -106,7 +106,7 @@ impl Image {
             DynamicImage::ImageRgba16(i) => {
                 width = i.width();
                 height = i.height();
-                format = TextureFormat::Rgba16Uint;
+                format = TextureFormat::Rgba16Unorm;
 
                 let raw_data = i.into_raw();

it seems like currently when loading images we choose the format somewhat arbitrarily. 8bpp formats are treated as Unorm and 16bpp formats are treated as Uint. Given that there isn't any further info avaiable at the point the image is built we need to decide whether they should be treated as Unorm [0.0, 1.0], Snorm [-1.0, 1.0], or Uint [0, 65535] by default. I think choosing Unorm is likely fine since it is consistent with the 8bpp formats. @superdump, thoughts?

Merge upstream

Merge from upstream

# Objective fix #8185, #6710 replace #7005 (closed) rgb and rgba 16 bit textures currently default to `Rgba16Uint`, the more common use is `Rgba16Unorm`, which also matches the default type of rgb8 and rgba8 textures. ## Solution Change default to `Rgba16Unorm`

…9611) # Objective fix bevyengine#8185, bevyengine#6710 replace bevyengine#7005 (closed) rgb and rgba 16 bit textures currently default to `Rgba16Uint`, the more common use is `Rgba16Unorm`, which also matches the default type of rgb8 and rgba8 textures. ## Solution Change default to `Rgba16Unorm`

Refine image texture conversion to fix crash when loading specified t…

d307007

…exture format from gltf

nicopap added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Dec 22, 2022

nicopap self-requested a review December 26, 2022 07:59

nicopap suggested changes Dec 26, 2022

View reviewed changes

nicopap suggested changes Feb 11, 2023

View reviewed changes

Add comment to state DynamicImage is now non exhaustive

8a01d35

nicopap approved these changes Feb 13, 2023

View reviewed changes

tim-blackbird mentioned this pull request Mar 23, 2023

Bevy 0.10 Res<AssetServer>::load(...) Causes Panic Given Valid Path #8185

Closed

AllenDang added 2 commits May 6, 2023 16:43

Merge pull request #1 from bevyengine/main

6677989

Merge upstream

Merge pull request #2 from bevyengine/main

95cdfc7

Merge from upstream

AllenDang closed this by deleting the head repository May 14, 2023

robtfm mentioned this pull request Aug 28, 2023

default 16bit rgb/rgba textures to unorm instead of uint #9611

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash when loading textures from gltf. #7005

Fix crash when loading textures from gltf. #7005

AllenDang commented Dec 22, 2022

AllenDang commented Dec 26, 2022

nicopap left a comment

nicopap Dec 26, 2022

AllenDang Dec 26, 2022

nicopap Feb 11, 2023

janhohenheim commented Feb 10, 2023 •

edited

Loading

nicopap commented Feb 11, 2023

nicopap left a comment

nicopap Feb 11, 2023

nicopap Feb 11, 2023

janhohenheim commented Feb 15, 2023

AllenDang commented Feb 26, 2023

janhohenheim commented Feb 26, 2023

AllenDang commented Feb 26, 2023

janhohenheim commented Feb 26, 2023

AllenDang commented Feb 26, 2023

janhohenheim commented Feb 26, 2023

nicopap commented Feb 26, 2023

robtfm commented Feb 27, 2023

		}
		// DynamicImage is now non exhaustive, catch future variants and convert them

Fix crash when loading textures from gltf. #7005

Fix crash when loading textures from gltf. #7005

Conversation

AllenDang commented Dec 22, 2022

Objective

Solution

AllenDang commented Dec 26, 2022

nicopap left a comment

Choose a reason for hiding this comment

nicopap Dec 26, 2022

Choose a reason for hiding this comment

AllenDang Dec 26, 2022

Choose a reason for hiding this comment

nicopap Feb 11, 2023

Choose a reason for hiding this comment

janhohenheim commented Feb 10, 2023 • edited Loading

nicopap commented Feb 11, 2023

nicopap left a comment

Choose a reason for hiding this comment

nicopap Feb 11, 2023

Choose a reason for hiding this comment

nicopap Feb 11, 2023

Choose a reason for hiding this comment

janhohenheim commented Feb 15, 2023

AllenDang commented Feb 26, 2023

janhohenheim commented Feb 26, 2023

AllenDang commented Feb 26, 2023

janhohenheim commented Feb 26, 2023

AllenDang commented Feb 26, 2023

janhohenheim commented Feb 26, 2023

nicopap commented Feb 26, 2023

robtfm commented Feb 27, 2023

janhohenheim commented Feb 10, 2023 •

edited

Loading