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

Split up features. #6905

Merged
merged 55 commits into from
Feb 8, 2025
Merged

Split up features. #6905

merged 55 commits into from
Feb 8, 2025

Conversation

Vecvec
Copy link
Contributor

@Vecvec Vecvec commented Jan 13, 2025

Connections
Fixes #5247 as this can be infinitely extended (or at least until the stack is exhausted).

Description
Creates an extendable way to use sets of bitflags together. This then splits up features into two subpart: FeaturesWGPU and FeaturesWebGPU. These can be further extended or split if that is needed (e.g shader features vs other features). This is ready for review, tell me if this is the wrong path. I'm quite new to macros (this is probably my second), so tell me if I've missed a helpful feature.

Implementation
Internally this uses a struct as that was the easiest to interact with extendable macros (though externally this interacts with arrays). I think it wouldn't be too hard to switch everything to arrays, but I think it might be slightly messier.

Testing
All existing tests should cover this

Implemented const Functions

  • empty
  • all
  • bits
  • from_bits
  • from_bits_truncate
  • from_bits_retain
  • from_name
  • is_empty
  • is_all
  • intersects
  • contains
  • intersection
  • union
  • difference
  • symmetric_difference
  • complement
  • iter
  • iter_names

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Sorry, something went wrong.

@Vecvec
Copy link
Contributor Author

Vecvec commented Jan 13, 2025

The diff that the entire thing has made is slightly confusing, I would recommend reading it separately (or at least Basic before Split up Features.).

Vecvec and others added 13 commits January 13, 2025 19:12
…ures-main

# Conflicts:
#	wgpu-types/src/lib.rs
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.

Took a look for a pre-review; looks nice and not as bad, especially if we add some more docs.

Vecvec and others added 8 commits January 15, 2025 08:35
# Conflicts:
#	wgpu-types/src/lib.rs
@cwfitzgerald
Copy link
Member

Would be interesting to run cargo-semver-checks against this change, might catch any subtleties missed

@Wumpf
Copy link
Member

Wumpf commented Feb 2, 2025

one thing I forgot to do so far is to look at the rustdoc output: We need to make sure that this is all still sane and shows the right things.
We should have a browse over that before merging. Something to look out for is the duplication of all feature flags that is between the "sub flag structs" and the actual Features flag - does it all make sense, or is it confusing thus that we should hide some docs? etc.

@Vecvec
Copy link
Contributor Author

Vecvec commented Feb 2, 2025

Something to look out for is the duplication of all feature flags that is between the "sub flag structs" and the actual Features flag

I could also not re-export wgpu/webgpu features structs to make users have to use wgpu_types::features:: for them while wgpu_types:: for the normal Features. This could also help distinguish them (though if its confusing hiding docs should be first and this second).

@cwfitzgerald cwfitzgerald self-requested a review February 3, 2025 01:51
Vecvec and others added 12 commits February 3, 2025 17:27
@aloucks
Copy link
Contributor

aloucks commented Feb 5, 2025

Why not just call FeaturesWGPU NativeFeatures (or FeaturesNative)?

@cwfitzgerald cwfitzgerald requested a review from Wumpf February 5, 2025 16:21
@Vecvec
Copy link
Contributor Author

Vecvec commented Feb 5, 2025

Why not just call FeaturesWGPU NativeFeatures (or FeaturesNative)?

No real reason, it was just the name I came up with first. I can call it that if you think it's more clearer though.

@Wumpf
Copy link
Member

Wumpf commented Feb 8, 2025

I actually prefer FeaturesWGPU since I think it's a lot clearer. These are features that aren't native by any particular standard as they are very specific to wgpu. Having that reflected in the name is a win imho.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

thanks for incorporating those improvements. All good to go!

Right now the new features structs aren't exposed through wgpu, I have some experiments locally of what that could look like. I'll create a separate PR for those and ping you on that :)
(alongside with some formatting nits that weren't worth the hold-up - would have pushed to this PR, but for some reason push to PR doesn't work again today, feels kinda random when it does 🙄 )

@Wumpf Wumpf merged commit 6558deb into gfx-rs:trunk Feb 8, 2025
33 checks passed
@Wumpf
Copy link
Member

Wumpf commented Feb 8, 2025

marcpabst pushed a commit to marcpabst/wgpu that referenced this pull request Feb 19, 2025
Splits up features into wgpu & webgpu features.
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.

Plan for When Features overflows u64
4 participants