Port to the new swapchain model #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Compatibility list:
|
|
How does this interact with present modes? In particular, out of order presentation and acquiring multiple swapchain images? |
Should be unaffected.
multiple images of the same swapchain - not supported by this path |
msiglreith
approved these changes
Apr 5, 2020
|
Given the amount of testing I'm doing right now, will be confident to release portability-0.8 right after this merges, if we proceed 😄 |
This was referenced Apr 7, 2020
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
| maxPerStageDescriptorStorageBuffers: limits.max_per_stage_descriptor_storage_buffers as _, | ||
| maxPerStageDescriptorSampledImages: limits.max_per_stage_descriptor_sampled_images as _, | ||
| maxPerStageDescriptorStorageImages: limits.max_per_stage_descriptor_storage_images as _, | ||
| maxPerStageDescriptorInputAttachments: limits.max_per_stage_descriptor_input_attachments as _, |
There was a problem hiding this comment.
Suggested change
| maxPerStageDescriptorInputAttachments: limits.max_per_stage_descriptor_input_attachments as _, | |
| maxPerStageDescriptorInputAttachments: limits.max_per_stage_descriptor_input_attachments | |
| as _, |
This comment was generated by: rustfmt
| // Warning: spec violation | ||
| // "The x/y rectangle of the viewport must lie entirely within the current attachment size." | ||
| viewportBoundsRange: [0.0, viewport_size as f32], | ||
| maxDescriptorSetUniformBuffersDynamic: limits.max_descriptor_set_uniform_buffers_dynamic.max(1) as _, |
There was a problem hiding this comment.
Suggested change
| maxDescriptorSetUniformBuffersDynamic: limits.max_descriptor_set_uniform_buffers_dynamic.max(1) as _, | |
| maxDescriptorSetUniformBuffersDynamic: limits | |
| .max_descriptor_set_uniform_buffers_dynamic | |
| .max(1) as _, |
This comment was generated by: rustfmt
| viewportBoundsRange: [0.0, viewport_size as f32], | ||
| maxDescriptorSetUniformBuffersDynamic: limits.max_descriptor_set_uniform_buffers_dynamic.max(1) as _, | ||
| maxDescriptorSetStorageBuffers: limits.max_descriptor_set_storage_buffers as _, | ||
| maxDescriptorSetStorageBuffersDynamic: limits.max_descriptor_set_storage_buffers_dynamic.max(1) as _, |
There was a problem hiding this comment.
Suggested change
| maxDescriptorSetStorageBuffersDynamic: limits.max_descriptor_set_storage_buffers_dynamic.max(1) as _, | |
| maxDescriptorSetStorageBuffersDynamic: limits | |
| .max_descriptor_set_storage_buffers_dynamic | |
| .max(1) as _, |
This comment was generated by: rustfmt
| pointSizeGranularity: 0.0, | ||
| lineWidthGranularity: 0.0, | ||
| strictLines: 0, | ||
| standardSampleLocations: if limits.standard_sample_locations { VK_TRUE } else { VK_FALSE }, |
There was a problem hiding this comment.
Suggested change
| standardSampleLocations: if limits.standard_sample_locations { VK_TRUE } else { VK_FALSE }, | |
| standardSampleLocations: if limits.standard_sample_locations { | |
| VK_TRUE | |
| } else { | |
| VK_FALSE | |
| }, |
This comment was generated by: rustfmt
| @@ -6,21 +6,19 @@ use hal::{ | |||
| pool::CommandPool as _, | |||
| pso::DescriptorPool, | |||
| queue::{CommandQueue as _, QueueFamily}, | |||
| window::{PresentMode, Surface, Swapchain as _}, | |||
| window::{PresentMode, PresentationSurface as _, Surface as _}, | |||
| {command as com, memory, pass, pso, queue}, | |||
There was a problem hiding this comment.
Suggested change
| {command as com, memory, pass, pso, queue}, | |
| {command as com, memory, pass, pso, queue}, {Features, Instance}, |
This comment was generated by: rustfmt
| @@ -1546,7 +1586,7 @@ pub extern "C" fn gfxCreateSemaphore( | |||
| pSemaphore: *mut VkSemaphore, | |||
| ) -> VkResult { | |||
| let semaphore = match gpu.device.create_semaphore() { | |||
| Ok(s) => s, | |||
| Ok(raw) => Semaphore { raw, is_fake: false }, | |||
There was a problem hiding this comment.
Suggested change
| Ok(raw) => Semaphore { raw, is_fake: false }, | |
| Ok(raw) => Semaphore { | |
| raw, | |
| is_fake: false, | |
| }, |
This comment was generated by: rustfmt
| @@ -1847,19 +1888,30 @@ pub extern "C" fn gfxCreateImageView( | |||
| pView: *mut VkImageView, | |||
| ) -> VkResult { | |||
| let info = unsafe { &*pCreateInfo }; | |||
| if let Image::SwapchainFrame { swapchain, frame } = *info.image { | |||
| unsafe { | |||
| *pView = Handle::new(ImageView::SwapchainFrame { | |||
There was a problem hiding this comment.
Suggested change
| *pView = Handle::new(ImageView::SwapchainFrame { | |
| *pView = Handle::new(ImageView::SwapchainFrame { swapchain, frame }); |
This comment was generated by: rustfmt
| if let Image::SwapchainFrame { swapchain, frame } = *info.image { | ||
| unsafe { | ||
| *pView = Handle::new(ImageView::SwapchainFrame { | ||
| swapchain, |
There was a problem hiding this comment.
Suggested change
| swapchain, |
This comment was generated by: rustfmt
| unsafe { | ||
| *pView = Handle::new(ImageView::SwapchainFrame { | ||
| swapchain, | ||
| frame, |
There was a problem hiding this comment.
Suggested change
| frame, |
This comment was generated by: rustfmt
| *pView = Handle::new(ImageView::SwapchainFrame { | ||
| swapchain, | ||
| frame, | ||
| }); |
There was a problem hiding this comment.
Suggested change
| }); |
This comment was generated by: rustfmt
|
We can't hold this off forever. |
bors bot
added a commit
that referenced
this pull request
Aug 5, 2020
210: Port to the new swapchain model r=kvark a=kvark Closes #208 Fixes #212 Also has general fixes to: - push constant offsets in `gfxCreatePipelineLayout` - destination subresources in `gfxCmdResolveImage` The PR is inviting for discussion: @grovesNL @msiglreith I'm not 100% sure what to do yet. The Good: 1. Being able to remove the old swapchain from gfx-rs COMPLETELY. It was hacky on all the backends other than Vulkan, quite complicated on Metal, and didn't work very in general. 2. Fullscreen support! It's not ideal yet (occasional hitches), but it was pretty much non-functional before. In general, stuff that runs is more robust this change. The Bad: 1. Portability implementation is a bit more complex now, but not extraordinary so. In general, I think it's a win if we move some complication out of gfx-rs into gfx-portability, since gfx-rs has more users, it's more important to keep clean. 2. We can't support recording a render pass that uses a swapchain image if either: - the image is not acquired at the moment of recording - the command buffer is re-usable. We expect the users to strictly acquire-record-submit-present. The Ugly: 1. Our KHR swapchain implementation is more limited: no support for any usage other than COLOR_ATTACHMENT. This is technically valid in Vulkan, but for some reason many apps expect to transfer to/from swapchain images. So this is fine for the matter of CTS and VkPI, but can introduce some friction in real world testing. 2. The new swapchain model itself needs to be evolved a bit more, according to gfx-rs/gfx#3184 . We don't know how exactly: there is both a risk (that we'll need to redo something here) and an opportunity (that we'll support other usages, for example). Co-authored-by: Dzmitry Malyshau <[email protected]>
|
Timed out. |
|
bors retry |
bors bot
added a commit
that referenced
this pull request
Aug 5, 2020
210: Port to the new swapchain model r=kvark a=kvark Closes #208 Fixes #212 Also has general fixes to: - push constant offsets in `gfxCreatePipelineLayout` - destination subresources in `gfxCmdResolveImage` The PR is inviting for discussion: @grovesNL @msiglreith I'm not 100% sure what to do yet. The Good: 1. Being able to remove the old swapchain from gfx-rs COMPLETELY. It was hacky on all the backends other than Vulkan, quite complicated on Metal, and didn't work very in general. 2. Fullscreen support! It's not ideal yet (occasional hitches), but it was pretty much non-functional before. In general, stuff that runs is more robust this change. The Bad: 1. Portability implementation is a bit more complex now, but not extraordinary so. In general, I think it's a win if we move some complication out of gfx-rs into gfx-portability, since gfx-rs has more users, it's more important to keep clean. 2. We can't support recording a render pass that uses a swapchain image if either: - the image is not acquired at the moment of recording - the command buffer is re-usable. We expect the users to strictly acquire-record-submit-present. The Ugly: 1. Our KHR swapchain implementation is more limited: no support for any usage other than COLOR_ATTACHMENT. This is technically valid in Vulkan, but for some reason many apps expect to transfer to/from swapchain images. So this is fine for the matter of CTS and VkPI, but can introduce some friction in real world testing. 2. The new swapchain model itself needs to be evolved a bit more, according to gfx-rs/gfx#3184 . We don't know how exactly: there is both a risk (that we'll need to redo something here) and an opportunity (that we'll support other usages, for example). Co-authored-by: Dzmitry Malyshau <[email protected]>
|
Timed out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #208
Fixes #212
Also has general fixes to:
gfxCreatePipelineLayoutgfxCmdResolveImageThe PR is inviting for discussion: @grovesNL @msiglreith
I'm not 100% sure what to do yet.
The Good:
The Bad:
- the image is not acquired at the moment of recording
- the command buffer is re-usable. We expect the users to strictly acquire-record-submit-present.
The Ugly: