Skip to content
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

Port to d3d12 0.7 with ownership #3936

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Conversation

jrmuizel
Copy link
Contributor

This replaces the WeakPtr usage with ComPtr which is no longer Copy. This means we need to do a bunch of clone()s and take some things as references.

Checklist

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories

Description
Describe what problem this is solving, and how it's solved.

Testing
Explain how this change is tested.

This replaces the WeakPtr usage with ComPtr which is no longer
Copy. This means we need to do a bunch of clone()s and take some
things as references.
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Noting that I have no merge privileges, I've reviewed these changes in some depth for the last couple of days. I feel satisfied that no meaningful changes have been made WRT the actual Release of COM objects where intended, which IMO is the major concern here. This does extend the lifetime of some objects based on our testing of issues that Firefox users have encountered, but in a way that's needed to avoid issues. We've already tested consuming this change in Firefox, and it fixes some important bugs, a few CTS test cases among them.

@ErichDonGubler ErichDonGubler added type: bug Something isn't working area: correctness We're behaving incorrectly api: dx12 Issues with DX12 or DXGI area: cts Issues stemming from the WebGPU Conformance Test Suite platform: windows Issues with integration with windows labels Jul 18, 2023
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

This all looks reasonable to me. Together with Erich's review I think this should land.

@jimblandy jimblandy merged commit 1161a22 into gfx-rs:trunk Jul 18, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: dx12 Issues with DX12 or DXGI area: correctness We're behaving incorrectly area: cts Issues stemming from the WebGPU Conformance Test Suite platform: windows Issues with integration with windows type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants