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

Make UiNode render pass respect render layers. #8332

Closed
wants to merge 1 commit into from

Conversation

jdm
Copy link
Contributor

@jdm jdm commented Apr 8, 2023

Objective

Ensures that UiNodes are only rendered to views with matching render layer configurations. Fixes #6069.

Solution

When UiNodes are queued, they are queued indiscriminately for each extracted view that exists. Since render layers can be a component of both the camera as well as the UiNode, we need to propagate the layer information in two ways:

  • by adding render layer components to those extracted view entities so they can be queried later
  • by adding render layer data to the UiBatches, so a batch of nodes all reference the same layer data

With this data propagated, we can perform a layer intersection check when queueing UiNodes against a particular view.


Changelog

Fixed

UI components only render in views with matching render layers.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2023

Welcome, new contributor!

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

@jdm
Copy link
Contributor Author

jdm commented Apr 8, 2023

These changes support both scenarios:

  • UI components with no render layer, camera with explicit render layer
  • UI components with render layer, camera with explicit render layer

Screen Shot 2023-04-08 at 4 18 02 PM

Screen Shot 2023-04-08 at 4 15 03 PM

@jdm jdm force-pushed the uinode-render-layers branch from daf8e8c to 73e8a27 Compare April 8, 2023 20:36
@IceSentry IceSentry added A-UI Graphical user interfaces, styles, layouts, and widgets A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible C-Bug An unexpected or incorrect behavior and removed C-Feature A new feature, making something new possible labels Apr 8, 2023
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

This moves the render layer checks into the render app. Would it be possible to just check RenderLayers during extraction and just not extract the nodes that aren't visible?

@jdm
Copy link
Contributor Author

jdm commented Apr 8, 2023

This moves the render layer checks into the render app. Would it be possible to just check RenderLayers during extraction and just not extract the nodes that aren't visible?

I like this idea but I don't see how to do it. Does extraction run per-view? Right now there's no interaction with the view at all; how would I determine which one is the correct one to compare against?

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 8, 2023
@nicopap
Copy link
Contributor

nicopap commented Apr 9, 2023

Have you seen #5414? This seems fairly similar. I closed it because UI nodes in the same layout would lead to very confusing results. This seem to exhibit the same behaviors.

Honestly, I don't think the confusion with different RenderLayers in same layout is a reason to block this PR, but it might be worth considering it since it was a motivation in closing previous attempts at this.

@jdm
Copy link
Contributor Author

jdm commented Apr 9, 2023

I have not seen that before. I will take some time to review the changes and discussion in that PR.

@1z-z1
Copy link

1z-z1 commented May 20, 2023

As someone who looking to pass different UI components and information to different viewports, this feature would be helpful.

@ickshonpe
Copy link
Contributor

ickshonpe commented Jun 23, 2023

Having to set the render layers for each ui node individually doesn't seem good.

Maybe render layers could be added to UiStack instead of querying for the RenderLayers component in the extraction functions. Then when ui_stack_system walks the node tree if it finds a node with a RenderLayers component it could set all the descendants to those render layers as well.

@jdm
Copy link
Contributor Author

jdm commented Sep 11, 2023

Having read through #5414, I find the argument compelling that the interaction of layout with UI spread across multiple render layers is confusing. I agree that a more satisfying solution would be supporting multiple UI roots that could exist on separate layers instead (#5622).

@alice-i-cecile
Copy link
Member

Having read through #5414, I find the argument compelling that the interaction of layout with UI spread across multiple render layers is confusing. I agree that a more satisfying solution would be supporting multiple UI roots that could exist on separate layers instead (#5622).

Given this, I'm going to close out this PR. We need a fix for this sort of thing eventually, but I don't think this is it.

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 A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI components not respecting RenderLayers
8 participants