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

Key render phases off the main world view entity, not the render world view entity. #16942

Merged
merged 24 commits into from
Jan 12, 2025

Conversation

pcwalton
Copy link
Contributor

We won't be able to retain render phases from frame to frame if the keys are unstable. It's not as simple as simply keying off the main world entity, however, because some main world entities extract to multiple render world entities. For example, directional lights extract to multiple shadow cascades, and point lights extract to one view per cubemap face. Therefore, we key off a new type, RetainedViewEntity, which contains the main entity plus a subview ID.

This is part of the preparation for retained bins.

view entity.

We won't be able to retain render phases from frame to frame if the keys
are unstable. It's not as simple as simply keying off the main world
entity, however, because some main world entities extract to multiple
render world entities. For example, directional lights extract to
multiple shadow cascades, and point lights extract to one view per
cubemap face. Therefore, we key off a new type, `RetainedViewEntity`,
which contains the main entity plus a *subview ID*.

This is part of the preparation for retained bins.
@pcwalton pcwalton added A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Performance A change motivated by improving speed, memory usage or compile times labels Dec 23, 2024
Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

Nothing gets drawn for the UI, but I'm not very familiar with the camera extraction internals and I couldn't quite unravel the problem.

Very loosely, from what I can tell:
extract_default_ui_camera_view spawns a default camera view entity for each camera which has an ExtractedView with subview_index: 1 and then creates a corresponding TransparentUi render phase.

But the extracted uinodes are given the entity id of the non-ui camera ExtractedView entity so that in queue_uinodes the non-ui view with subview_index: 0 is retrieved and then no TransparentUi phases are found because they were all keyed to subview_index: 1.

Setting the subview index to 1 in extract_cameras fixes the UI examples but that breaks everything else of course.

@pcwalton pcwalton requested a review from ickshonpe December 31, 2024 16:46
@pcwalton
Copy link
Contributor Author

This should fix the issue with the UI. The proper fix was a bit tricky and involved creating a new subgraph runner to make sure to run the UI rendering subgraph on the right render world entity.

Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

Multi-window UI isn't working, I think I found the problem though.

Otherwise everything looks good now, the different shaders etc all seem to be working correctly.

Shadows and texture slicing isn't working either.

crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved
@rparrett rparrett added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 4, 2025
Copy link
Contributor

github-actions bot commented Jan 4, 2025

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

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@IceSentry
Copy link
Contributor

It looks like there's still some issues with UI based on the example runner https://pixel-eagle.com/project/B25A040A-A980-4602-B90C-D480AB84076D?filter=PR-16942

@ickshonpe
Copy link
Contributor

It looks like there's still some issues with UI based on the example runner https://pixel-eagle.com/project/B25A040A-A980-4602-B90C-D480AB84076D?filter=PR-16942

Ah yeah you're right. Shadows and texture slices still don't work, I must have ran those examples on the wrong branch by mistake.

@ickshonpe
Copy link
Contributor

The extraction and queue functions just needed to be updated. I put in a pr on this pr with the needed changes.

@pcwalton
Copy link
Contributor Author

pcwalton commented Jan 5, 2025

Cherry picked over, thanks!

@pcwalton pcwalton requested a review from ickshonpe January 5, 2025 05:27
Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

The UI rendering is all fixed now. There is a slight performance regression from the extra indirection but I don't think it's a problem with this PR really, it's more of a sign that we need to rethink how the UI handles camera views.

@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 Jan 7, 2025
@alice-i-cecile
Copy link
Member

Let me know when this has passed an example check run and merge conflicts are resolved please :)

@pcwalton
Copy link
Contributor Author

@alice-i-cecile I kicked off a run. https://pixel-eagle.com/project/B25A040A-A980-4602-B90C-D480AB84076D?filter=PR-16942 Looks like the failures are known intermittents, so we should be good to go.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 12, 2025
Merged via the queue into bevyengine:main with commit 141b767 Jan 12, 2025
28 checks passed
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-Performance A change motivated by improving speed, memory usage or compile times 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.

5 participants