Skip to content
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 dynamic viewport and scissor by default #1268

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AnyOldName3
Copy link
Contributor

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.

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.
@robertosfield
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Repository owner deleted a comment from AnyOldName3 Sep 18, 2024
@robertosfield
Copy link
Collaborator

@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
Copy link
Contributor Author

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.

ViewDependentState was less appropriate, and didn't necessarily exist for all the things that might have provided the non-dynamic viewport.
I think I've covered everything this time.
@AnyOldName3
Copy link
Contributor Author

I've pushed that alternative implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants