/gfx

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

[mtl] fix renderpass clear operations #2129

Merged
merged 4 commits into from Jun 9, 2018

Conversation

Reviewers

@grovesNL grovesNL

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@kvark @grovesNL
@kvark
Member

kvark commented Jun 8, 2018

Our big problem was unexpected clearing of the render passes, happened because:

  1. the descriptor was created in create_render_pass with the clear ops setup
  2. then copied over in create_framebuffer, set with exact texture references
  3. then we'd copy that one again on begin_render_pass, and set the actual clear values..

The PR eliminates the first instance of the descriptor, which greatly simplifies the code.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends:
@grovesNL

grovesNL reviewed Jun 8, 2018
View changes

src/backend/metal/src/window.rs Outdated
@@ -16,7 +16,7 @@ use core_foundation::base::TCFType;
use core_foundation::string::CFString;
use core_foundation::dictionary::CFDictionary;
use core_foundation::number::CFNumber;
use core_graphics::base::CGFloat;
//use core_graphics::base::CGFloat;

This comment has been minimized.

Sign in to view
Copy link
@grovesNL

grovesNL Jun 8, 2018

Member

Are we keeping these in gfx for now? Does it break hal/quad on your Retina display?

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jun 8, 2018

Member

Excellent point! They are broken. How about we only apply the scale when initialized via winit and ignore it otherwise?
src/backend/metal/src/command.rs Outdated
fn set_operations(attachment: &metal::RenderPassAttachmentDescriptorRef, ops: AttachmentOps) -> bool {
attachment.set_load_action(conv::map_load_operation(ops.load));
attachment.set_store_action(conv::map_store_operation(ops.store));
ops.load == AttachmentLoadOp::Clear

This comment has been minimized.

Sign in to view
Copy link
@grovesNL

grovesNL Jun 8, 2018

Member

nit: seems a bit confusing to do this here

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jun 8, 2018

Member

yeah, I agree. It is sooo tempting though. Would it be cleaner if I return something like:

OperationFollowup {
  clear: bool,
}

@kvark kvark force-pushed the kvark:mtl-ops branch from 3b9791c to 62577b4 Jun 8, 2018

@kvark
[mtl] limited swizzle support
Loading status checks…
c1cf138
@grovesNL

grovesNL reviewed Jun 9, 2018
View changes

src/backend/metal/src/command.rs Outdated
@@ -1109,11 +1109,11 @@ impl pool::RawCommandPool<Backend> for CommandPool {
}
}

// Sets up the load/store operations. Returns `true` if the clear color needs to be set.
fn set_operations(attachment: &metal::RenderPassAttachmentDescriptorRef, ops: AttachmentOps) -> bool {
/// Sets up the load/store operations. Returns `true` if the clear color needs to be set.

This comment has been minimized.

Sign in to view
Copy link
@grovesNL

grovesNL Jun 9, 2018

Member

nit: this comment is incorrect now
@kvark

This comment has been minimized.

Sign in to view
Member

kvark commented Jun 9, 2018

bors r=grovesNL

bors bot added a commit that referenced this pull request Jun 9, 2018

@bors[bot] @kvark
Merge #2129 
2129: [mtl] fix renderpass clear operations r=grovesNL a=kvark

Our big problem was unexpected clearing of the render passes, happened because:
  1. the descriptor was created in `create_render_pass` with the clear ops setup
  2. then copied over in `create_framebuffer`, set with exact texture references
  3. *then* we'd copy that one again on `begin_render_pass`, and set the actual clear values..

The PR eliminates the first instance of the descriptor, which greatly simplifies the code.

PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [ ] tested examples with the following backends:


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Loading status checks…
e249eb1
@bors

This comment has been minimized.

Sign in to view
Contributor

bors bot commented Jun 9, 2018

Build succeeded

@bors bors bot merged commit c1cf138 into gfx-rs:master Jun 9, 2018
3 checks passed

3 checks passed

bors Build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kvark kvark deleted the kvark:mtl-ops branch Jun 9, 2018

@grovesNL grovesNL referenced this pull request Jun 11, 2018

Merged

mtl: Fix depth stencil issues #2137

3 of 3 tasks complete

bors bot added a commit that referenced this pull request Jun 12, 2018

@bors[bot] @grovesNL
Merge #2137 
2137: mtl: Fix depth stencil issues r=kvark a=grovesNL

Fixes #2134

PR checklist:
- [X] `make` succeeds (on *nix)
- [X] `make reftests` succeeds
- [X] tested examples with the following backends: Metal

Probably easier to read these commits one at a time because they all fix separate issues:

- Always set load/store actions for attachments which aren't being cleared. We need this to preserve values in shared textures, i.e. between two separate depth and stencil attachments with the same backing texture.
- During the changes in #2129, we stopped mapping load/store actions during framebuffer creation. As a result we always used "don't care" which led to unexpected images at the beginning of render passes. For example, if image clears occurred prior to beginning the render pass, we had no guarantee that the image was still cleared at the beginning of the render pass.
- The stencil write mask updates passed the wrong argument, so they were misinterpreted as read mask updates.

Co-authored-by: Joshua Groves <josh@joshgroves.com>
Loading status checks…
e27378b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment