-
Notifications
You must be signed in to change notification settings - Fork 215
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
Use renderArea instead of full window when compiling RenderGraph #1331
base: master
Are you sure you want to change the base?
Use renderArea instead of full window when compiling RenderGraph #1331
Conversation
I have done a quick code review of the change and it looks cleaner than the original, but I need to look into whether RenderGraph::renderArea is consistently setup to reflect the extents. |
This looks like it might be something that #1268 fixes, too (as it will change the viewport when the record traversal encounters another view). If not, then it's something I'll need to account for in that PR as this fix will stop working. It doesn't turn the Either way, it'd be helpful if @theodoregoetz could try the PR I linked. |
I rebased the |
Sorry for the delayed response - most of my street had no electricity or internet yesterday. How are you setting things up? Is it just manually-created rendergraphs being added as children of a view with a different extent? The original PR description suggests that the view's camera has a different viewport state to the window/framebuffer and the rendergraphs match the view, but I'd expect my branch to handle that, so I'm worried that as well as not respecting the rendergraph extents, it might have a bug with view extents. |
@AnyOldName3 You are right that we are careful to match the rendergraph's renderArea to the scissor(s) of the View. However, I suppose in general we would only have to ensure that all scissors are only within the rendergraph's renderArea. We actually go through a lot of effort during compile and record to adjust views and views within views regarding the ViewportState and I'm wondering if maybe it would be useful to take ViewportState out of the Camera and have it be it's own group node - I almost feel the same way about the projection and view transformations of the Camera since we have layers that share the projection and others that don't and the same with the view. I suppose this could be done with Transform and StateGroup nodes? P.S. We were without power for a while too! There was a "bomb cyclone storm" that hit the Seattle area pretty hard that affected us. Hope you are doing alright! |
14a3be1
to
5026aa2
Compare
The point of the camera class is to encapsulate things like the viewport - most users would want to be able to control all the things the camera controls at the same time, and you can nest cameras to have other passes (e.g. this is what's done to render view-specific shadow maps). Using more cameras might make what you're trying to do easier rather than fighting to avoid them. The Vulkan made easy tagline is assuming you're doing everything the easy way rather than defaulting to the lower-level hard way. If you could come up with some example code where my PR branch doesn't work even when the rendergraph's extents match the view's, that would be helpful. I see it (or something derived from it) as inevitable, so I'd rather it actually worked. Our powercut was just because the workmen digging up the road to replace some pipes also dug through some wires, so nothing as exciting as a cyclone. |
5026aa2
to
322f2ad
Compare
Use renderArea of RenderGraph instead of the full window/framebuffer extent when creating the compile-context default viewportstate Nominally, Views are above RenderGraphs in VSG scene graphs, however when a View that takes up a subset of the window/framebuffer extent contains individual RenderGraphs, the recorded default renderArea (viewportstate) was incorrect and resulted in the scene being drawn to the entire window area, but scissored to the render graphs renderArea. This change ensures that the default viewportstate is consistent with the rendergraph being compiled.
322f2ad
to
eb6d1b9
Compare
We found that we needed to consider the View's viewport in coordination with the render graph's render area (which must be consistent with the View's scissor). To this end, we now save off the current View into the context during record. This then is used to set the viewport, keeping the scissor the same as the render graph's render area. This works quite well for us, but we only ever use a single viewport/scissor in the viewport states - these are vectors and I'm not sure how to handle that. Also, I'll try to work up an example that demonstrates rendergraphs both above and below views within the scenegraph, with scissors that are not the same as the viewports in the viewport states involved. |
The reason you'd set multiple viewports would be for layered rendering, e.g. rendering the scene to multiple faces of a cubemap, multiple eyes of a VR headset, or multiple shadow map cascades in one pass. I think it's generally accepted that using multiview is typically less hassle, though. |
Use renderArea of RenderGraph instead of the full window/framebuffer extent when creating the compile-context default viewportstate
Nominally, Views are above RenderGraphs in VSG scene graphs, however when a View that takes up a subset of the window/framebuffer extent contains individual RenderGraphs, the recorded default renderArea (viewportstate) was incorrect and resulted in the scene being drawn to the entire window area, but scissored to the render graphs renderArea. This change ensures that the default viewportstate is consistent with the rendergraph being compiled.
Type of change
This is a breaking change, but I believe it resolves a long-standing bug in the Compilation of the scene.
How Has This Been Tested?
I have a View with non-zero offset and a smaller extent than the whole window. This View contains multiple RenderGraphs which is required because I need to issue a PipelineBarrier between them.
Checklist:
Note: before I go ahead and work up a VSG example to validate this change, I'd like some feedback from @robertosfield .