-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add MULTISAMPLE_ARRAY feature flag.
#8571
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
base: trunk
Are you sure you want to change the base?
Conversation
`MULTISAMPLE_RESOLVE`.
|
You asked about this in matrix, but you should probably still check the portability subset. With new drivers like KosmicKrisp and also HoneyKrisp, simply filtering for "apple" might not be a good idea. Also, other drivers can technically use these extensions. |
|
While true that other drivers could use the portability subset, currently WGPU only requests a portability subset on apple, and does not store this in any way. Every other instance of code specifically for the portability extension uses this check. I could properly implement this check but that seems a bit out of scope if everything else does it this way. |
teoxoy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be a format flag as none of the platforms make this conditional on the format. It should be an adapter/device feature.
Vulkan (non portability) and D3D12 support this unconditionally. Metal's MTLTextureType2DMultisampleArray requires iOS/iPadOS 14.0+, macOS 10.14+, tvOS 16.0+, visionOS 1.0+. I haven't checked OpenGL ES.
Somewhat related: #4405 - though this is a different usecase.
MULTISAMPLE_ARRAY format feature flag.MULTISAMPLE_ARRAY feature flag.
|
I've changed the feature to be an adapter flag instead. This way it also was a lot easier to add support for portability platforms. I don't however have a device I can test that on. |
teoxoy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good but it should have at least a test making sure texture/view creation works and submitting a render pass with such an attachment works.
If there's low hanging fruit where it could be supported immediately on other platforms too without a lot of changes, that would be a nice-to-have.
Exposing the feature unconditionally on D3D12 should be enough to get it going. Metal would need a bit more work (optional feature and changes to view creation).
|
I've added the DX12 platform, though I don't have a device that runs it so I can't test it. |
|
I'm seeing the tests currently fail under DX12? I'll have to check the test in more detail tomorrow to see what exactly is failing. |
|
I had a look at it just now and I'm a bit stumped on why the DX12 backend is failing on the test. I'm tempted to just turn off support in DX12 again for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks very good. If we could add support for Metal that'd be superb but you aren't obligated to do that.
I'll check out the CI failures & test later
| if desc.size.depth_or_array_layers != 1 | ||
| && !self.features.contains(wgt::Features::MULTISAMPLE_ARRAY) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good
| /// Features provided by `VK_KHR_portability_subset`. | ||
| /// | ||
| /// Strictly speaking this tells us what features we *don't* have compared to core. | ||
| portability_subset: Option<vk::PhysicalDevicePortabilitySubsetFeaturesKHR<'static>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be added to device creation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for references to other *Features members in this struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch, added to the list.
| /// Enables creating texture arrays that are also multisampled. | ||
| /// | ||
| /// Without this feature, you cannot create a texture that has both a `sample_count` higher | ||
| /// than 1, and a `depth_or_array_layers` higher than 1. | ||
| /// | ||
| /// Supported platforms: | ||
| /// - Vulkan (except VK_KHR_portability_subset if multisampleArrayImage is not available) | ||
| /// - DX12 | ||
| const MULTISAMPLE_ARRAY = 1 << 56; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way we could add support to some Metal platforms in this PR? Especially with Apple Vision Pro this would probably be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be very useful for apple vision pro, but I don't have any apple products to test this on and the support seems to be non-trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the support is non-trivial? Isn't it just checking if the OS version is high enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @teoxoy it would require a few more changes. I have absolutely zero experience with the Metal API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LaylBongers Alright. If you'd rather not than can you file an issue for Metal multisample array textures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added at #8593.
Connections
Closes #8565
Description
This adds a newMULTISAMPLE_ARRAYentry toTextureFormatFeatureFlags. WhenTEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURESis enabled and the feature is supported, applications can use this to create 2D Array textures and views with multi-sampling enabled.This adds a new
MULTISAMPLE_ARRAYentry toFeatures. When enabled and the feature is supported, applications can use this to create 2D Array textures and views with multi-sampling enabledAdditionally, this is implemented in the Vulkan backend. Following the Vulkan specificiation, this should be allowed:
https://docs.vulkan.org/refpages/latest/refpages/source/VkImageCreateInfo.html
https://docs.vulkan.org/refpages/latest/refpages/source/VkImageViewCreateInfo.html#VUID-VkImageViewCreateInfo-image-04972
However this is not necessarily supported if
VK_KHR_portability_subsetis used, which is the case for apple platform. In this case support should be detected frommultisampleArrayImage.https://docs.vulkan.org/refpages/latest/refpages/source/VkPhysicalDevicePortabilitySubsetFeaturesKHR.html
This isn't currently done yet, instead apple target, which always usesVK_KHR_portability_subsetis for now excluded.Now implemented too!
To-Dos
VK_KHR_portability_subsetis not yet implemented.CurrentlyLimited to the same degree asMULTISAMPLE_ARRAYis marked true on all formats. In theory all formats that support multisample should also support this. Should this be more conservative?MULTISAMPLE_RESOLVE.Given that this bypasses some validation, are there any previous assumptions this breaks? Any things that now need to be explicitly checked?Other limitations should already be caught by other constraints.Testing
I am actively testing this branch at https://github.com/celphase/indite. Developers with an OpenXR compatible headset should be able to test multiview there.
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.