Default use of dynamic ViewportState.

[robertosfield]

No description provided.

[AnyOldName3]

The vsgshadow example is dying with this branch. The shadow map camera doesn't have its viewport state explicitly initialised to a meaningful value, so it's nullptr, and without this branch's changes, that meant it'd implicitly use the framebuffer's dimensions. Some of the bits I left in my earlier branch which this one eliminates were needed to make that work, so something new needs adding to replace them.

[robertosfield]

Thanks for spotting this regression in vsgshadow, time for me to dig into the code and figure out a fix.

[robertosfield]

A quick workaround for the crash, but doesn't fix the actual bug -

$ git diff
diff --git a/src/vsg/app/RecordTraversal.cpp b/src/vsg/app/RecordTraversal.cpp
index 890f71a6..b18bb4d0 100644
--- a/src/vsg/app/RecordTraversal.cpp
+++ b/src/vsg/app/RecordTraversal.cpp
@@ -576,15 +576,17 @@ void RecordTraversal::apply(const View& view)
         _state->setProjectionAndViewMatrix(view.camera->projectionMatrix->transform(), view.camera->viewMatrix->transform());
 
         auto& viewportState = view.camera->viewportState;
-
-        if (viewportState->slot >= _state->stateStacks.size())
+        if (viewportState)
         {
-            _state->stateStacks.resize(viewportState->slot + 1);
-            // info("RecordTraversal::apply(const View& view) _state->stateStacks.size() not big enough, expanding to viewportState->slot+1.");
+            if (viewportState->slot >= _state->stateStacks.size())
+            {
+                _state->stateStacks.resize(viewportState->slot + 1);
+                // info("RecordTraversal::apply(const View& view) _state->stateStacks.size() not big enough, expanding to viewportState->slot+1.");
+            }
+            _state->stateStacks[viewportState->slot].push(viewportState);
+            _state->dirty = true;
         }
 
-        _state->stateStacks[viewportState->slot].push(viewportState);
-        _state->dirty = true;
 
         if (_viewDependentState && viewportState)
         {
@@ -610,8 +612,11 @@ void RecordTraversal::apply(const View& view)
 
         view.traverse(*this);
 
-        _state->stateStacks[viewportState->slot].pop();
-        _state->dirty = true;
+        if (viewportState)
+        {
+            _state->stateStacks[viewportState->slot].pop();
+            _state->dirty = true;
+        }
     }
     else
     {

As it's not a bug fix I'll not check this in an keep investigating how best to set things up.

[robertosfield]

I have checked in a fix:

a29be81

The shadow mapping set up always should have have a ViewportState assigned to each View's Camera, rather than implicitly inheriting it from the RenderGraph, so I'm happy this is the right fix.

However, I'm torn on what changes to make to RecordTraversal to catch the case where Views are setup with Camera's that don't have a ViewportState applied. I took away the default creation of a ViewportState in RenderGraph, but now wondering if this hacky bit of RenderGraph should be added back in to just provide a safety net for existing applications that are also a bit sloppy when it comes to View/Camera setup.

[AnyOldName3]

Things can be done without needing the high-level view or camera classes, so having one of the lower-level classes handle viewport state and default to matching the framebuffer dimensions would make sense to me.

[robertosfield]

I have added dedicated State::push()/pop() methods for StateCommands vector and ref_ptr, and add use of these in RecordTraversal and RenderGraph so now there will be a fallback for when rendering is down without use of vsg::View: 83ff107

I want to look at how the slotNumber is managed to see if that can be refined further. I think this PR is probably in good enough state to merge now even without this.

[robertosfield]

As part of my testing I ran various examples with the Vulkan Validation Layer and got the following:

vsgviewer models/teapot.vsgt

VUID-VkGraphicsPipelineCreateInfo-rasterizerDiscardEnable-09024(ERROR / SPEC): msgNum: 2005076034 - Validation Error: [ VUID-VkGraphicsPipelineCreateInfo-rasterizerDiscardEnable-09024 ] | MessageID = 0x77830842 | vkCreateGraphicsPipelines(): pCreateInfos[0] Rasterization is enabled (pCreateInfos[0].pRasterizationState->rasterizerDiscardEnable is VK_FALSE), but pCreateInfos[0].pViewportState is NULL. The Vulkan spec states: If the pipeline requires pre-rasterization shader state, and the VK_DYNAMIC_STATE_RASTERIZER_DISCARD_ENABLE dynamic state is enabled or the rasterizerDiscardEnable member of pRasterizationState is VK_FALSE, and related dynamic state is not set, pViewportState must be a valid pointer to a valid VkPipelineViewportStateCreateInfo structure (https://vulkan.lunarg.com/doc/view/1.3.290.0/linux/1.3-extensions/vkspec.html#VUID-VkGraphicsPipelineCreateInfo-rasterizerDiscardEnable-09024)
    Objects: 0

Now looking into it.

[robertosfield]

The issue with null ViewportState on compile for subgraphs below vsg::View is now fixed: 12862b1

[robertosfield]

@AnyOldName3 I've wrapped up my testing and changes and feel that this PR is now is a good enough place to merge with VSG master. I'll take a break for an hour then if no issues have been reported will go ahead and merged.

[theodoregoetz]

I'm testing this branch now to see if it addresses the issue associated with #1331

[theodoregoetz]

Almost, but not quite. The scissors are updated but not the viewport which may not be the same as the scissor (often in our case). We rely on the viewport to determine the aspect ratio used in the projection, ignoring the scissor for any camera related calculation. It looks like this code assumes that the scissor and viewport are the same in many places. I'll dig a bit deeper into this to see if I can see what would need to be done.

As an aside, I think it may be a problem to have RenderGraph::renderArea which is redundant with RenderGraph::viewportState::getScissor(). there's a potential for these to conflict, and then what happens?

[robertosfield]

The RenderGraph::accept(RecordTraversal&) implementation now resets the new RenderGraph::viewportState member so that it's the same as the RenderGraph::renderArea, it does this via a viewportState::set(..) call. I've just done a code review of the ViewportState::set(..) method and it looks correct, updating both the viewport and scissor dimensions.

If users attempt to set the RenderGraph::viewportState themselves the value they attempt to set will just be overritten on the next frame.

If users want a custom combination of scissors and viewports then they can decorate their subgraph with a StateGraph which they attach a ViewportState to.

As I don't have an example that illustrates your usage case I am not able to test or even mentally walk through what the VSG will be doing and how this relates to your usage case, so I'll have to stick to trying to implement something that I think should ecompass of usage cases and hope it includes what you do.

[theodoregoetz]

I appologize for not getting a use case example to you in a timely manor. I will try to do that today. Mentally, it's as simple as this: a view that is partially off the screen. That is, the viewport (VkViewport) goes beyond the window extent, however the scissor's VkRect2D remains within the window's extent. The creation of the renderpass must use the scissor but the camera's projection matrix must use the viewport. Again, I will try to work up an example in the next few hours.

[theodoregoetz]

note that in the vsgcameras example, if the window is resized, the aspect ratio of the overlayed views gets distorted. I think this is related to this change.

Here is a quick and dirty example showing the problem of snapping the viewport to the same values of the scissor on a window resize: vsg-dev/vsgExamples#345

[robertosfield]

Thanks for the changes to vsgcameras. I will have a look. My first thought is it seems like a resize convention issue rather than something wrong with dynamic-viewport3 branch/this PR.

I think the aspect ratio distortion on resize may have existed prior to the recent changes. I had this on my TODO list to investigate.

[robertosfield]

I have just tested vsgcameras with VSG master and it exhibits the same wrong behavior of resizing the insert cameras, so this isn't a bug related to dynamic-viewports3, just another oddity that I have to look into...

[theodoregoetz]

Oh I see. It's still a good example for the viewport state work because each overlay view has it's own rendergraph and therefore it's own renderarea/viewport.

[robertosfield]

I'm wrapping up for the day now, tomorrow I'll look into the resize issue and test your mods.

I have some other issues to resolve as well, so this PR isn't quite ready for prime time. I'll aim to resolve these all tomorrow.

[robertosfield]

@AnyOldName3 I have reverted changes to CompileManager/CompileTraversal that remove the assignment of ViewportState to the defaultPipelineStates. It looks like my push to simplify just went too far.

[robertosfield]

I have updated the default ViewportState::slot number to 8 to avoid overlap with possible BindDescriptorSet. This makes the vsg::State::record() slower by 1-2% in my bigger model tests, not a regression I'm comfortable with long term, but for testing this is fine.

Later this afternoon I'll work on optimizing vsg::State so that it can handle sparse vsg::State::stateStacks vector without slow down. Once that's working we can then keep the slot numbers more widely spaced and give room for different BindGraphicsPipeline/ComputePipeline etc. as well as making a 1:1 mapping between DescriptorSet binding and slot number.

[robertosfield]

@AnyOldName3 , The vsg::State refactor that I made a couple of days ago to improve performance when handling sparse State::stateStacks, such as required to handle use of ViewportState and other dynamic state improved performance a bit when I kept the ViewportState::slot low but I found when I simulated really sparse slot numbers performance got worse and worse.

I just couldn't get my head around why until just now - the vsg::Bin::add(..) which is used for transparent objects has to duplicate the current top of the stateStacks in order to capture the full state that needs to be assigned before each transparent object is rendered. When the State::stateStacks vector is very large it has to traverse a very large set even if most are false positives.

To address this I'll need to have an active list of StateCommands in vsg::State, which is the subset of the stateStacks' that have been pushed to.

This performance issue actually highlights a bug that was lurking in the original dynamic-viewport branch - essentially the ViewportState would not have been captured so the transparent objects would have been rendered in an indeterminate state. The change I made in dynamic-viewport2 to move ViewportState to be a StateCommand by luck fixed this bug without me ever knowing about it or trying to fix it.

This does make me concerned there might be other usage cases lurking that we haven't pick up on yet. I only found this would because out of desperation I renamed the State::stateStacks member variable to find out where in the VSG it was being accessed directly and by breaking the build get to review where other code was digging into the State's internal implementation.

This all probably means it would make sense to encapsulate more functionality into vsg::State rather than have other code do it's own thing based on the State internal data structures. All more work unfortunately, but has to be done.

[robertosfield]

I was really hoping to have this branch merged and be on to tagging the 1.1.11 dev release by now, but this performance regression with sparse stackStacks is a deal breaker.

I'll not be able to resolve this issue today, so will work this weekend to try and resolve it. Yay.