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

Learning and minor corrections #163

Closed
wants to merge 1 commit into from

Conversation

CyberKenneth
Copy link

added a note around 309 with a possible issue with sync and saw a , where a ; should be i believe at 240

@LukasBanana LukasBanana added the improvement General improvements and logical fixes. label Feb 19, 2025
Copy link
Author

@CyberKenneth CyberKenneth left a comment

Choose a reason for hiding this comment

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

I remember commenting on the layout tracking issue because not updating the internal state after transitioning the image layout could lead to synchronization problems. Without correctly tracking the layout (like currentLayout_ or pendingLayout_), subsequent operations might use the wrong layout, causing rendering artifacts and Vulkan validation errors. This could be the reason behind the issue #145 I believe was it. This was my tiny suggestion to see if it helped while it's not the entire file and stuff it is something that in theory May improve performance or decrease artifacts and if that was the case then we could go further

@CyberKenneth
Copy link
Author

removed the comment in

   VKCommandContext&           context,
   VkImageLayout               newLayout,
   bool                        flushBarrier)
{
   const TextureSubresource fullSubresource{ 0, numArrayLayers_, 0, numMipLevels_ };
   VkImageLayout oldLayout = image_.TransitionImageLayout(context, GetVkFormat(), newLayout, fullSubresource);
   if (flushBarrier)
       context.FlushBarriers();
   return oldLayout;
   // CRITICAL ISSUE Possible: This function doesn't update any internal state to track the new layout.
   // Without tracking the layout internally, subsequent calls to GetVkImageLayout() may return incorrect information.
   // This can lead to synchronization errors, validation layer errors, and potential crashes.
   // Consider adding:
   // if (flushBarrier) {
   //     context.FlushBarriers();
   //     currentLayout_ = newLayout;  // Only update layout when barriers are flushed
   // } else {
   //     pendingLayout_ = newLayout;  // Otherwise track that there's a pending transition
   //     hasPendingTransition_ = true;
   // }
}

@CyberKenneth
Copy link
Author

I closed by accident but think THe vk command etc massive comment is gone.

@LukasBanana
Copy link
Owner

I closed by accident

Can you re-open the PR or do you need assistance with that? Can't tell if you have the permissions. Otherwise, feel free to open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement General improvements and logical fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants