-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
core module UBO #9040
core module UBO #9040
Conversation
fs: uniformBlock, | ||
uniformTypes: { | ||
texSize: 'vec2<f32>' | ||
} as const satisfies UniformTypes<Required<ScreenUniformProps>> |
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.
I still think this double type assertion is undesirable. You said you felt it adds clarity. If it is only needed to add clarity in some cases of more "advanced usage" that is one thing. Vut if it always needs to be done I am not so sure I can feel good about it :)
Grist for the mill I suppose...
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.
I wonder if we should review the ShaderModule
type, the UniformsT
& BindingsT
generics don't seem to be that useful IMHO. I think it would be better to have the ShaderModule
be generic on PropsT
and UniformTypesT
, as UniformsT
and BindingsT
can be obtained by filtering PropsT
.
I've had a play and we could remove the nested const satisfies UniformTypes<..>
by using the following filtering type helper:
type UniformValue = number | boolean | number[];
type FilterUniformKeys<T> = {[K in keyof T]: T[K] extends UniformValue ? K : never}[keyof T];
type UniformsOnly<T> = {[K in FilterUniformKeys<T>]: T[K]};
We then don't need the ScreenBindingProps
& ScreenUniformProps
types and can just do:
export const screenUniforms = {
name: 'screen',
fs: uniformBlock,
uniformTypes: {
texSize: 'vec2<f32>'
} as const satisfies UniformTypes<UniformsOnly<ScreenProps>>
} as const satisfies ShaderModule<ScreenProps>;
Which could all be encapsulated in luma's ShaderModule
definition
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.
Now that is some slick typescript.
- You could perhaps test all this in deck by creating a compatible type or wrapper type for
ShaderModule
in deck.gl and once you have it working we upstream it? - A possible problem is to find a name that covers both "uniforms and bindings". But I suppose
UniformsAndBindings
is a possible choice. - We want to be able to explain that shader modules mapping semantic props to shader uniforms and bindings.
export type ShaderModule<
PropsT extends Record<string, unknown> = Record<string, unknown>,
UniformsAndBindingsT extends Record<string, UniformValue | BindingValue> = Record<string, UniformValue | BindingValue>
> = {
texSrc: TextureView; | ||
}; | ||
|
||
type ScreenUniformProps = { |
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.
I really didn't anticipate this type splitting, seems like you have to go through more hoops than I anticipated. Seeing how the system is used in practice is illuminating.
For #8997
Change List
ScreenPass
& tidy types