Use dynamic viewport and scissor by default

[AnyOldName3]

Backwards compatibility is maximised by making this be set by default on DynamicState instances (so applications that already use dynamic state don't need to explicitly set extra state) and having Context have a defaulted DynamicState instance unless otherwise specified.

No changes were required to vsgExamples as far as I can tell, although there's some code in vsgmultiviews.cpp that becomes unnecessary when swapping views.

This could be implemented a little differently if SetScissor and SetViewport were changed to be StateCommands instead of plain Commands, as then they could just be pushed to state stacks during the record traversal instead of being controlled by ViewDependentState and needing explicitly dirtying if ever used without a Camera. I didn't do this in this initial implementation because it would invite discussion about which other dynamic state related Command subclasses should be turned into StateCommands at the same time and whether the slot system as it already exists is friendly towards that given that descriptor sets can eat arbitrarily many slots from 2+.

I've stripped the pipeline recreation from WindowResizeHandler as it already wasn't entirely reliable and if someone's mad enough to opt back into baked-in viewports despite using a resizable window, they can create a subclass. I've left UpdateGraphicsPipelines as the viewport count could still be changed and necessitate pipeline recreation even if viewport size changes wouldn't.

[robertosfield]

Thanks for the changes. I've done a quick code review and plan to merge and test as a branch.

I haven't done this already as I'm still off in the wees trying to get to the bottom of issues with my CompileManager/TransferTask work that is behaving differently under Windows and Linux. This work I thought would just take a week but now a month in and still debugging :-|

[AnyOldName3]

I determined earlier today that this breaks shadows as the viewport isn't bound when drawing the shadow view. I could fairly easily fix that specific problem, but it inspired me to hunt for other situations I might have missed, and unfortunately, I found one.

The current approach of this PR tacks the viewport and scissor onto the function that sets the view-dependent descriptor set. However, it seems to be possible to do a compile traversal without a View if you give it a Window instead, and with the non-dynamic viewport path, it'd use the Window's dimensions to set the viewport. When there's no View, there's no view-dependent descriptor set, so adding onto the function in charge of setting it wouldn't be sufficient.

There's an obvious alternative - if SetViewport were a StateCommand instead of a plain Command, it could be tracked by the vsg::State's state stacks and set whenever the record traversal encountered anything that needed to set the viewport. However, this makes it seem sensible that all dynamic state should exist as StateCommands, but the current slot-based indexing system doesn't really lend itself to this. The sensible thing to use as the slot identifier would be the VkDynamicState value that means that particular piece of dynamic state, but that's an enum with non-contiguous values, so not the most straightforward thing to use as an index, and even if it were contiguous, it'd clash with anything that existing applications were assigning slots to. It might be sensible to have a separate container of state stacks just for dynamic state, and then a container more amenable to this kind of indexing could be used and wouldn't clash with any existing user code. It also might give some more power to address another problem - when a pipeline where some state isn't dynamic is bound, Vulkan forgets the previous dynamic value, and it's left zeroed if you're lucky, or undefined if you're not, so you need to rebind the previous value when another dynamic pipeline's bound. That wouldn't be too much hassle to add if pipelines remembered their dynamic state and dirtied the right state stacks when they were bound.

That seemed like more design work would be warranted before leaping into an implementation, so I avoided making it part of this PR.

[AnyOldName3]

Yep, shows up twice even though it didn't show up when I refreshed the page.

[robertosfield]

@AnyOldName3 I have deleted the 2nd of the double entry :-)

Once I do a deep dive on this PR I should get an idea of what issues there are and what solutions might be appropriate. It may be that we need to extend StateGroup, or create a set of specialist versions of StateGroup that handle program and associated dynamic state vs descriptor related state. Right now we have just one StateGroup which is tied to StateCommands which may be limiting he ability to handle programs and dynamic state in the best way for them.

[AnyOldName3]

I've got most of a slightly different implementation done locally that avoids the main limitation of this PR, but I still wouldn't consider it something that's likely to remain unchanged for several years. The key difference is that instead of bodging the viewport and scissor into ViewDependentState, it adds a new member for each to State, so they can still benefit from being managed as stacks and initialised by the command graph (based on the size of the window/framebuffer to avoid problems when there's no camera with an explicit viewport), but without needing to deal with the more complicated problem of whether dynamic state should be StateCommands and how to assign slots to it if it is.

