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.

Sign up for GitHub

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

Jump to bottom

feat(engine): ShaderModule.getBindings() #2104

Merged
merged 5 commits into from
Jun 20, 2024
Merged

feat(engine): ShaderModule.getBindings() #2104

merged 5 commits into from
Jun 20, 2024

Conversation

felixpalmer
Copy link
Collaborator

@felixpalmer felixpalmer commented Jun 18, 2024 •
edited
Loading

Background

ShaderModule has no support for bindings, which are needed to complete the port over to UBOs. Discussion in v9.0 PR: #2099

Change List

  • Add getBindings to ShaderModule type
  • Support in ShaderInputs & Model classes
  • Tests

ibgreen
ibgreen approved these changes Jun 18, 2024
View reviewed changes
modules/shadertools/src/lib/shader-module/shader-module.ts Outdated Show resolved Hide resolved
modules/shadertools/src/lib/shader-module/shader-module.ts Outdated
@@ -51,6 +52,7 @@ export type ShaderModule<

/** uniform buffers, textures, samplers, storage, ... */
bindings?: Record<keyof BindingsT, {location: number; type: 'texture' | 'sampler' | 'uniforms'}>;
getBindings?: (settings?: any, prevBindings?: any) => Record<string, Texture | Sampler>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment about dual functions in the other PR.

@felixpalmer
Copy link
Collaborator Author

@ibgreen @Pessimistress I've reworked the code to remove the getBindings callback and rely on splitUniformsAndBindings. As a result it is possible now to define modules as pure objects if the names of the props map to the names of the uniforms & bindings

ibgreen
ibgreen approved these changes Jun 19, 2024
View reviewed changes
modules/engine/test/shader-inputs.spec.ts Outdated Show resolved Hide resolved
ibgreen
ibgreen reviewed Jun 19, 2024
View reviewed changes
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

  • I have a sense that something should be documented about this?
  • Maybe you can help "the cause" by spending 5-10 minutes improving shadermodules docs?

@felixpalmer
Copy link
Collaborator Author

@ibgreen I've done a pass on the ShaderModule docs, and while it isn't perfect, it is less wrong. I'm not sure what to do with the bindings field though. We use it in pbr-material, but is it really required?

@felixpalmer felixpalmer merged commit a35c4dd into master Jun 20, 2024
2 checks passed
@felixpalmer felixpalmer deleted the felix/shader-module-getBindings-9.1 branch June 20, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibgreen ibgreen ibgreen approved these changes

@donmccurdy donmccurdy donmccurdy approved these changes

@Pessimistress Pessimistress Awaiting requested review from Pessimistress

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@felixpalmer @donmccurdy @ibgreen