Skip to content

Make Skybox not break rendering if its image is default or 2D.#23689

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
kpreid:skybox-validation
Apr 11, 2026
Merged

Make Skybox not break rendering if its image is default or 2D.#23689
alice-i-cecile merged 1 commit intobevyengine:mainfrom
kpreid:skybox-validation

Conversation

@kpreid
Copy link
Copy Markdown
Contributor

@kpreid kpreid commented Apr 6, 2026

Objective

Solution

If the image is detected to be invalid, have prepare_skybox_bind_groups ignore it (and warn once).

Testing

Manually tested that examples/3d/skybox.rs works if the initial skybox has the default image.

If there’s a good way to programmatically test things like this, let me know — I didn’t see any similar tests to emulate.

@alice-i-cecile alice-i-cecile added this to the 0.19 milestone Apr 6, 2026
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen P-Crash A sudden unexpected crash S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 6, 2026
@github-project-automation github-project-automation bot moved this to Needs SME Triage in Rendering Apr 6, 2026
@alice-i-cecile alice-i-cecile added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Apr 6, 2026
// catch here, but this is a common mistake (passing a 2D image to `Skybox`) and
// also the result of `Skybox::default()` and not overriding the `image`.
warn_once!(
"skybox {entity}'s image {image:?} has texture view dimension \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"skybox {entity}'s image {image:?} has texture view dimension \
"Skybox {entity}'s image {image:?} has texture view dimension \

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I’m happy to change this if required, but is there a project style guide that specifies this?

By default, for all error, panic, and warning messages, I follow Rust API Guidelines — which is specific to errors, but I believe consistency is a good principle here: “The error message given by the Display representation of an error type should be lowercase without trailing punctuation”.

Copy link
Copy Markdown
Contributor

@jasmine-nominal jasmine-nominal left a comment

Choose a reason for hiding this comment

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

Lgtm minus one nit

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 10, 2026
@kpreid kpreid force-pushed the skybox-validation branch from beece36 to 12d02a3 Compare April 11, 2026 00:37
@kpreid
Copy link
Copy Markdown
Contributor Author

kpreid commented Apr 11, 2026

Rebased to fix conflict with #23691. Note that I had to reorganize the code substantially (to maintain both reasonable syntax and #23691's feature of removing the SkyboxBindGroup component), so you may wish to re-review. It does the same thing but there is a separate function now.

Copy link
Copy Markdown
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

Lets merge, looks good

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 11, 2026
Merged via the queue into bevyengine:main with commit 5330b01 Apr 11, 2026
38 checks passed
@github-project-automation github-project-automation bot moved this from Needs SME Triage to Done in Rendering Apr 11, 2026
@kpreid kpreid deleted the skybox-validation branch April 11, 2026 21:36
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants