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

Add support for multi viewports and windows to UI #5892

Closed
wants to merge 9 commits into from

Conversation

oceantume
Copy link
Contributor

@oceantume oceantume commented Sep 6, 2022

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.

Solution

Add a new optional component that can be inserted to UI root nodes 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 on the main window using the default camera. This reduces the complexity for users while giving control in contexts where it matters.

This comes with a lot of changes to the flexbox code, mainly because each node that's rendered now depends on information that's stored on its root (the scaling factor coming from the render target and viewport) which requires recursive iteration of the nodes instead of a simple query iteration.

What needs to be done to publish PR

  • Decide on the final concept for the UI root component. I feel like it could replace the existing UiCameraConfig since they're similar.
  • Add proper rendering support. Flexbox and input are mostly there, but a solution must be found to only render the extracted UI nodes on the wanted target. I will look into this, but may need some pointers since I'm not even sure if this is something that's supported at the moment.

Possible future improvements

  • Add support for RenderTarget::Image if it's even possible and a wanted feature.

Changelog

  • Adds support for selecting which window and viewport each UI tree should be presented on.

Migration Guide

UI roots (UI nodes with no parent) are now fully independent from each other in the tree. Prior to these changes, they all belonged to a common flexbox node with default Style internally. Any UI code that spawned multiple UI roots that were positioned relative to each other will need to be updated.

The simplest way to fix existing UI's in that case is to move your current root nodes to a new single root that's spawned as a transparent NodeBundle::default().

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-UI Graphical user interfaces, styles, layouts, and widgets labels Sep 6, 2022
@nicopap nicopap self-requested a review September 6, 2022 06:05
Comment on lines +24 to +29
/// Maps UI root entities to taffy root nodes.
///
/// The taffy root node is a special node that has only one child: the real root node
/// present that is stored in `entity_to_taffy`. This means that two taffy nodes are
/// maintained for each bevy_ui root node.
root_nodes: HashMap<Entity, taffy::node::Node>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a design or limitation already existing or a design choice you have made?

Is there value in doing this special-casing instead of just introducing multiple taffy-trees, one for each new root?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still think this should be redesigned to use multiple trees instead

This is not complete. It has some temporary code while issues
are being worked on. The main issue being that UI cameras seem
to clear each other or at least prevent lower-priority UI nodes
from showing somehow.
This example shows that only the UI on the highest priority camera
will ever render. Looking at output, you can clearly see that the
draw calls are happening.
This is taken from bevyengine#5721 as a temporary fix. Without it, only the UI
from the highest priority camera will actually appear.
@nicopap
Copy link
Contributor

nicopap commented Nov 2, 2022

Still need help with this? I finally have the opportunity to help out here.

One thing that I'd really like to start working on this is an example that uses the API with all the use cases you mention in the PR description, (no problem if it doesn't compile or crash at startup)

This way I can easily tell what is missing and from that, give pointers of where to look for to implement the missing features.

@oceantume
Copy link
Contributor Author

oceantume commented Nov 2, 2022

I think this was in a decent place, but it definitely needs to be updated with main. I don't remember why I merged my zindex branch into this before it was merged to bevy main, maybe it was by mistake. I may have to look into fixing that because it now shows commits from that other branch as if they were part of this PR.

@oceantume
Copy link
Contributor Author

oceantume commented Nov 2, 2022

I removed the merge with that other branch so now it's clearer what changes were made. Looking through the merge conflicts with main, there are a few things that would need to be corrected.

I think there's a conversation that needs to be resolved on how we want UI roots to relate to windows. In the current state of this PR, this is done by "linking" each UI root to a camera entity, which allows the UI code to find the target information including window, scale factor, viewport rect, etc.

I think that makes sense, but I had some concerns about having to repetitively query for the node's camera at many different points in the different UI systems (flex, render, focus) and being forced to use recursive functions with Query<&Children> everywhere to be able to keep that information around while we loop through each node hierarchy starting from its root.

Hopefully the following makes sense. I can try to explain further if not. One solution that I had in mind was to change the recently-merged UI Stack (#5877) so that it would list each UI root's stack instead of assuming that the UI is a single big stack. So the following:

#[derive(Debug, Resource, Default)]
pub struct UiStack {
    pub uinodes: Vec<Entity>,
}

Would become this:

#[derive(Debug, Resource, Default)]
pub struct UiStacks {
    pub stacks: Vec<UiStack>
}

pub struct UiStack {
    // Note that the root entity is also present in uinodes. That's because it's a logical root
    // and it's entirely possible that one of the stack's node wants a z-index that's 
    // higher or lower than its root's z-index.
    // So the utility of root here is to be able to easily query for this stack's `UiRootCamera`
    // component which is useful in many different systems (flex, rendering, focus, etc.)
    // In theory, this could also just be the camera's Entity, but root makes it more flexible
    // to create new components that can be added to a UI's root (for example a controller id)
    pub root: Entity,
    pub uinodes: Vec<Entity>,
}

Let me know what you think. If you think this makes sense and has value, it's something I'd be down to put some more time on in the next few weeks.

@nicopap
Copy link
Contributor

nicopap commented Nov 3, 2022

I think we already had this discussion about what the best API for this would look like and we concluded that a link between a camera and a root was the one we should use.

Your solution is fine and more than acceptable. Even if ideally, we could encode this in the ECS. An initial element of response (although currently too limited for resolving your concerns) is #5534. Relations would also be a potential tool for this. And it's likely that once implemented, we'll have a hard time modifying it in the future.

But at the same time, we ought to ask: How large do we expect UI trees to be? Depending on the use-case it may go from about a dozen to several hundreds or thousands! Which leads me to implementation strategies that account for UI tree size and discriminate between them.

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Sep 5, 2023
@bardt
Copy link
Contributor

bardt commented Nov 11, 2023

I recently started re-implementing ideas from this PR on the current code base. A new PR is coming some time next week, hopefully.

@bardt bardt mentioned this pull request Nov 14, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2024
# 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.

- Fixes #5622 
- Fixes #5570 
- Fixes #5621 

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.

---------

Co-authored-by: davier <bricedavier@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com>
@tygyh
Copy link
Contributor

tygyh commented Feb 1, 2024

If #10559 was a replacement for this, please close it.

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-Enhancement A new feature S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

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