-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Improve spec_v2
patterns
#20114
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
base: main
Are you sure you want to change the base?
Improve spec_v2
patterns
#20114
Conversation
TODO: migration guide
moved the blocking items to this pr, so it should be merged before the others actually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach seems like a nice cleanup, just a few questions.
@@ -62,3 +64,19 @@ impl Deref for BindGroupLayout { | |||
&self.value | |||
} | |||
} | |||
|
|||
static EMPTY_BIND_GROUP_LAYOUT: OnceLock<BindGroupLayout> = OnceLock::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so we now use an empty layout in a few places post wgpu 25, but I'm a little wary about this. I guess the set_at api is nicer than just doing a push
like we do in some places, but I'm not sure I understand why it should ever be necessary to implicitly fill in the holes in a layout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it should ever be necessary to implicitly fill in the holes in a layout?
Only with the assumption that it'll be properly filled in by other logic, like by composing specializers.
There's probably a better solution than filling_set_at
, but since specializers can't really rely on their input having a certain shape (like what the current length of the vec is) both push
and direct indexing cause problems without bounds checks or padding.
pub struct SpecializedCache<T: Specializable, S: Specializer<T>> { | ||
specializer: S, | ||
user_specializer: Option<SpecializerFn<T, S>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the thought on how this should work now? Should we copy the specialization fn pointer into whatever specializer component we create?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a simple wrapper too:
struct WithUserSpecializer<T: Specializable, S: Specializer<T>> {
specializer: S,
user_specializer: SpecializerFn<T, S>
}
I just think it's a bit niche to put in the collection itself. Also (marginally) easier to iterate on if we want to turn user_specializer
into a Box<dyn DynSpecialize>
or something later
Objective
set_target
andset_layout
will be the most used I think.push
ing to each vec, the methods pad the length of the vec if necessary and set the value directly.Specializer
s,GetBaseDescriptor
&SpecializedCache: Resource
both seem like anti-patterns, especially with dynamic materials on the horizonuser_specializer
s. If anyone needs that functionality they can easily make a wrapper for it.Solution