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

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

Closed
wants to merge 4 commits into from

Conversation

mockersf
Copy link
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
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
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
Member

cart commented Mar 3, 2021

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

Copy link
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
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
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
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
Contributor

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