Skip to content

22 descriptor pools and sets #7

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

Open
wants to merge 91 commits into
base: master
Choose a base branch
from

Conversation

matthew-russo
Copy link
Contributor

this is the second half of the uniform buffer section with descriptor pools and sets

@matthew-russo matthew-russo force-pushed the 22_descriptor_pools_and_sets branch from 18ef3d7 to 127e6fd Compare February 10, 2019 03:12
@matthew-russo matthew-russo force-pushed the 22_descriptor_pools_and_sets branch from 127e6fd to 63b7d46 Compare February 14, 2019 03:58
@matthew-russo matthew-russo force-pushed the 22_descriptor_pools_and_sets branch from 63b7d46 to 5716979 Compare February 14, 2019 04:00
@matthew-russo
Copy link
Contributor Author

Found the issue on MacOS. I'm pretty sure its this: vulkano-rs/vulkano#1135.

I made some changes in the pr, and would like to get your feedback on it. Essentially we were chaining a ton of futures as you said and then the image stutters when the GPU catches up since we never actually waited for them. I added a call to wait for the future and it seems to clear up the stutter on my machine. I'm not sure if this means we can just get rid of previous_frame_end altogether now that we're waiting on the future or not. I'm still no expert on vulkano so going to keep investigating but wanted to get your input

@bwasty
Copy link
Owner

bwasty commented Feb 16, 2019

Fixes the issue for me too. Not sure about the previous_frame_end either - I would have expected that the .join(acquire_future) should have the same effect as explicitly waiting.

I also wonder if this should be marked as a hack and restricted to macOS (#[cfg(target_os = "macos")]). The CPU and GPU should not be synchronized at this point (right after submitting commands) - that's where the C++ version used all those semaphores (-> https://vulkan-tutorial.com/Drawing_a_triangle/Drawing/Rendering_and_presentation#page_Frames_in_flight).

@matthew-russo
Copy link
Contributor Author

Noted. Will make those changes. But I also had a chance to test on a different mac machine today and found it didnt work -- just a black screen pops up. It only happens on 22 and after so I'm going to look into whats going on with that. What model mac were you able to test on? I found it worked on a Macbook Pro 2015 but not on a Macbook Pro 2018. Going to look into whether other people have had issues and what the hardware differences are.

@matthew-russo
Copy link
Contributor Author

Weird. After updating the vulkan sdk from 1.1.92.1 to 1.1.97.0, everything now works on the 2018 Pro. Not sure if its worth digging into this more?

@bwasty
Copy link
Owner

bwasty commented Feb 19, 2019

I've got a Macbook Pro 2015 with Vulkan SDK 1.1.82. I'd say it's fine if it works with the latest SDK. Perhaps we can add a note to the readme to make to use the latest SDK on macOS.

@bwasty
Copy link
Owner

bwasty commented Mar 2, 2019

Looks good (apart from that small conflict). I think I'll find time to finish reviewing this and a few more chapters in a week or two.

@bwasty bwasty force-pushed the 22_descriptor_pools_and_sets branch from 266867b to adc600d Compare March 25, 2019 08:16
fn create_descriptor_pool(graphics_pipeline: &Arc<GraphicsPipelineAbstract + Send + Sync>)
-> Arc<Mutex<FixedSizeDescriptorSetsPool<Arc<GraphicsPipelineAbstract + Send + Sync>>>>
{
Arc::new(
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason for the Arc? Seems unnecessary when already using a mutex, and it works fine if I remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay in responding. No reason -- I was just in the habit of wrapping everything in an Arc. I have updated the PR accordingly

@matthew-russo matthew-russo force-pushed the 22_descriptor_pools_and_sets branch from 6a4517c to bfef11d Compare January 6, 2020 04:48
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