-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
UI render layers #5414
UI render layers #5414
Conversation
15a248b
to
2d73c50
Compare
This enables having different UI per camera. With bevyengine#5225, this enables having different interactive UIs per window. Although, to properly complete this, the focus system will need to account for RenderLayer of UI nodes.
2d73c50
to
17153b6
Compare
/// This component is inserted into the render world in the | ||
/// [`extract_default_ui_camera_view`] system. | ||
/// | ||
/// The component is attached to the "actual" viewport's camera. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, this is making me second guess removing explicitly initialized UI cameras. Seems like the right design if we're not changing that though.
} | ||
for (mut visibility, node_layers) in &mut nodes { | ||
if config.ui_render_layers.intersects(node_layers) { | ||
visibility.set_visible_in_view(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this override all other forms of computed visibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does, which will be an issue with inherited visibility. Good catch, need to update the code.
I don't like this, though. The default visibility check is buggy on UI entities since it cannot account for UiCameraConfig
, but it does the inherited visibility thing.
In practice, that means the checks in check_visibility
needs to be completely discarded and recomputed here :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, why not merge this together with those checks in some way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would create a circular dependency between bevy_render
and bevy_ui
(need access to the UiCameraConfig
and Node
components, and you wouldn't want bevy_render
to depend on bevy_ui
regardless).
An alternative design where the UI camera is spawned as any other camera in the app world is possible, but the fact that it's a sort of special camera that shares a viewport with another one makes it awkward. I've given motivation in the opening comment of #5252.
Hot take: should just remove Visibility
and ComputedVisibility
from UI entities. Bonus: avoids confusion with display: None
:D
But I feel like this is a discussion that should be had in a different PR or an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, right :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely having some regrets about moving to implicit UI cameras seeing the hoops this PR (and its sibling) have to jump through. That said, I'm largely happy with the changes here otherwise, and think that this is an important feature.
This is adding a "gotcha" with layout. Layout is not looking at render layers, and it shouldn't because cameras can use masks as they want, but in the basic use case of one camera => one layer, ui on each camera will impact the layout of each other |
I think it's a major limitation, that I'm not sure how to address. My approach was simply to ignore it, because otherwise I'd have to argue why I chose an approach over another. 😅 |
I think the best way of handling it would be to support multiple UI roots with an explicit component that also encodes the settings for the whole tree under the root. This would be one place to specify the |
After reviewing the PR, I don't think this is the right approach.
Considering we are now doing camera driven rendering with implicit ui cameras, it would make more sense to me if every ui camera was also a ui root node. Layout only really makes sense in the context of a "document", which in this case is the camera's rendertarget.
|
I think I'll close this PR and try a different approach. I agree with most of the objections. I'm looking forward to see how your mouse picking proposal works with render priority and A few notes though:
But if the UI camera is implicit, then there is no such thing as a "UI camera", what do you mean by that? Maybe adding a field to I personally think that adding by default a UI to every cameras is a bad idea. I would be in favor of making explicit UI in render targets. |
Good catch. I think it would make sense to make the ui camera explicit, and add the ui nodes to it. |
I've opened #5570 which relates to this conversation |
I wrote it before reading about this one, but in my recent draft #5892 I take the reverse approach of mapping UI root nodes (so only UI nodes that are at the root of a UI tree) to a camera entity instead. This has the benefit of enabling any number of UI trees to exist in a single camera and making sure that a UI tree is always constrained within that camera's target and viewport. One limitation is that a single UI tree can't be rendered to multiple cameras, but that sounds like a complex use-case to me that you could work around if you really wanted to by having multiple of the same UI tree or rendering the UI to a camera that renders to a texture and presenting that elsewhere. If you don't add that component to your UI root, the assumption would either be that you only have one camera and that you want the UI to fill it, or that you just want to render a UI on the full main window and don't really care about specific cameras for UI. This seems to make some sense, but I've yet to write the rendering part so I'm not sure if it'll work in practice. If you're up for it, please let me know what you think! |
@oceantume I'm on board with your implementation. I don't see any realistic blocker, so it's probably good to go. I'd really like to help you get it working, but I'm a bit short on time. Edit: I have one worry, it's knowing in "which" UI tree a given UI node is. This will always require navigating up to the root. A costly operation if it is a common occurence |
I have an idea for making this a little better. I plan to alter the |
Objective
This enables having different UI per camera. With #5225, this enables
having different interactive UIs per window. Although, to properly
complete this, the focus system will need to account for RenderLayer of
UI nodes.
Solution
render_layers: RenderLayers
field to UI bundlesui_render_layers
field toUiCameraConfig
RenderLayers
in the UI render codeChangelog
RenderLayers
support to UI nodes, you now can display different UIs on different cameras.