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

Buffer backend rework, SSBO support #523

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

skalarproduktraum
Copy link
Member

No description provided.

@skalarproduktraum skalarproduktraum added enhancement Issue or PR discusses an enhancement vulkan Concerns only the Vulkan backend labels May 3, 2023
@skalarproduktraum skalarproduktraum requested a review from moreApi May 3, 2023 18:52
@skalarproduktraum skalarproduktraum self-assigned this May 3, 2023
@@ -709,6 +711,17 @@ open class VulkanRenderer(hub: Hub,
usage = VK_BUFFER_USAGE_VERTEX_BUFFER_BIT or VK_BUFFER_USAGE_INDEX_BUFFER_BIT or VK_BUFFER_USAGE_TRANSFER_DST_BIT
)

ssboUploadPool = VulkanBufferPool(
device,
usage = VK_BUFFER_USAGE_STORAGE_BUFFER_BIT or VK_BUFFER_USAGE_TRANSFER_DST_BIT
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to double-check: the upload pool is intended to receive data from the CPU side, yes? Then TRANSFER_DST_BIT is correct. Otherwise the two need to be switched. Also, is there a particular reason to have two pools? There's likely no performance issue with having one that has both SRC and DST.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the GPU side, there is a slight performance boost, as SSBOs are layed out differently. Also, a read only buffer can live in faster device local memory, while a writable buffer needs to be host visible or coherent (not to familiar with the concepts yet)
Thats why I decided to split up and download buffers into 2. You dont always want to write into a buffer and vice verse. Especially for read only buffers it might be good to keep this practice

@@ -0,0 +1,184 @@
package graphics.scenery.primitives
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 is the completely wrong place for an example 😅 And if it's not an actual test (which seems to be the case) then it should not be named Test (this might actually lead to issues with auto-discovery of tests, some of my logic there just filters by class name)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh yes, at that point I wasn't sure what to do to test it. Its more a help for me to visualize the general data flow through the renderer ':D

@moreApi moreApi removed their request for review May 19, 2023 09:22
@skalarproduktraum skalarproduktraum marked this pull request as ready for review May 30, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue or PR discusses an enhancement vulkan Concerns only the Vulkan backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants