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

Camera-driven UI #10559

Merged
merged 42 commits into from
Jan 16, 2024
Merged

Camera-driven UI #10559

merged 42 commits into from
Jan 16, 2024

Conversation

bardt
Copy link
Contributor

@bardt bardt commented Nov 14, 2023

Objective

Add support for presenting each UI tree on a specific window and viewport, while making as few breaking changes as possible.

This PR is meant to resolve the following issues at once, since they're all related.

Adopted #5892 , but started over since the current codebase diverged significantly from the original PR branch. Also, I made a decision to propagate component to children instead of recursively iterating over nodes in search for the root.

Solution

Add a new optional component that can be inserted to UI root nodes and propagate to children to specify which camera it should render onto. This is then used to get the render target and the viewport for that UI tree. Since this component is optional, the default behavior should be to render onto the single camera (if only one exist) and warn of ambiguity if multiple cameras exist. This reduces the complexity for users with just one camera, while giving control in contexts where it matters.

Changelog

  • Adds TargetCamera(Entity) component to specify which camera should a node tree be rendered into. If only one camera exists, this component is optional.
  • Adds an example of rendering UI to a texture and using it as a material in a 3D world.
  • Fixes recalculation of physical viewport size when target scale factor changes. This can happen when the window is moved between displays with different DPI.
  • Changes examples to demonstrate assigning UI to different viewports and windows and make interactions in an offset viewport testable.
  • Removes UiCameraConfig. UI visibility now can be controlled via combination of explicit TargetCamera and Visibility on the root nodes.

Migration Guide

If the world contains more than one camera, insert TargetCamera(Entity) component to each UI root node, where Entity is the ID of the camera you want this specific UI tree to be rendered to. Test for any required adjustments of UI positioning and scaling.

// before
commands.spawn(Camera3dBundle { ... });
commands.spawn(NodeBundle { ... });

// after
let camera = commands.spawn(Camera3dBundle { ... }).id();
commands.spawn((TargetCamera(camera), NodeBundle { ... }));

Remove UiCameraConfig component from all cameras. If it was used to control UI visibility, insert Visibility component on UI root nodes instead.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@IceSentry IceSentry added A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible labels Nov 14, 2023
@IceSentry IceSentry self-requested a review November 14, 2023 21:01
@IceSentry
Copy link
Contributor

IceSentry commented Nov 14, 2023

I believe this PR could also close #5476. Since you could now render it to a texture and render that texture to a world space quad

@IceSentry
Copy link
Contributor

CI is failing because you need to run rustfmt

@nicoburns nicoburns added this to the 0.13 milestone Nov 15, 2023
@bardt
Copy link
Contributor Author

bardt commented Nov 15, 2023

I will clean up the examples and figure out why tests are failing in the upcoming few days.

examples/ui/button.rs Outdated Show resolved Hide resolved
@bardt
Copy link
Contributor Author

bardt commented Nov 15, 2023

I've discovered that the tests are failing due to nodes not being added to the UiSurface if there is no camera set (which is the case in most tests). Arguably, camera-driven UI can require a camera to be rendered. But this would make the changes less backward compatible, which I would prefer to avoid. I will look into a better way to default rendering target, which would allow for a camera-less rendering.

@ickshonpe
Copy link
Contributor

ickshonpe commented Nov 16, 2023

I have something I'm working on I need to finish urgently but once that's done (hopefully finished today), I'll do an in-depth review. These are changes I've really wanted for a long time.

@alice-i-cecile
Copy link
Member

this is great and seems to work well.

i suspect this pr makes UiCameraConfig pretty much obsolete? the only remaining use is if you want to disable ui on your only camera (which you can always do with Visibility anyway) ... is it worth keeping around just for that?

IMO we remove this in this PR too.

@bardt
Copy link
Contributor Author

bardt commented Jan 14, 2024

Done removing UiCameraConfig. Added a comment about it to the PR description.

Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

LGTM. i backported this to bevy 0.12 and have been using it in my work project.

i haven't thoroughly tested the scale factor changes, but the basic functionality works as intended and is super useful.

@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 Jan 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 16, 2024
Merged via the queue into bevyengine:main with commit eb9db21 Jan 16, 2024
26 checks passed
@rparrett rparrett mentioned this pull request Feb 1, 2024
@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 Feb 12, 2024
Copy link
Contributor

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.

github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2024
# Objective

- #12500 use the primary window resolution to do all its calculation.
This means bad support for multiple windows or multiple ui camera

## Solution

- Use camera driven UI (#10559)
github-merge-queue bot pushed a commit that referenced this pull request Jun 6, 2024
# Objective

- Fixes #13687 

## Solution

- Text rendering in UI is still dependent on the `PrimaryWIndow`
- implements #10559 for text rendering

There are other parts of UI that are still `PrimaryWindow` dependent, if
the changes here are OK I'll apply them everywhere.
I'm not a fan of the `EntityHashMap` here to hold the scale factors, but
it seems the quick and easy fix

## Testing

- Run example `multiple_windows` on a screen with a scale factor
different than 1, close the primary window
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible 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
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple UI roots Support window independent UI scaling Viewport Specific UI