close
Skip to content

[Merged by Bors] - Glb textures should use bevy_render to load images#1454

Closed
mockersf wants to merge 4 commits into
bevyengine:mainfrom
mockersf:glb-textures
Closed

[Merged by Bors] - Glb textures should use bevy_render to load images#1454
mockersf wants to merge 4 commits into
bevyengine:mainfrom
mockersf:glb-textures

Conversation

@mockersf
Copy link
Copy Markdown
Member

@mockersf mockersf commented Feb 16, 2021

Fixes #1396

Screenshot 2021-02-16 at 02 24 01

Issue was that, when loading an image directly from its bytes in the binary glb file, it didn't follow the same flow as when loaded as a texture file. This PR removes the dependency to image from bevy_gltf, and load the image using bevy_render in all cases. I also added support for more mime types while there.

Screenshot 2021-02-16 at 02 44 56

@mockersf
Copy link
Copy Markdown
Member Author

mockersf commented Feb 16, 2021

Fails to build on wasm because module to load images in bevy_render is behind features...

Currently, it works as a side effect because, when enabling feature jpeg, it enables features image/jpeg through bevy_render, so it's also available to bevy_gltf

Possible fixes:

  • remove the check for those features on the module in bevy_render, the control for enabled formats will still be done in crate image. it may increate build time a little for someone who doesn't want any image format enabled at all
  • add similar features to bevy_gltf for image formats. this increases somewhat the features complexity available in bevy

What do you think?

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior help wanted labels Feb 17, 2021
@mockersf
Copy link
Copy Markdown
Member Author

after doing some tests, I didn't notice a slowdown of compilation time by removing the feature check so I went this way

Base automatically changed from master to main February 19, 2021 20:44
@cart
Copy link
Copy Markdown
Member

cart commented Mar 3, 2021

Yeah I think you made the right call for "image features". Looking now!

Copy link
Copy Markdown
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This is a good change ❤️

Just one small point of discussion

}

/// Load a bytes buffer in a [`Texture`], according to type `image_type`, using the `image` crate`
pub fn buffer_to_texture(buffer: &[u8], image_type: ImageType) -> Result<Texture, TextureError> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than being a floating function, maybe this should be added to Texture? Ex:

Texture::from_buffer(buffer: &[u8], image_type: ImageType) -> Result<Texture, TextureError> { ... }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok for me!

@cart
Copy link
Copy Markdown
Member

cart commented Mar 3, 2021

bors r+

bors Bot pushed a commit that referenced this pull request Mar 3, 2021
Fixes #1396 

<img width="1392" alt="Screenshot 2021-02-16 at 02 24 01" src="https://user-images.githubusercontent.com/8672791/108011774-1b991a80-7008-11eb-979e-6ebfc51fba3c.png">

Issue was that, when loading an image directly from its bytes in the binary glb file, it didn't follow the same flow as when loaded as a texture file. This PR removes the dependency to `image` from `bevy_gltf`, and load the image using `bevy_render` in all cases. I also added support for more mime types while there.

<img width="1392" alt="Screenshot 2021-02-16 at 02 44 56" src="https://user-images.githubusercontent.com/8672791/108011915-674bc400-7008-11eb-83d4-ded96a38919b.png">
@bors
Copy link
Copy Markdown

bors Bot commented Mar 3, 2021

@bors bors Bot changed the title Glb textures should use bevy_render to load images [Merged by Bors] - Glb textures should use bevy_render to load images Mar 3, 2021
@bors bors Bot closed this Mar 3, 2021
@mockersf mockersf deleted the glb-textures branch April 27, 2021 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GLTF: different rendering when loaded from GLB vs. GLTF/ascii

3 participants