vsg-dev
Crash on record with nested views.
Commit 32262f8 which resets the state stack during record when it visits a View made the assumption that views were not nested. In our use-cases, we make extensive use of nested views for many different reasons. I consider this commit to be a regression on established behavior and should be reverted in favor of a solution to the original problem described in Issue #1501 that allows for nested views in the scene graph.
I didn't find an example in vsgExamples right off that had nested views but maybe there is one somewhere. If not I could try to build up an example so a regression like this does not happen again.
Shadows uses nested View's, but so far I haven't been aware of any regressions, but it doesn't depend upon inheriting state.
Could you create a example that illustrates the usage case of nested Views in the way that you use them and is now broken?
@rolandhill as the PR #1502 is from you, could do a chip in with the process of creating an example, or perhaps another example so we have a test for each usage so when we settle upon a final solution it works for each usage case.
I don't have an example prepared and the code I'm using is part of a much bigger project. Also, the issue only presents if the state is left a certain way after the previous RecordTraversal.
I think it can be fixed easily though. The regression is occurring with nested views, so we can count the views as the RecordTraversal passes through the graph and only reset state for the top level view. I'll put a PR together for testing.
I've sent https://github.com/vsg-dev/VulkanSceneGraph/pull/1540 as a draft PR, which I think should fix this issue while still fixing my previous issue https://github.com/vsg-dev/VulkanSceneGraph/issues/1501. @theodoregoetz , can you please test it and see if it solves your problem.
Is there any chance that removing the state->reset() might be possible with the addition of layout compatibility checks? @AnyOldName3 has done work on this and I have merged his related PR #1464, and also tweaked that with my own versions to optimize it further:
https://github.com/vsg-dev/VulkanSceneGraph/tree/optimized-layout-compatibility-checks
@rolandhill, I can't help be feel that the _state->reset() change in your original PR is a workaround for a problem that actually lies elsewhere.
If the lazy state updating is breaking your applications usage of sibling View's then there must be some state that is not being tracked correctly such that updates that should be happening aren't, or updates are happening that aren't appropriate or required.
These issues may well be lurking in the VSG or client applications and might pop up with odd symptoms or not noticed for a very long time, but when they do pop up could be very perplexing and hard to characterize.
As the _state->reset() workaround caused problems with @theodoregoetz's usage case, and the workaround to this workaround to get this to work for that usage case is just adding an extra layer of complexity of a workaround of a workaround that likely isn't necessary at all because the actual problem is elsewhere.
So... @rolandhill if you application has a usage case that reliably has issues with the lazy state updating then I think it would be really valuable to have a VSG example that illustrates this usage case so that we can then use it as a test case for a final proper fix rather than a workaround.
And for the same reasons I think it would test example of @theodoregoetz's usage case so that we can make sure any changes also work for that usage case.
Could you guys make some suggestions on what an example test case might look like? Even better if you could provide C++ program that illustrates the problem.
At this point I'm inclined to revert the _state->reset() workaround, as I am not confident that is a correct solution. I'll wait for further discussion before making any decision.
Right, I think something like #1540 might work but instead of adding a counter we could inspect the state stack, that is, use _state->stateStacks.size() instead of _viewCounter as proposed in that PR.
In the mean time I will try to come up with a VSG example that demonstrates our use case. I have several other things to attend to first so it may be a couple weeks out.
@rolandhill following @theodoregoetz's suggestion would something like this work?:
if (_state->stateStacks.empty()) _state->reset();
I don't think that is the same thing. Maybe I'm getting confused, but isn't the size of stateStacks related to the number of slots required? We are interested in the size of each stateStack inside the _state->stateStacks vector. State->reset() iterates through the stateStacks and resets each one individually.
If that's the case, then I don't think testing the size of each individual stateStack will work either. This is because lazy state pushes state onto the stateStack in RecordTraversal, but never pops it off. Instead, it maintains a counter (called pos) to track the current position in the stackStack. So, after RecordTraversal has been down the graph and back up again the size of the StateStack (an element of the _state->stateStacks vector) is proportional to the depth of the scenegraph (ie undefined). It is not 0, because nothing was popped off, only the tracking index (pos) changed.
We could iterate through each stateStack within the _state->stateStacks vector and test if pos == 0 and reset that stateStack if it's true. In fact, the StateStack class even includes an empty() method which just tests if pos == 0.
I think counting the Views down and back up the RecordTraversal is much faster and simpler.
The root cause of this issue in my opinion is that lazy updates leaves the stateStacks within the _state->stateStacks vector in an undefined state, in particular it leaves stack[0] set to whatever was last recorded.
There is another possible solution that I've just thought of while reviewing the code. In StateStack, index position 0 only appears to be used to store a copy of the last recorded state so that the current state can be compared with stack[0] to see if it needs to be recorded. In that case, we could test if pos == 0 in the pop() method and set state[0] = nullptr if true. So, in StateStack change
inline void pop()
{
--pos;
}
to
inline void pop()
{
--pos;
if(pos == 0) stack[0] = nullptr;
}
This would leave the stateStack in a known status at the end of a RecordTraversal as far as lazy state comparisons are concerned (assuming no state can be above the top-level Views).
I haven't tested this, but can tomorrow (I'm in Australia)
Thoughts?
Regards, Roland
You are right. Will need to check the contents of each if stacks.
On Thu, 24 Jul 2025, 09:43 Roland Hill, @.***> wrote:
rolandhill left a comment (vsg-dev/VulkanSceneGraph#1538) https://github.com/vsg-dev/VulkanSceneGraph/issues/1538#issuecomment-3112585866
I don't think that is the same thing. Maybe I'm getting confused, but isn't the size of stateStacks related to the number of slots required? We are interested in the size of each stateStack inside the _state->stateStacks vector. State->reset() iterates through the stateStacks and resets each one individually.
If that's the case, then I don't think testing the size of each individual stateStack will work either. This is because lazy state pushes state onto the stateStack in RecordTraversal, but never pops it off. Instead, it maintains a counter (called pos) to track the current position in the stackStack. So, after RecordTraversal has been down the graph and back up again the size of the StateStack (an element of the _state->stateStacks vector) is proportional to the depth of the scenegraph (ie undefined). It is not 0, because nothing was popped off, only the tracking index (pos) changed.
We could iterate through each stateStack within the _state->stateStacks vector and test if pos == 0 and reset that stateStack if it's true. In fact, the StateStack class even includes an empty() method which just tests if pos == 0.
I think counting the Views down and back up the RecordTraversal is much faster and simpler.
The root cause of this issue in my opinion is that lazy updates leaves the stateStacks within the _state->stateStacks vector in an undefined state, in particular it leaves stack[0] set to whatever was last recorded.
There is another possible solution that I've just thought of while reviewing the code. In StateStack, index position 0 only appears to be used to store a copy of the last recorded state so that the current state can be compared with stack[0] to see if it needs to be recorded. In that case, we could test if pos == 0 in the pop() method and set state[0] = nullptr if true. So, in StateStack change
inline void pop() { --pos; }to
inline void pop() { --pos; if(pos == 0) stack[0] = nullptr; }This would leave the stateStack in a known status at the end of a RecordTraversal as far as lazy state comparisons are concerned (assuming no state can be above the top-level Views).
I haven't tested this, but can tomorrow (I'm in Australia)
Thoughts?
Regards, Roland
— Reply to this email directly, view it on GitHub https://github.com/vsg-dev/VulkanSceneGraph/issues/1538#issuecomment-3112585866, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKEGUAVITU5OI6JHUMGINT3KCMBPAVCNFSM6AAAAACCDMP3Z2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCMJSGU4DKOBWGY . You are receiving this because you modified the open/close state.Message ID: @.***>
Turns out we already have a State::dirtyStateStacks() which resets the dirty mechanism in each StateStack rather that resetting the stacks to empty.
I have replaced the problem _state->reset() with _state->dirtyStateStacks() with commit 7dfa8333e3a530c9596bf73a657a1c86bed73143 and checked it in to VSG master.
@rolandhill & @theodoregoetz could you test VSG master to see if it works for both your applications. Thanks.
I'd have expected that it would be starting a new command graph rather than anything directly related to views or the record traversal that should dirty the state stacks, as it's the command graph that's in charge of the command buffer, and it's starting a new command buffer that makes the implementation forget the last recorded state (unless extensions are enabled like VK_NV_inherited_viewport_scissor). Wouldn't putting the dirtyStateStacks() call in CommandGraph::record (and similar functions that call vkBeginCommandBuffer) fix the problem identified here and probably also some other issues no one's discovered yet?
