vsg-dev / VulkanSceneGraph

Vulkan & C++17 based Scene Graph Project
http://www.vulkanscenegraph.org
MIT License
1.32k stars 212 forks source link

Use dynamic viewport and scissor by default #1268

Open AnyOldName3 opened 2 months ago

AnyOldName3 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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

robertosfield commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

I've pushed that alternative implementation.