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

Robust view resize via modification counting #1265

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

Conversation

AnyOldName3
Copy link
Contributor

This should solve the problem where, if a subgraph is temporarily removed from the scenegraph, and a resize happens while it's detached, nothing would clear the outdated pipeline(s), and future attempts to recompile the pipeline would be skipped due to one already existing.

As noted elsewhere, I think it would be more sensible for viewport state to be dynamic so the pipeline doesn't need to be recompiled in the first place, but this is a simpler and less invasive change that also fixes the problem.

The compile traversal restores the previous view ID when it's done traversing a view, so that's probably also what the WindowResizeHandler should be doing.

I've not actually seen anything be broken or fixed by this change, so it was plausibly fine before.
Otherwise subgraphs that were temporarily detached when the resize happened won't be aware of the resize, even if they're later recompiled.
@robertosfield
Copy link
Collaborator

I've done a quick review but on first reading it feels a bit awkward so I'll want to review it a few more times, test it out and reflect on whether this is the neatest way to resolve the issue.

Is there a VSG example that illustrates the problem that this PR resolves?

@AnyOldName3
Copy link
Contributor Author

I don't think there is at the moment, but one could be thrown together pretty quickly - you just need to compile the scenegraph, detach a node with a BindGraphicsPipeline whose pipeline isn't used elsewhere, resize the window, attach the node again, and attempt to compile it. Without this PR, the attempted recompile won't do anything, and it'll use the old viewport. With this PR, the recompile will work and the new viewport will be used.

The alternative where GraphicsPipelineState has the ModifiedCount rather than the view, and GraphicsPipeline::Implementation tracks which instances of GraphicsPipleineState subclasses it was created with and what their modification counts were when it was created would be less awkward in some ways, but would mandate that more stuff was tracked and checked, so would have more of a performance impact. Copying the GraphicsPipelineState instances into GraphicsPipeline::Implementation, and then doing a deep equality check would also work, but be even slower.

As it's just the viewport that's likely to get changed globally instead of by the node itself (as it's the only state that's both likely to change and not specific to the node), it makes sense to handle it separately. As I said, the least awkward way would be if it became dynamic state, but this PR requires much fewer changes than that would.

@AnyOldName3
Copy link
Contributor Author

As an alternative, I've made #1268.

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