I'm not sure why I didn't go for that in the first place as it's a much more sensible compromise. I guess I overreacted to determining it was complicated to come up with a systemic solution involving vsg::State and decided I couldn't use vsg::State at all.

[AnyOldName3]

I've pushed that alternative implementation.

[robertosfield]

I have mostly wrapped up the ColorSpace work, save for community testing and making a dev release, so am now looking to the next task - dynamic viewport/scissor functionality.

Is there anything new to consider beyond this PR?

Does this PR make #1265 redundant?

[robertosfield]

I have merged as a branch so I can start testing.

https://github.com/vsg-dev/VulkanSceneGraph/tree/dynamic-viewport

I won't merge changes with master till after the 1.1.10 dev release as I don't want to clump too many changes into one dev release and make it harder to pinpoint regressions if they occur.

[AnyOldName3]

We only want one of #1265 and #1268, and #1268 (this one) is my preferred approach.

As far as I could find when I wrote this, which was five months ago now, there wasn't anything that would be able to tell the difference unless it was going out of its way to work with the same things, so none of the examples or other VSG libraries needed updating, and hopefully no VSG-based applications would, either.

Since then, though #1331 appeared, which seemed to me like something this should fix, too, unless the application was doing something weird (which might have been a sensible thing I'd not thought of or something overcomplicated that we don't want to support). This PR apparently didn't fix that problem and also made the fix from #1331 stop working, so that's potentially something that should be looked into. I found some code in one of that user's other public repos that looked potentially relevant, but it wasn't clear why it needed a fix or whether it was doing what it was doing in the most straightforward way. I think it was related to depth peeling, and that shouldn't need to meddle with viewports at all.

[robertosfield]

Thanks for the details. While I wait for replies from others on various topics I've jumped back to reviewing and pondering the #1268 changes.

My general feeling is that the approach in #1268 is on the right track and it's the details/possible optimizations and generalize that I'm pondering at present. My particular focus of pondering right now is the changes to vsg::State with the addition of two more stack stacks so the State::record() now looks like:

       inline void record()
       {
           if (dirty)
            {
                for (auto& stateStack : stateStacks)
                {
                    stateStack.record(*_commandBuffer);
                }

                scissorStack.record(*_commandBuffer);
                viewportStack.record(*_commandBuffer);

                projectionMatrixStack.record(*_commandBuffer);
                modelviewMatrixStack.record(*_commandBuffer);

                dirty = false;
            }
         }

So we have a for loop going through the stateStacks, two hardwired additions of scissorStack and viewportStack and two more for the project and modelview matrix stacks. The ViewDependentState slot defaults to 2 so we'll have at least 3 for the stateStacks to check for each stacks dirty then the 4 others, so that's typically 7 we'll be checking plus the main State::dirty() too.

So... my optimizer brain is seeing all these datastructures that need checking each time we might have some dirty state that needs record. I was never really happy with the compromise I came up with, so now we've added another 2 stacks that will be checked I'm a bit less happy. It's a CPU overhead that has grown just to accommodate dynamic viewport related state.

The other side of my optimizer brain is looking at the added code complexity and seeing all these similar but not the same data structures that have to be invoked individually because these aren't all exactly the same.

I don't have an answer for this, and the current implementation you have submitted will likely only result in a measurable slow down for very state heavy fine grained scene graph, but my gut feeling is this is an issue that I'd likely to keep shaking the tree to see if there's a way to improve flexibility and performance.

I don't know whether to try and resolve this before merging or to leave it till after.

I'm heading offline now, so more tomorrow.

[AnyOldName3]

We decided fairly early on that having a stack for each piece of state that can be changed separately was going to be bad, even if it was mapped most obviously onto a scenegraph (hence why that was what the OSG did). My earlier attempts to not have stacks for viewport and scissor state were more complicated and seemed more fragile than what we've got in this PR.

Maybe there's some way we can exploit the fact that we'd expect the viewport and scissor state to usually not change between draws (although we do need to reapply them when a pipeline with them baked-in gets bound then we go back to a dynamic viewport pipeline)?

[robertosfield]

Without more extensive changes to the VSG making all the dynamic state supportable in vsg::State would not be suitable, so the approach you've taken of just extending it to viewport and state was appropriate.

Being the guy who designed and wrote the VSG's scene graph and state management I'm in a position to consider "more extensive changes" as well as more localized changes to this PR that might take us a bit more in direction of generalizing management of state further than what the VSG does already.

As mentioned above one area of the VSG that I've not been entirely happy with is how vsg::State currently iterates through all the various state stacks checking if they are dirty. It works but can it be made more flexible and efficient?

Another area is the whole slot scheme for StateCommand, again it works but it's a bit of kludge. DescriptorSet's are bound to particular locations so there's an overlap in how Vulkan does this and the StateCommand::slot. There is also the placement of BindGraphicsPipeline, BindComputePipeline and BindRayTracingPipline on the same slot 0 by default. This steers users away from mixing graphics, compute and ray tracing in the same subgraphs.

Then there is the whole, might we want to manage dynamic graphics pipeline state in the same way as StateCommand, it's not presently a good fit for the existing way VSG works. The compromise you've come up with works, but doesn't lend itself to general purpose use for dynamic graphics pipeline state.

So all these topics are bubbling around in my head - do I spend a week or so bashing my head against a wall trying to come up with a refactor of the way the VSG manages state and unify all these strands, or just merge and come back to this more ambitious refactor.

I have the dev release to push out, so will do that this morning, then return to this topic.

[robertosfield]

I've just done another codereview and see that several sets of SetScissor and SetViewport are being created and destroyed each frame in RecordTraversal,CommandGraph and SecondaryCommandGraph in the form:

    if (framebuffer || window)
    {
        const VkExtent2D& extent = framebuffer ? framebuffer->extent2D() : window->extent2D();
        recordTraversal->getState()->scissorStack.push(SetScissor::create(0, Scissors{{{0, 0}, extent}}));
        recordTraversal->getState()->viewportStack.push(SetViewport::create(0, Viewports{{0.0f, 0.0f, static_cast<float>(extent.width), static_cast<float>(extent.height), 0.0f, 1.0f}}));
    }

Ideally we want to avoid creating and destroying objects every frame, it's not a big memory and performance overhead for a few objects but it's still an addition in the wrong direction.

While we can't always achieve it, where possible I want to find opportunities in the VSG to keep reducing the amount of data created/destroyed each frame, and to avoid conditionals and extra memory we have to process.

I don't yet have an answer to how better to do things to avoid this creation and destruction but it's something I'll note down as an area of possible refinement.

[AnyOldName3]

It'd be pretty simple to have the classes that currently allocate viewport and scissor state each frame have a member that holds a single instance that gets updated and reused. As it was five months ago, I don't recall why I didn't do that. I suspect it's either that I just copied what CompileTraversal was doing (which isn't entirely equivalent as that's not expected to run for everything every frame), or that I thought it might be confusing to have classes with a ViewportState member that duplicated what an existing extent member did and would get overwritten each frame.

Something else that could give some optimisation potential would be dropping support for scenegraphs without dynamic viewport and scissor state, as that would get rid of many of the situations where the dynamic state might get unset, and we could get away with only setting it at the start of renderpasses, subpasses and command buffers when we know it's been cleared. If an application needs something more complicated, it could provide its own implementation with explicit SetViewport calls.

[robertosfield]

Making things simpler and more efficient is something I'll always be drawn to like a month to flame. Except in this instance done right we'll see improvements rather than being scortched!

[robertosfield]

@AnyOldName3 Did you do any performance benchmarking when making these changes?

I've been playing with different performance tests today with bigger models I have but the CPU overhead is pretty low (1ms) in even quite large models I have.

In my testing I got some small slow downs with the dynamic_viewports on some runs but the variations between runs are higher than the difference with VSG master so broadly I think there isn't a notable overhead with your changes.

I have been thinking for a while about the need for automatic test scene generation that can build scenes to test various scenarios, but this is a whole task on it's own so really don't want to get go down this path as I want to wrap up the dynamic viewport work before anything else.

I will need to do some proper testing though, so thought I'd ask if there are publicly available models that you've come across that are useful for really stressing testing the scene graph.

