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

Fix VK_KHR_separate_depth_stencil_layouts #1068

Merged

Conversation

mikes-lunarg
Copy link
Collaborator

Even though the test app captured and replayed without issue, the state view
showed that VkAttachmentDescriptionStencilLayout was not saved for the
render passes that included it. This appears to be caused by the api
file silently failing to modify renderPass.AttachmentDescriptions[i]
after initially setting it, so set it after processing the pNext chain
instead.

@mikes-lunarg
Copy link
Collaborator Author

@pmuetschard I'm still unsure why the original code was failing to update the render pass object, any thoughts?

@mark-lunarg
Copy link
Collaborator

@mikes-lunarg, how did you manage to notice the discrepancy -- examining the state in AGI perhaps?

@mikes-lunarg
Copy link
Collaborator Author

@mark-lunarg That's exactly how I found it. As an extra sanity check I went looking for the new extension data in the state tab of AGI and it wasn't there.

@AWoloszyn
Copy link
Contributor

Map access operations return a value not a reference.
See https://github.com/google/agi/tree/master/gapil#map (although that could probably be a bit clearer)
So given that:

renderPass.AttachmentDescriptions[j] returns a value
renderPass.AttachmentDescriptions[j].StencilLayout gives you a temporary, then sets the value on that temporary, then discards it.

@mikes-lunarg
Copy link
Collaborator Author

Map access operations return a value not a reference. See https://github.com/google/agi/tree/master/gapil#map (although that could probably be a bit clearer) So given that:

renderPass.AttachmentDescriptions[j] returns a value renderPass.AttachmentDescriptions[j].StencilLayout gives you a temporary, then sets the value on that temporary, then discards it.

Makes sense, thanks for the info!

@yalcinmelihyasin
Copy link
Contributor

Map access operations return a value not a reference. See https://github.com/google/agi/tree/master/gapil#map (although that could probably be a bit clearer) So given that:

renderPass.AttachmentDescriptions[j] returns a value renderPass.AttachmentDescriptions[j].StencilLayout gives you a temporary, then sets the value on that temporary, then discards it.

Are the a difference in handling lvalue and rvalue?

gapis/api/vulkan/api/renderpass_framebuffer.api Outdated Show resolved Hide resolved
@@ -366,7 +366,7 @@ sub ref!RenderPassObject createRenderPassObjectFromInfo2(
attachments := info.pAttachments[0:info.attachmentCount]
for i in (0 .. info.attachmentCount) {
attachment := attachments[i]
renderPass.AttachmentDescriptions[i] = AttachmentDescription(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having hard time to see how changing this solves the problem

Copy link
Collaborator Author

@mikes-lunarg mikes-lunarg Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @AWoloszyn gave a pretty thorough answer, but the short version is:

I need to might need to modify this struct inside the pNext loop so I assign it to a local variable. Then, once I have the final version of the struct, I can put it in the map. Modifying it in-place in the map (the original code) won't work because the map access or will give me a copy of the data instead of a reference.

@yalcinmelihyasin
Copy link
Contributor

yalcinmelihyasin commented Mar 29, 2022

Based on the change are these two examples are semantically different? @AWoloszyn

renderPass.AttachmentDescriptions[j] = AttachmentDescripton(...)
attachmentDescription := AttachmentDescripton(...)
renderPass.AttachmentDescriptions[j] = attachmentDescripton

@AWoloszyn AWoloszyn closed this Mar 29, 2022
@AWoloszyn AWoloszyn reopened this Mar 29, 2022
@AWoloszyn
Copy link
Contributor

AWoloszyn commented Mar 29, 2022

Based on the change are these two examples are semantically different? @AWoloszyn

renderPass.AttachmentDescriptions[j] = AttachmentDescripton(...)
attachmentDescription := AttachmentDescripton(...)
renderPass.AttachmentDescriptions[j] = attachmentDescripton

Those are the same. What would be different is

a := A(...)
a.foo = Foo()
blah.a[j] = a
a := A(...)
blah.a[j] = a
blah.a[j].foo = Foo()

The difference is that

blah.a[j].foo = Foo()

is a no-op.

This is something very subtle and error prone are there any way to fix this?

@AWoloszyn
Copy link
Contributor

Map access operations return a value not a reference. See https://github.com/google/agi/tree/master/gapil#map (although that could probably be a bit clearer) So given that:
renderPass.AttachmentDescriptions[j] returns a value renderPass.AttachmentDescriptions[j].StencilLayout gives you a temporary, then sets the value on that temporary, then discards it.

Are the a difference in handling lvalue and rvalue?

This is more about with what value map[] returns. In psuedo-C++ it would look something like:

template<typename K, typename V>
class map {
    V operator[](K key);
};

This differs from a STD unordered_map which looks like

template<typename K, typename V>
class map {
    V& operator[](K key);
};

I.e. the [] operator in GAPIL returns a COPY of the element in the map, not a REFERENCE to the element in the map.

Even though the test app captured and replayed without issue, the state view
showed that `VkAttachmentDescriptionStencilLayout` was not saved for the
render passes that included it. This is caused by the map access
operation returning a copy instead of a reference, making in-place
modification of `renderPass.AttachmentDescriptions[j].StencilLayout`
impossible. The solution here is to assign it to the map after
processing the pNext chain.
@mikes-lunarg mikes-lunarg force-pushed the fix_separate_depth_stencil_layouts branch from 91aeaf9 to 2290b35 Compare April 6, 2022 04:51
@mikes-lunarg
Copy link
Collaborator Author

@yalcinmelihyasin What is our next step? Can we merge this fix or would you like to see additional changes?

@mikes-lunarg mikes-lunarg merged commit c05bbcf into google:master May 26, 2022
@mikes-lunarg mikes-lunarg deleted the fix_separate_depth_stencil_layouts branch May 26, 2022 19:44
rosasco-wk pushed a commit to rosasco-wk/agi that referenced this pull request Sep 2, 2022
Even though the test app captured and replayed without issue, the state view
showed that `VkAttachmentDescriptionStencilLayout` was not saved for the
render passes that included it. This is caused by the map access
operation returning a copy instead of a reference, making in-place
modification of `renderPass.AttachmentDescriptions[j].StencilLayout`
impossible. The solution here is to assign it to the map after
processing the pNext chain.
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.

4 participants