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 a few errors in VKCommandBuffer.cpp #16731

Closed
wants to merge 2 commits into from

Conversation

jcyuan
Copy link
Contributor

@jcyuan jcyuan commented Feb 18, 2024

1, should be Access mask, not Image layout
2, the ASSERT is useless because it's already tested above in the missed value
3, same with item 1
4, uint32_t memoryBarrierCount should not be 0 when pMemoryBarrier is not null

Copy link

Interface Check Report

This pull request does not change any public interfaces !

@minggo minggo requested a review from star-e February 19, 2024 02:30
Comment on lines +968 to +969
vkCmdPipelineBarrier(_gpuCommandBuffer->vkCommandBuffer, fullSrcStageMask, fullDstStageMask, 0, pMemoryBarrier ? 1 : 0,
pMemoryBarrier, fullBufferBarriers.size(), fullBufferBarriers.data(), fullImageBarriers.size(), fullImageBarriers.data());
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we change the if (!fullBufferBarriers.empty() || !fullImageBarriers.empty()) to if (!fullBufferBarriers.empty() || !fullImageBarriers.empty() || pMemoryBarrier) ?

pMemoryBarrier might not be executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

personally i think it's fine, because memoryBarrierCount arg for vkCmdPipelineBarrier decides it will be used or not.

@minggo minggo deleted the branch cocos:v3.8.3 May 16, 2024 02:51
@minggo minggo closed this May 16, 2024
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.

3 participants