I have spotted that vsgmulipleviews, vsgcameras and vsgwindows don't have stats built in for them so will add this so we can just get basic FPS stats and be able to enable the vsg::Profiler.

One performance optimization that would be good to do is sharing vkPipeline objects for different viewID's when everything is the same, one we use dynamic viewports this should be able to share vkPipeline across views and lower the memory and scene graph compile overhead at least.

[AnyOldName3]

Now for the less time-critical part of the response, I didn't go out of my way to do much performance testing with this branch as I couldn't think of a way to make the overhead big enough to measure.

In principle, there shouldn't be a cost to this beyond what comes from the VSG itself, as my research gave me a lot of places saying all Vulkan implementations have dynamic viewport and scissor state under the hood, and emulate it being part of the pipeline when applications bake it into the pipeline. Obviously, we're checking more state on our end, though, so there will be a cost. It's a safe bet that the if (dirty) check for the two new state stacks will effectively never pass except in situations where we'd historically have switched pipelines, so we don't need to worry about the slow path as it's replacing an even slower operation, so it's just the cost of two boolean checks happening lots of times in terms of state tracking.

There's the new cost of pushing state onto the new state stacks, though, which doesn't happen loads as it's per view/command graph/rendergraph. I don't think we expect to have a huge number of these, so it's not going to be a huge overhead even if the cost per operation is large.

As for test data, one thing I've used for other tasks is a .glb file I made with loads of identical cubes which are all one mesh. I intended it specifically to measure having loads of triangles with minimal overhead from the scenegraph for testing shader modifications, so it's the opposite of what we want here, but I'll attach it anyway in case it's useful for something else. Annoyingly, GitHub won't let me attach a .7z or .xz, and it's more than twice as large as a plain .zip.

cubes.zip

[robertosfield]

Thanks for the details and model. FYI, I've edited my previous post thanks. This is what the profiler makes of cubes.glb on my Kubuntu 24.04 + Geforce 1650 system:

ProfileLog::report(63039)
{
 { FRAME, cpu_duration = 2.19306ms, , func=virtual bool vsg::Viewer::advanceToNextFrame(double), line=157
  { CPU, cpu_duration = 8e-05ms, , func=void vsg::RecordAndSubmitTask::advance(), line=46 }
  { CPU, cpu_duration = 0.00187ms, , func=virtual void vsg::Viewer::handleEvents(), line=267
   { CPU, cpu_duration = 0.00026ms, , func=virtual void vsg::Viewer::close(), line=107
    { CPU, cpu_duration = 6e-05ms, , func=void vsg::Viewer::stopThreading(), line=766 }
   } CPU, cpu_duration = 0.00026ms, , func=virtual void vsg::Viewer::close(), line=107
  } CPU, cpu_duration = 0.00187ms, , func=virtual void vsg::Viewer::handleEvents(), line=267
  { CPU, cpu_duration = 0.00045ms, , func=virtual void vsg::Viewer::update(), line=787
   { CPU, cpu_duration = 6e-05ms, , func=virtual void vsg::AnimationManager::run(vsg::ref_ptr<vsg::FrameStamp>), line=80 }
  } CPU, cpu_duration = 0.00045ms, , func=virtual void vsg::Viewer::update(), line=787
  { CPU, cpu_duration = 2.14316ms, , func=virtual void vsg::Viewer::recordAndSubmit(), line=809
   { CPU, cpu_duration = 2.14286ms, , func=virtual VkResult vsg::RecordAndSubmitTask::submit(vsg::ref_ptr<vsg::FrameStamp>), line=85
    { CPU, cpu_duration = 2.07514ms, , func=virtual VkResult vsg::RecordAndSubmitTask::start(), line=116 }
    { CPU, cpu_duration = 0.0009ms, , func=vsg::TransferTask::TransferResult vsg::TransferTask::_transferData(DataToCopy&), line=330 }
    { CPU, cpu_duration = 0.036209ms, , func=virtual VkResult vsg::RecordAndSubmitTask::record(vsg::ref_ptr<vsg::RecordedCommandBuffers>, vsg::ref_ptr<vsg::FrameStamp>), line=143
     { CPU, cpu_duration = 0.035929ms, , func=virtual void vsg::CommandGraph::record(vsg::ref_ptr<vsg::RecordedCommandBuffers>, vsg::ref_ptr<vsg::FrameStamp>, vsg::ref_ptr<vsg::DatabasePager>), line=77
      { COMMAND_BUFFER, cpu_duration = 0.0208ms, , gpu_duration = 1.93888ms, , func=virtual void vsg::CommandGraph::record(vsg::ref_ptr<vsg::RecordedCommandBuffers>, vsg::ref_ptr<vsg::FrameStamp>, vsg::ref_ptr<vsg::DatabasePager>), line=136
       { GPU, cpu_duration = 0.02002ms, , gpu_duration = 1.93741ms, , func=virtual void vsg::RenderGraph::accept(vsg::RecordTraversal&) const, line=104
        { GPU, cpu_duration = 0.00871ms, , gpu_duration = 1.9335ms, , func=void vsg::RecordTraversal::apply(const vsg::View&), line=532
         { CPU, cpu_duration = 0.00101ms, , func=virtual void vsg::ViewDependentState::traverse(vsg::RecordTraversal&) const, line=546 }
        } GPU, cpu_duration = 0.00871ms, , gpu_duration = 1.9335ms, , func=void vsg::RecordTraversal::apply(const vsg::View&), line=532
       } GPU, cpu_duration = 0.02002ms, , gpu_duration = 1.93741ms, , func=virtual void vsg::RenderGraph::accept(vsg::RecordTraversal&) const, line=104
      } COMMAND_BUFFER, cpu_duration = 0.0208ms, , gpu_duration = 1.93888ms, , func=virtual void vsg::CommandGraph::record(vsg::ref_ptr<vsg::RecordedCommandBuffers>, vsg::ref_ptr<vsg::FrameStamp>, vsg::ref_ptr<vsg::DatabasePager>), line=136
     } CPU, cpu_duration = 0.035929ms, , func=virtual void vsg::CommandGraph::record(vsg::ref_ptr<vsg::RecordedCommandBuffers>, vsg::ref_ptr<vsg::FrameStamp>, vsg::ref_ptr<vsg::DatabasePager>), line=77
    } CPU, cpu_duration = 0.036209ms, , func=virtual VkResult vsg::RecordAndSubmitTask::record(vsg::ref_ptr<vsg::RecordedCommandBuffers>, vsg::ref_ptr<vsg::FrameStamp>), line=143
    { CPU, cpu_duration = 0.02897ms, , func=virtual VkResult vsg::RecordAndSubmitTask::finish(vsg::ref_ptr<vsg::RecordedCommandBuffers>), line=155
     { CPU, cpu_duration = 0.02238ms, , func=vsg::TransferTask::TransferResult vsg::TransferTask::_transferData(DataToCopy&), line=330
      { COMMAND_BUFFER, cpu_duration = 0.00159ms, , gpu_duration = 0.003968ms, , func=vsg::TransferTask::TransferResult vsg::TransferTask::_transferData(DataToCopy&), line=473
       { CPU, cpu_duration = 6e-05ms, , func=void vsg::TransferTask::_transferImageInfos(DataToCopy&, VkCommandBuffer, TransferBlock&, VkDeviceSize&), line=194 }
       { CPU, cpu_duration = 0.00115ms, , func=void vsg::TransferTask::_transferBufferInfos(DataToCopy&, VkCommandBuffer, TransferBlock&, VkDeviceSize&), line=85 }
      } COMMAND_BUFFER, cpu_duration = 0.00159ms, , gpu_duration = 0.003968ms, , func=vsg::TransferTask::TransferResult vsg::TransferTask::_transferData(DataToCopy&), line=473
     } CPU, cpu_duration = 0.02238ms, , func=vsg::TransferTask::TransferResult vsg::TransferTask::_transferData(DataToCopy&), line=330
    } CPU, cpu_duration = 0.02897ms, , func=virtual VkResult vsg::RecordAndSubmitTask::finish(vsg::ref_ptr<vsg::RecordedCommandBuffers>), line=155
   } CPU, cpu_duration = 2.14286ms, , func=virtual VkResult vsg::RecordAndSubmitTask::submit(vsg::ref_ptr<vsg::FrameStamp>), line=85
  } CPU, cpu_duration = 2.14316ms, , func=virtual void vsg::Viewer::recordAndSubmit(), line=809
  { CPU, cpu_duration = 0.046118ms, , func=virtual void vsg::Viewer::present(), line=843 }
 } FRAME, cpu_duration = 2.19306ms, , func=virtual bool vsg::Viewer::advanceToNextFrame(double), line=157
}

