Skip to content

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Aug 11, 2025

Objective

When using RenderTarget::Image with an image set to use in the render world only, the camera_system is unable to get its render target info and fails silently, breaking things down the line. From what I saw the view uniforms stopped updating, but there might be other things it broke as well.

Solution

At least make it fail loudly :)

Not sure if it should panic here, or return a better error enum that the caller can decide what to do with. Should all failures to update the render target info be hard errors?

Testing

Error gets printed when running the project I found the bug with

@ecoskey ecoskey added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 11, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I would prefer to return a Result rather than an Option here TBH. Programatically handling error cases is much nicer than just sprinkling in logging.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 11, 2025
@ecoskey ecoskey 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 Aug 11, 2025
@ecoskey ecoskey added this to the 0.17 milestone Aug 11, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Much nicer! Needs a migration guide though <3

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 12, 2025
Copy link
Contributor

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.

@ecoskey ecoskey 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 Aug 14, 2025
@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 Aug 14, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 14, 2025
Merged via the queue into bevyengine:main with commit fcf22bb Aug 14, 2025
34 checks passed
@ecoskey ecoskey deleted the fix/image_target branch August 21, 2025 17:43
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 M-Needs-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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants