Separate Out Backend Options into Individual Structs #6895
Conversation
|
There's a decent amount of code that was just moved and not majorly rewritten. This unfortunately couldn't really be captured well in a move commit without undue effort on my part. |
cwfitzgerald
force-pushed
the
cw/per-backend-configurations
branch
3 times, most recently
from
1c87f07 to
5dc796b
Compare
cwfitzgerald
force-pushed
the
cw/per-backend-configurations
branch
from
5dc796b to
96815d2
Compare
There was a problem hiding this comment.
I've been recently dealing with a lot of config things in egui & Rerun and as such I deeply appreciate this refactor. Rather kinda embarrassing I didn't bring it up myself!
But now I'd really really would want to go the extra step to make it even better & consistent, see comments. Also happy to take over
| #### Environment Variable Handling Overhaul | ||
|
|
||
| Previously how various bits of code handled reading settings from environment variables was inconsistent and unideomatic. | ||
| We have unified it to `Type::from_env()` for all types. |
There was a problem hiding this comment.
I found the with_env we had in a few places rather convenient as well. You'd do some settings and then instruct to be able to override it freely with environment variables.
We can take that as a separate iteration ofc
There was a problem hiding this comment.
In that case all from_env would just be a shortcut for default().with_env(). How that that sound to you?
There was a problem hiding this comment.
It's fine just feels a little indirect and I'm not sure I'd think of that pattern when looking at the docs maybe having both is the answer?
There was a problem hiding this comment.
Oh both is exactly what you're suggesting
There was a problem hiding this comment.
An advantage of from_env() -> Option<Self> is that the caller knows whether the environment variable was used (by seeing whether the Option is Some). This can be useful to give users hints about the usage of the variables, in particular,
- if not set, that it is possible to change the behavior, and
- if set, that the behavior was changed, which is useful confirmation when troubleshooting (because it separates “did the environment variable do anything? was it spelled correctly?” from “did using a different setting make any difference?”).
Some of the benefit can be gotten from logging the final value, but it’s clearer to explicitly log the provenance of the value rather than requiring the user to infer from whether it is different.
(It happens that in the only case so far I do this it's for initialize_adapter_from_env, which isn't changed in this PR and arguably shouldn’t change, but since you’re stating a general principle, I felt I should give my general counterargument.)
[Edit:] Several comments happened while I was writing, and I also see I wasn’t entirely clear. To be clear, what I’m asking for is that there should continue to be a from_env() function for each type-that-can-be-taken-from-environment, and it should continue to return Option<Self> (indicates whether the value was taken from environment) rather than Self (doesn't indicate).
There was a problem hiding this comment.
Re your edit: does this include compound cases (BackendOptions) or just single env-var cases?
| + backend_options: wgpu::BackendOptions { | ||
| + dx12: wgpu::Dx12BackendOptions { | ||
| + shader_compiler: wgpu::Dx12ShaderCompiler::Dxc, | ||
| + }, | ||
| + gl: wgpu::GlBackendOptions { | ||
| + gles_minor_version: wgpu::Gles3MinorVersion::Automatic, | ||
| + }, |
| /// This is equivalent to calling `from_env` on every field. | ||
| #[must_use] | ||
| pub fn from_env() -> Self { | ||
| let backends = Backends::from_env().unwrap_or_default(); |
There was a problem hiding this comment.
all other from_env just give you their default if no environment is set. It's weirdly inconsistent that Backends is the exception here
edit: DxCompiler, GlesMinorVersion, PowerPreference also give options. To an extent I get it because those are (right now) just single values that are set from a single environment variable other than conglomerates like this. But it's still super irritating to me since this feels like "family of settings that have defaults and can be set from env vars" and this PR is all about making them more similar (which is great!)
| /// Derive defaults from environment variables. See [`Self::with_env()`] for more information. | ||
| #[must_use] | ||
| pub fn from_env() -> Self { | ||
| Self::default().with_env() | ||
| } |
There was a problem hiding this comment.
as per above, I want all of these to tick like this :)
| /// - `Dxc` or `DynamicDxc` | ||
| /// - `StaticDxc` | ||
| #[must_use] | ||
| pub fn from_env() -> Option<Self> { |
There was a problem hiding this comment.
why not pick default like on the others?
There was a problem hiding this comment.
The idea is so the user can choose their own default - it feels weird having an operation that is inherently fallible but not exposing that. Maybe it's fine with with_env, the "put the fallback preference first" required of that is just weird to my brain. Maybe making this infallible can be solved through improved docs?
| /// | ||
| /// Use with `unwrap_or_default()` to get the default value if the environment variable is not set. | ||
| #[must_use] | ||
| pub fn from_env() -> Option<Self> { |
|
Appreciate you caring so much about the little api details, very much appreciate it! |
Connections
Part of the work on #4589
Description
This better organizes the backend options to allow us to add more options without the users needing to explicitly think about it
Testing
Clappy
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.