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.

Sign up for GitHub

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

Jump to bottom

Fix panic whilst loading UASTC encoded ktx2 textures #9158

Merged
merged 4 commits into from
Jul 23, 2023
Merged

Fix panic whilst loading UASTC encoded ktx2 textures #9158

merged 4 commits into from
Jul 23, 2023

Conversation

66OJ66
Copy link
Contributor

@66OJ66 66OJ66 commented Jul 14, 2023

Objective

Fixes #9121

Context:

  • ImageTextureLoader depends on RenderDevice to work out which compressed image formats it can support
  • RenderDevice is initialised by RenderPlugin
  • Webgpu support #8336 made RenderPlugin initialisation async
  • This caused RenderDevice to be missing at the time of ImageTextureLoader initialisation, which in turn meant UASTC encoded ktx2 textures were being converted to unsupported formats, and thus caused panics

Solution

  • Delay ImageTextureLoader initialisation

Changelog

  • Moved ImageTextureLoader initialisation from ImagePlugin::build() to ImagePlugin::finish()
  • Default to CompressedImageFormats::NONE if RenderDevice resource is missing

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@nicopap nicopap added A-Rendering Drawing game state to the screen P-Crash A sudden unexpected crash labels Jul 14, 2023
@nicopap nicopap added this to the 0.11.1 milestone Jul 14, 2023
@nicopap
Copy link
Contributor

nicopap commented Jul 14, 2023

This doesn't fix it for me. I tested for a file that works on 5da8af7 (the commit before #8336). The file panics both on main (05a35f6) and this PR

@nicopap
Copy link
Contributor

nicopap commented Jul 14, 2023

Maybe the GLTF loader also needs to be adjusted?

@66OJ66
Copy link
Contributor Author

66OJ66 commented Jul 14, 2023

Thanks for flagging @nicopap!
I've applied the same change to the GltfLoader as well - could you check if your file loads successfully now?

nicopap
nicopap approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@nicopap nicopap left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! It works now. Thank you for the fix

@nicopap nicopap requested a review from mockersf July 18, 2023 07:00
@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2023
@mockersf mockersf added this pull request to the merge queue Jul 23, 2023
Merged via the queue into bevyengine:main with commit 5b0e6a5 Jul 23, 2023
cart pushed a commit that referenced this pull request Aug 10, 2023
@66OJ66 @cart
Fix panic whilst loading UASTC encoded ktx2 textures (#9158)
c6d7740 
# Objective

Fixes #9121

Context:
- `ImageTextureLoader` depends on `RenderDevice` to work out which
compressed image formats it can support
- `RenderDevice` is initialised by `RenderPlugin`
- #8336 made `RenderPlugin`
initialisation async
- This caused `RenderDevice` to be missing at the time of
`ImageTextureLoader` initialisation, which in turn meant UASTC encoded
ktx2 textures were being converted to unsupported formats, and thus
caused panics

## Solution

- Delay `ImageTextureLoader` initialisation

---

## Changelog

- Moved `ImageTextureLoader` initialisation from `ImagePlugin::build()`
to `ImagePlugin::finish()`
- Default to `CompressedImageFormats::NONE` if `RenderDevice` resource
is missing

---------

Co-authored-by: 66OJ66 <hi0obxud@anonaddy.me>
@mockersf mockersf mentioned this pull request Aug 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@nicopap nicopap nicopap approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen P-Crash A sudden unexpected crash S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.11.1
Development

Successfully merging this pull request may close these issues.

Panic whilst loading ktx2 file
3 participants
@66OJ66 @nicopap @mockersf