Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upFix Metal backend #1165
Conversation
JohnColanduoni
added some commits
Jan 29, 2017
kvark
reviewed
Jan 30, 2017
| #[macro_use] | ||
| extern crate gfx; | ||
| extern crate gfx_window_glutin; | ||
| extern crate glutin; | ||
| extern crate gfx_app; |
This comment has been minimized.
This comment has been minimized.
kvark
Jan 30, 2017
Member
I'm sorry, but we'd prefer to keep triangle example to be straight without basing on gfx_app.
| @@ -34,12 +34,17 @@ use native::{Rtv, Srv, Dsv}; | |||
| use metal::*; | |||
|
|
|||
| use std::ptr; | |||
| use std::collections::HashMap; | |||
| use std::collections::hash_map::Entry; | |||
This comment has been minimized.
This comment has been minimized.
| if let Some(val) = clear { | ||
| attachment.set_load_action(MTLLoadAction::Clear); | ||
| attachment.set_clear_depth(val as f64); | ||
| if let Entry::Occupied(mut clear_entry) = self.dsv_clear.entry(depth) { |
This comment has been minimized.
This comment has been minimized.
kvark
Jan 30, 2017
Member
I don't think you need to use the Entry API here, just a regular get_mut should do
This comment has been minimized.
This comment has been minimized.
JohnColanduoni
Jan 30, 2017
Author
Member
The reason I used it is that the entry may need to be deleted entirely, not just modified. If get_mut is used there would be two lookups in that case. For the depth target this is conditional so I can't simplify it to a remove(), but the render target analogue always removes the entry if present. I'll change that to remove() instead of mucking about with Entries.
This comment has been minimized.
This comment has been minimized.
|
Oh, sounds like Entry makes sense here then, no need to touch that code!
… On Jan 30, 2017, at 17:33, John Colanduoni ***@***.***> wrote:
@JohnColanduoni commented on this pull request.
In src/backend/metal/src/command.rs:
>
- let clear = unsafe { *depth.2 };
-
- if let Some(val) = clear {
- attachment.set_load_action(MTLLoadAction::Clear);
- attachment.set_clear_depth(val as f64);
+ if let Entry::Occupied(mut clear_entry) = self.dsv_clear.entry(depth) {
The reason I used it is that the entry may need to be deleted entirely, not just modified. If get_mut is used there would be two lookups in that case. For the depth target this is conditional so I can't simplify it to a remove(), but the render target analogue always removes the entry if present. I'll change that to remove() instead of mucking about with Entries.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
fkaa
reviewed
Jan 31, 2017
| @@ -247,6 +247,10 @@ impl core::Factory<Resources> for Factory { | |||
| .object_at(0) | |||
| .set_pixel_format(MTLPixelFormat::BGRA8Unorm_sRGB); | |||
|
|
|||
| // We need fake depth attachments in case explicit writes to the depth buffer are required | |||
| pso_descriptor.set_depth_attachment_pixel_format(MTLPixelFormat::Depth24Unorm_Stencil8); | |||
This comment has been minimized.
This comment has been minimized.
fkaa
Jan 31, 2017
Member
I get an error for this:
/Library/Caches/com.apple.xbs/Sources/Metal/Metal-85.82.1/Framework/MTLRenderPipeline.mm:1571: failed assertion `depthAttachmentPixelFormat is not a valid MTLPixelFormat.'
Switching out for MTLPixelFormat::Depth32_Stencil8 works though
This comment has been minimized.
This comment has been minimized.
JohnColanduoni
Jan 31, 2017
Author
Member
Good catch, apparently D24S8 is only supported on some OS X GPUs (and not at all on iOS). According to the docs all feature sets support Depth32_Stencil8 so that's a good choice.
This comment has been minimized.
This comment has been minimized.
|
Other than the triggered assertion on my end looks good to merge |
This comment has been minimized.
This comment has been minimized.
|
Awesome, thanks @JohnColanduoni and @fkaa ! |
This comment has been minimized.
This comment has been minimized.
|
|
homu
added a commit
that referenced
this pull request
Jan 31, 2017
This comment has been minimized.
This comment has been minimized.
|
|
homu
merged commit 255f54d
into
gfx-rs:master
Jan 31, 2017
JohnColanduoni
added a commit
to JohnColanduoni/gfx
that referenced
this pull request
Jun 16, 2017
JohnColanduoni commentedJan 30, 2017
I've updated the parts of the Metal backend that were out of date, allowing the examples to run. I also fixed the stencil buffer support and improved the mechanism for applying clear/clear_depth/clear_stencil commands.