So very low CPU overhead, just too small a model to properly tax the VSG's RecordTraversal :-)

I have checked in support for --fps and profiler to vsgwindows, vsgmultiviews and vsgcameras so we can now use them to test having multiple views in play.

[robertosfield]

I am still wandering around looking for a coherent path forward on the topic of dynamic state, I have lots of ideas for different parts but nothing that brings everything together in a way that's efficient and extensible yet. I think there is a coherent solution somewhere in all of this so I'm going to keep plugging away.

With fuzzy solution spaces like this one can fell like one is aimlessly going around and around so I think I just need to start putting some concrete bits down and see what works and what doesn't, using the dynamic-viewports branch as a reference of what works, and perhaps something to build upon/adapt.

I am just starting to ticker with a dynamic-viewports2 branch derived from dynamic-viewports, but don't know if I'll have anything checked in today. I tweaked my back yesterday so my original plan for a day out running with friends has had to be canned, so I'm around tomorrow with nothing else to do so will tinker some more. I'll use this branch as a sketch-pad as just doddle code base to learn about possible approaches rather than actually aim for it be release quality.

Once I've learned more about possible design + implementation approaches we can reflect and what the final form should be and start from scratch or use one of these branches as a starting place.

The bit I think I'll play with first is whether we can use the vsg::ViewportState object as a source for the scissors and viewports passed to Vulkan via vkCmdSetScissors/vkCmdSetViewports rather than unpacking a View's Camera's ViewportState and then passing it to vsg::SetScissor/vsg::SetViewports. Perhaps a vsg::SetViewportState that "has a" vsg::ViewportSet would do the trick, or just have SetViewports "has a" ViewportState. My thought is we want to avoid copying data if can and just have one place where the data is held.

Another approach might be to make ViewportState something that can be pushed/popped from vsg::State and have this used as input for local calling of vkCmdSetScissor/vkCmdSetViewport, or have a record method in GraphicsPipelineState so that it can reused directly in the scene graph, though this might get a bit confusing to uses in GraphicsPipelineState becomes a StateCommand or Command.

[robertosfield]

@AnyOldName3 Are there any vsgExamples that relying upon the vsg::SetScissor + vsg::SetViewport push/pop now in vsg::CommandGraph + vsg::SecondaryCommandGraph?

I'm wondering about when these might be needed, I'm presuming it's cases where vsg::BindGraphicsPipeline appear in scene graphs outside of vsg::View use?

I'm currently playing with a vsg::ViewportState::record(CommandBuffer&) method that does the vkCmdSetScissor and vsg::CmdSetViewport with the vsg::GraphicsPipelineState now subclassed from vsg::StateCommand rather than vsg::Object so it can be cheekily added to a vsg::StateGroup as a StateCommand or just directly to the scene graph as a Command. If this isn't too clunky it might be a way around applications that don't want to use vsg::View but still want things synced up.

Potentially a vsg::Window/vsg::Framebuffer could "have a" ViewportState that it keeps it in sync on resize, and could be pushed/popped onto vsg::State alongside the other StateCommands. The vsg::View would also just push/pop the vsg::Camera's ViewportState.

While I'm confident I can get it to function, I don't know yet whether the concept of GraphicsPipelineState that is used to create a vkGraphicsPipeline is also something that can be more generally in the scene graph for setting pipeline state is a good fit. Actually as I write this it reads like it makes sense, though the dynamic state side doesn't have a 1:1 mapping to all of GraphicsPipelineState so at the Vulkan level it isn't a perfect fit.

Perhaps it's just a matter for finding the least kludgy, most flexible and low cost solution and we should just accept a bit of kludginess otherwise we might ever write anything.

[robertosfield]

I have now checked in my first experimental changes built on top of the dynamic-viewport branch:

https://github.com/vsg-dev/VulkanSceneGraph/tree/dynamic-viewport2

The first commit promotes GraphicsPipelineState to a StateCommand and sets the slot number for the ViewportState to 3 and implements a ViewportState::record(CommandBuffer&) that does the vkCmdSetViewport+SetScissor, and replaces the hardwired viewportStack+scissorStack from the vsg::State::record(..) :

4cc5c36

I haven't completed the change across so using ViewportState so CommandGraph and SecondaryCommandGraph still have the push/pops. I'll tackle that this afternoon.

These changes don't feel awkward so far, open the door to more flexibility and potential optimization in vsg::State::record() so it still feels like a decent enough path to explore.

[robertosfield]

I've just been searching the VSG codebase for places where the PipelineStates as set and got a whole bunch of hit in the CompileTraversal where there are various methods for passing the camera's ViewportState or creating a ViewportState as a fallback. Lots of code in there that could be potentially removed if we do enforce that we always handle Viewport/Scissors as dynamic state.

I don't yet know how much of this code we'll be safe to remove so I'm really just writing this post as a mental note/discussion point.

[AnyOldName3]

I don't think I found any examples that cared, so I just did what I could to try and make things backwards-compatible on paper by setting things wherever they were set before.

One thing to bear in mind is that it's only the viewport and scissor extents that are dynamic, but not the count. If you're doing layered rendering etc., there needs to be a different viewport and scissor set per layer, and the count needs to be made part of the pipeline. It can be made dynamic via VK_DYNAMIC_STATE_VIEWPORT_WITH_COUNT, but that's not something that's necessarily supported by all hardware or as fast as having the count baked into the pipeline. At the moment, the count and extents are kept in the same VSG object, so I kept all the existing places it was applied to the pipeline.

[robertosfield]

Thanks for the input. My current inclination is see how much we can pare things down to stuff that is essential for normal usage, and if there is possible usage that might break because of the changes at least think through possible workarounds that users could apply.

If it turns out that things end up well supported for certain usage cases then putting together an example that can be used to illustrate this and then using it as test case so we can then refine the code base to support these usage cases elegantly as well. The VK_DYNAMIC_STATE_VIEWPORT_WITH_COUNT usage case you have in mind would be a good candidate for making a example/test program to flesh this out.

I'm hopeful that the changes in @theodoregoetz's PR #1331 may be made redundant by the end of refinement of dynamic viewport support. I have already merged some of changes into VSG master, the dynamic-viewports branch based on this PR, and my dynamic-viewports2 experimental branch, so this might ease part of the issue that John was trying to solve. The commit is: 9386b11

[AnyOldName3]

I think things like VK_DYNAMIC_STATE_VIEWPORT_WITH_COUNT mostly exist to help with old-API-on-top-of-Vulkan compatibility layers like Zink and DXVK, so I don't think there's necessarily any use case for it with a VSG-based app, and we don't necessarily need to support it (or at least don't need to make it particularly easy if it causes other problems). It's baked-in viewport counts with dynamic viewport sizes that make sense to support.

[robertosfield]

I had a few minutes available so I created a dynamic-viewport3 branch from dynamic-viewport2:

https://github.com/vsg-dev/VulkanSceneGraph/tree/dynamic-viewport3

And quickly removed the code in CompileTraversal that passes on the ViewportState on as the context's defaultPipeline States:

266560e

I've run a few examples and so far haven't spotted any regressions. I think there is more I simplify beyond this. Happy to take suggestions on what can be removed once we standardize on dynamic viewport state.

[robertosfield]

Taken the scalpel to some CompileManger/CompileTraversal methods to drop the ViewportState entries as I think these are not required.

9d17041

[robertosfield]

I have now implemented sharing of GraphicsPipelineImplementation objects with commit: da2d31e

This is checked into the latest dynamic viewport branch:
https://github.com/vsg-dev/VulkanSceneGraph/tree/dynamic-viewport3

I think wraps what is required for the functionality to be complete and ready to merge with VSG master. I will build under Windows to make sure that it's all working. If things look OK I'll generate a PR from dynamic-viewport3 branch.

[robertosfield]

I have tested under Windows and am running cppcheck to do my final checks on the dynamic-viewport3 branch. While cppcheck hasn't completed yet I'm comfortable enough to generate a PR from this branch: #1390.

Once this branch is merged with VSG master I'll close this PR and associated branches and start working towards a new point release.