Skip to content

Make Skybox’s image optional so that Skybox::default() is valid.#23691

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

Make Skybox’s image optional so that Skybox::default() is valid.#23691
alice-i-cecile merged 1 commit intobevyengine:mainfrom
kpreid:skybox-default

Conversation

@kpreid
Copy link
Copy Markdown
Contributor

@kpreid kpreid commented Apr 6, 2026 •
edited
Loading

Objective

Solution

Skybox::default() is problematic because it contains an Image that is not a valid skybox. This change removes the Default implementation and instead provides a new() function which takes the image as a parameter (and also the brightness, which is practically required). This change makes the image field optional so that the default None renders nothing.

Things we could do instead of this:

  • Make Skybox not implement Default. I am informed this is a bad idea.
  • Create a default cubemap image for default() to use.

Testing

Ran the skybox and irradiance_volumes examples.

@IceSentry
Copy link
Copy Markdown
Contributor

IceSentry commented Apr 6, 2026

I would prefer the Option<Image> solution. I believe this will work better with bsn. I vaguely remember Default being required for bsn.

@kpreid
Copy link
Copy Markdown
Contributor Author

kpreid commented Apr 6, 2026

@IceSentry Happy to turn this PR into that instead, if I hear a more confident statement that that is the way to go.

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Apr 6, 2026

@IceSentry Happy to turn this PR into that instead, if I hear a more confident statement that that is the way to go.

Insert more confident statement that this is the way to go.

(I spent a ton of time last week writing docs for the crate and that will play much nicer.)

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 6, 2026
@github-project-automation github-project-automation bot moved this to Needs SME Triage in Rendering Apr 6, 2026
@kpreid kpreid changed the title Remove Skybox::default() and add Skybox::new(). Make Skybox’s image optional so that Skybox::default() is valid. Apr 6, 2026
@kpreid
Copy link
Copy Markdown
Contributor Author

kpreid commented Apr 6, 2026

I have updated the code to make the image field optional and continue to implement Default. Also, I made prepare_skybox_bind_groups remove the SkyboxBindGroup component again so that if the image value goes from Some to None, the rendering follows.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 6, 2026
@alice-i-cecile alice-i-cecile added this to the 0.19 milestone Apr 6, 2026
@alice-i-cecile alice-i-cecile requested a review from IceSentry April 6, 2026 19:03
@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon labels Apr 6, 2026
@IceSentry IceSentry 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 6, 2026
@kpreid
Copy link
Copy Markdown
Contributor Author

kpreid commented Apr 6, 2026

This PR is a breaking change. Should I be doing anything regarding the migration guide?

@alice-i-cecile alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 6, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Apr 6, 2026

Yep: something brief would be good. The bot will be by with advice momentarily <3

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 6, 2026
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Apr 6, 2026
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Apr 6, 2026

Sibling to #23689.

@kpreid kpreid force-pushed the skybox-default branch 2 times, most recently from 0a3c784 to 0ff7151 Compare April 7, 2026 04:00
@kpreid
Copy link
Copy Markdown
Contributor Author

kpreid commented Apr 7, 2026

Added migration guide.

@kpreid
Make Skybox’s image optional.
9f47546 
This makes `Skybox::default()` a valid no-op `Skybox` component rather
than one that contains an image that cannot be rendered as a skybox.
@kpreid
Copy link
Copy Markdown
Contributor Author

kpreid commented Apr 7, 2026

I rebased and, as a consequence, had to add #[template(OptionTemplate<HandleTemplate<Image>>)] to satisfy the new derive(FromTemplate). I hope that’s the right code.

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Apr 7, 2026

You might be able to use the new #[template(builtin)] to clean that up further :)

@kpreid
Copy link
Copy Markdown
Contributor Author

kpreid commented Apr 9, 2026

Just to confirm I understand correctly: #[template(built_in)] is from #23699, so that is only relevant to this PR if that PR is merged first.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 10, 2026
Merged via the queue into bevyengine:main with commit ec254de Apr 10, 2026
38 checks passed
@github-project-automation github-project-automation bot moved this from Needs SME Triage to Done in Rendering Apr 10, 2026
viridia pushed a commit to viridia/bevy that referenced this pull request Apr 10, 2026
@kpreid @viridia
Make Skybox’s image optional so that Skybox::default() is valid. (
555569b 
bevyengine#23691)

# Objective

- Partially addresses bevyengine#23688.
- Prevents use of `Skybox::default()` from causing errors.

## Solution

`Skybox::default()` is problematic because it contains an `Image` that
is not a valid skybox. ~~This change removes the `Default`
implementation and instead provides a `new()` function which takes the
image as a parameter (and also the brightness, which is practically
required).~~ This change makes the `image` field optional so that the
default `None` renders nothing.

Things we could do instead of this:
* Make `Skybox` not implement `Default`. I am informed this is a bad
idea.
* Create a default cubemap image for `default()` to use.

## Testing

Ran the `skybox` and `irradiance_volumes` examples.
@kpreid kpreid deleted the skybox-default branch April 11, 2026 00:37
This was referenced Apr 11, 2026
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

+1 more reviewer

@IceSentry IceSentry IceSentry approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

Rendering
Status: Done

Milestone

0.19

Development

Successfully merging this pull request may close these issues.

3 participants

@kpreid @IceSentry @alice-i-cecile