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

Add expose-ids feature #3104

Merged
merged 7 commits into from
Nov 4, 2022
Merged

Add expose-ids feature #3104

merged 7 commits into from
Nov 4, 2022

Conversation

TheOnlyMrCat
Copy link
Contributor

@TheOnlyMrCat TheOnlyMrCat commented Oct 14, 2022

Checklist

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

Connections
Discussed in matrix

Description
Adds the expose-ids feature, which allows API users to compare and store the identity of GPU objects through an opaque Id type. On native, Id wraps an unzipped wgc::TypedId; on web, Id wraps a manually-generated unique u64.

Most types with an id field now expose a feature-gated global_id method.

Testing
Not functionally tested yet, only typechecked

Unresolved Questions

  • Should Ids be type-tagged, and if so, should there still be an untyped option?
  • Should global atomics be used to guarantee uniqueness between multiple Contexts on native?

@i509VCB
Copy link
Contributor

i509VCB commented Oct 14, 2022

Are these changes related to #3051?

wgpu/src/backend/web.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

@i509VCB this particular use case is for unique identification of various resources for use in a cache, they need to be globally unique, not just unique-at-any-one-time.

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2022

Codecov Report

Merging #3104 (1d30c08) into master (ce08179) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3104      +/-   ##
==========================================
- Coverage   64.75%   64.74%   -0.02%     
==========================================
  Files          81       81              
  Lines       38711    38747      +36     
==========================================
+ Hits        25066    25085      +19     
- Misses      13645    13662      +17     
Impacted Files Coverage Δ
wgpu/src/backend/direct.rs 55.25% <0.00%> (-0.48%) ⬇️
wgpu/src/lib.rs 52.81% <0.00%> (-0.06%) ⬇️
wgpu-core/src/track/texture.rs 60.18% <0.00%> (-20.66%) ⬇️
wgpu-core/src/track/buffer.rs 85.38% <0.00%> (-11.52%) ⬇️
wgpu-core/src/resource.rs 25.13% <0.00%> (-2.47%) ⬇️
wgpu-hal/src/dx12/descriptor.rs 87.56% <0.00%> (-0.50%) ⬇️
wgpu-hal/src/vulkan/adapter.rs 77.68% <0.00%> (-0.19%) ⬇️
wgpu-hal/src/gles/queue.rs 61.65% <0.00%> (-0.17%) ⬇️
wgpu-hal/src/dx12/adapter.rs 80.65% <0.00%> (-0.05%) ⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Looks great outside of a small comment

wgpu/src/backend/web.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

Sorry for the long delay in reviewing

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Thanks!

@cwfitzgerald cwfitzgerald merged commit 6ed103e into gfx-rs:master Nov 4, 2022
@TheOnlyMrCat TheOnlyMrCat deleted the expose-ids branch January 2, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants