Use renderArea instead of full window when compiling RenderGraph

[theodoregoetz]

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 .

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[robertosfield]

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.

[AnyOldName3]

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 renderArea into dynamic ViewportState right now, but does do so for the window/camera extents, which are what initialise renderArea. However, it's a public field, so people can change it other ways. I don't think that was the cause of the problem here, so everything should be fine.

Either way, it'd be helpful if @theodoregoetz could try the PR I linked.

[theodoregoetz]

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 renderArea into dynamic ViewportState right now, but does do so for the window/camera extents, which are what initialise renderArea. However, it's a public field, so people can change it other ways. I don't think that was the cause of the problem here, so everything should be fine.

Either way, it'd be helpful if @theodoregoetz could try the PR I linked.

I rebased the dynamic-viewports branch onto master and found it does not fix the issue I came up against. I also included my changes on top of that and the result is that it's using the full extent instead of what's given in the rendergraph's renderArea - so somehow it would have to account for this renderArea during record.

[AnyOldName3]

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.

[theodoregoetz]

@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!

[AnyOldName3]

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.

[theodoregoetz]

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.

[AnyOldName3]

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.

[robertosfield]

@theodoregoetz I'm now the process of reviewing @AnyOldName3's PR Use dynamic viewport and scissor by default #1268 and as this PR affects similar parts of VSG functionality will review this at the same. I don't know the best way to resolve all the issues touched upon by the two PR's so am keen to learn the usage cases that illustrate how the existing VSG is causing problems that need resolving.

@theodoregoetz on your post to this PR thread on the 7th December 2024 you mentioned the possibility of creating an example that illustrates the problem that this PR seeks to address. Did you write this? Are there any existing VSG examples that might be adapted for this purpose?

[theodoregoetz]

I did mean to write a minimal example but left it for high priority items. Unfortunately, we've had to maintain our own fork of VSG as a result of this. I can try to do that in the next few days. The specific use case for us is to support the rather complex multiple render pass organization required for efficient handling of order-independent-transparency (OIT) using the atomic linked-list storage buffer technique.

[robertosfield]

Sounds like we need a vsgExample to test out this usage case, not just to test this PR and other changes w.r.t like @AnyOldName3's dynamic-viewport PR. Any help you can provide with such an example would be really appreciated.

Maintaining your own fork is big benefit from the VSG being open source but praticality wise it'll mean that there could be divergence between the codebases that makes it awkward for you to benefit from all the other improvements that are happening in the VSG. So... it'd be good to get things working so you can use VSG master/releases once more.

I'd like to get this issue resolved in the next week so we can make an 1.1.11 dev release of the VSG and keep us moving towards a stable 1.2.0 release in time for Spring. As things stand though I can't merge a PR that I can't test properly so this particular PR will have to wait till we have a written a test example.

[robertosfield]

@theodoregoetz I have trying out alternate approaches to implementing dynamic viewports, and the approach I've trying out right now is to promote ViewportState to a StateCommand so that it can be push/popped from an associated State::stateStack[slot] as other StateCommands held in a StateGroup are assigned.

I've written up a little about this approach in the dynamic-viewports PR thread:

#1268 (comment)

I don't know if this new approach is going to be an elegant solution to the various problems that need addressing around this topic but will keep exploring to see if something coherent drops out.

I really want to have a unified solution that works for the issues addressed in this PR, and looking at your PR and the changes I'm making w.r.t ViewportState I'm now wondering if we have have RenderGraph "has a" ViewportState, then can be shared with a vsg::Camera's ViewportState or just used independently when required.

The ViewportState can be used directly to set up the GraphicsPipeline's during compile, or for calling vkCmdSetScissor and vkCmdSetViewportState via the newly added ViewportState::record(CommandBuffer&) method.

As ViewportState is this approach is a StateCommand you can use it yourself attaching it to a StateGroup so perhaps enabling one of your suggestions above.

Without an example that illustrates your usage case I can only speculate whether this new approach will help your usage case, and will have to ask you to try out the new development work to see if it helps you. Ideally I'd like to have a example I can test against so can shorten how much iterations we need to do to get this all pinned down robustly.

[robertosfield]

One of the changes in this PR - the cache, setting and restoring of Context::view pointer looks like a sensible to merge with VSG master and the dynamic-viewport branches so I've gone ahead and made these changes and merged them:

9386b11

This leaves the local ViewportState::create() using the RenderGraph::reanderArea changes, but I'm now thinking this code might be removable completely once we have dynamic ViewportState working fully. This could make this whole PR redundant, however, there are lots of work left for me to do on the dynamic ViewportState work first.

As I don't have an example to test against I won't be able to confirm that my hunch is correct. I'll keep posting to both PR threads as this work progress with the aim of getting input and extra testing.

[robertosfield]

From my experimental dynamic-viewports2 branch I have removed the State::scissorStack and viewportStack as these aren't necessary now. There's the diff between the dynamic-viewport and dynamic-viewport2 branches:

dynamic-viewport...dynamic-viewport2

I have been able to remove more lines of code than I've added and in principle if should be more flexible and have lower overhead so I think it's coming together well enough.

I'm now thinking about how much of the older VSG code relating to setting up the ViewportState can be removed completely. Removing all this code is potentially more risky, but I feel it's worth seeing if it can be done. It may be best for me to create another branch for these changes. It's getting near end of my working day (Saturday?!) so I think I'll not attempt anything more significant today.

I think the dynamic-viewports2 is now far enough along to warren wider testing.

[robertosfield]

In the dynamic_viewport3 branch I have added a RenderGraph::viewportState member and assign a vsg::ViewportState to RenderGraph object in the constructor and then sync the values at runtime. The commit that adds these is: 7742fb5

@theodoregoetz With this commit I now believe this PR is no longer required. I am about to merge the dynamic_viewports3 branch with VSG master so would appreciate testing to confirm that the commit.