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

Polish a few things in Vulkan RD #81912

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Sep 19, 2023

A few little silly things. Summoning Production as reviewer, since this is not even about rendering, but area-agnostic code maintenance.

(Comments added to the changes via GH feature about why each change.)

@RandomShaper RandomShaper added this to the 4.2 milestone Sep 19, 2023
@RandomShaper RandomShaper requested a review from a team September 19, 2023 10:50
@RandomShaper RandomShaper requested a review from a team as a code owner September 19, 2023 10:50
@@ -5918,7 +5918,7 @@ Error RenderingDeviceVulkan::buffer_copy(RID p_src_buffer, RID p_dst_buffer, uin

// This method assumes the barriers have been pushed prior to being called, therefore no barriers are pushed
// for the source or destination buffers before performing the copy. These masks are effectively ignored.
VkPipelineShaderStageCreateFlags src_stage_mask = 0;
VkPipelineStageFlags src_stage_mask = 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

This worked because in the end it's a plain int, but was the wrong typedef.

Comment on lines -6041 to -6042
// Should not be needed.
// _buffer_memory_barrier(buffer->buffer, p_offset, p_size, dst_stage_mask, VK_PIPELINE_STAGE_TRANSFER_BIT, dst_access, VK_ACCESS_TRANSFER_WRITE_BIT, p_post_barrier);
Copy link
Member Author

Choose a reason for hiding this comment

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

By now we'd already know. Anyway, we can always find out it's not true and re-add, tracking this removal if needed.

@@ -6050,7 +6047,7 @@ Error RenderingDeviceVulkan::buffer_clear(RID p_buffer, uint32_t p_offset, uint3
dst_stage_mask = VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT;
}

_buffer_memory_barrier(buffer->buffer, p_offset, p_size, VK_PIPELINE_STAGE_TRANSFER_BIT, dst_stage_mask, VK_ACCESS_TRANSFER_WRITE_BIT, dst_access, dst_stage_mask);
_buffer_memory_barrier(buffer->buffer, p_offset, p_size, VK_PIPELINE_STAGE_TRANSFER_BIT, dst_stage_mask, VK_ACCESS_TRANSFER_WRITE_BIT, dst_access, true);
Copy link
Member Author

Choose a reason for hiding this comment

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

This worked before the change because dst_stage_mask would always be non-zero, that is, true would be passed as the argument, but the code was awkward.

Copy link

Choose a reason for hiding this comment

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

buffer_update() above is very similar to this function but it wraps this barrier in a condition. Should this function also have the condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's very likely, but since this PR aims to be about very silly little things, the reasoning to be sure is out of its scope.

@akien-mga akien-mga merged commit 12b3130 into godotengine:master Sep 20, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the polish_vk branch September 20, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants