Skip to content

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

Merged
superdump merged 10 commits intobevyengine:mainfrom
anarelion:dds_mipmap_level
Feb 11, 2024
Merged

mipmap levels can be 0 and they should be interpreted as 1#11767
superdump merged 10 commits intobevyengine:mainfrom
anarelion:dds_mipmap_level

Conversation

@anarelion
Copy link
Copy Markdown
Contributor

@anarelion anarelion commented Feb 7, 2024

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
doonv
doonv reviewed Feb 7, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

alice-i-cecile commented Feb 7, 2024

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
Copy Markdown
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.

doonv
doonv reviewed Feb 7, 2024
View reviewed changes
@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
Copy Markdown
Contributor Author

anarelion commented Feb 8, 2024

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
Copy Markdown
Member

alice-i-cecile commented Feb 8, 2024

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

@anarelion
Copy link
Copy Markdown
Contributor Author

anarelion commented Feb 8, 2024

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
Copy Markdown
Contributor Author

anarelion commented Feb 8, 2024

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
Copy Markdown
Member

alice-i-cecile commented Feb 8, 2024

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
Copy Markdown
Contributor Author

anarelion commented Feb 11, 2024

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

superdump
superdump requested changes Feb 11, 2024
View reviewed changes
@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
SludgePhD
SludgePhD reviewed May 7, 2024
View reviewed changes
crates/bevy_render/src/texture/image.rs
/// 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
Copy Markdown
Contributor

@SludgePhD SludgePhD May 7, 2024

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).

@rparrett rparrett mentioned this pull request Feb 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@superdump superdump superdump approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

+3 more reviewers

@SludgePhD SludgePhD SludgePhD left review comments

@BorisBoutillier BorisBoutillier BorisBoutillier approved these changes

@doonv doonv doonv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@anarelion @alice-i-cecile @doonv @superdump @mockersf @BorisBoutillier @SludgePhD