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

mipmap levels can be 0 and they should be interpreted as 1 #11767

Merged
merged 10 commits into from
Feb 11, 2024

Conversation

anarelion
Copy link
Contributor

Objective

Loading some textures from the days of yonder give me errors cause the mipmap level is 0

Solution

Set a minimum of 1

Changelog

Make mipmap level at least 1

@mockersf mockersf added the A-Assets Load files from disk to use for things like images, models, and sounds label Feb 7, 2024
Copy link
Contributor

@doonv doonv left a comment

Choose a reason for hiding this comment

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

I feel like we should give a warning when the mipmap level is invalid instead of just silently fixing the problem.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Feb 7, 2024
@alice-i-cecile
Copy link
Member

I agree that a warning would be nice. Unsure if we can get away with a warn_once! in here without costing performance.

@doonv
Copy link
Contributor

doonv commented Feb 7, 2024

Unsure if we can get away with a warn_once! in here without costing performance.

We could make it only happen when cfg(debug_assertions) is enabled.

crates/bevy_render/src/texture/dds.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/texture/dds.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Feb 8, 2024
auto-merge was automatically disabled February 8, 2024 18:02

Head branch was pushed to by a user without write access

@anarelion
Copy link
Contributor Author

Now it should be properly done, sorry for the spam, I thought everything was ok, but 2 minute later rust analyzer came back with more problems

@alice-i-cecile
Copy link
Member

Looks like CI is still missing an import of warn_once! :)

@anarelion
Copy link
Contributor Author

Looks like CI is still missing an import of warn_once! :)

Thanks, didn't check all features. Still learning how to work with git/github and this project. I am a mercurial person

@anarelion
Copy link
Contributor Author

I don't know how to get around the duplicated versions problem, I guess there is not much I can do. Seems to be

     ┌─ /home/runner/work/bevy/bevy/Cargo.lock:342:1
    │  
342 │ ╭ raw-window-handle 0.5.2 registry+https://github.com/rust-lang/crates.io-index
343 │ │ raw-window-handle 0.6.0 registry+https://github.com/rust-lang/crates.io-index
    │ ╰─────────────────────────────────────────────────────────────────────────────^ lock entries

@alice-i-cecile
Copy link
Member

Yeah, the duplicated dependencies is not your problem: it's the result of us updating versions. Won't block the merge though.

@anarelion
Copy link
Contributor Author

Is there anything else I can do to get this merged?

@superdump superdump added this pull request to the merge queue Feb 11, 2024
Merged via the queue into bevyengine:main with commit 94ab84e Feb 11, 2024
26 of 27 checks passed
@@ -643,6 +643,7 @@ impl Image {
/// Load a bytes buffer in a [`Image`], according to type `image_type`, using the `image`
/// crate
pub fn from_buffer(
#[cfg(all(debug_assertions, feature = "dds"))] name: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a public API, so this will break bevy plugins using it if the "wrong" build configuration is enabled by the user.

tracing allows attaching this information to a Span instead, which is a better way to do this (the warn_once! will include the span's fields).

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 C-Bug An unexpected or incorrect behavior 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
Development

Successfully merging this pull request may close these issues.

7 participants