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

Update Shader and UniformsUtils #719

Merged
merged 17 commits into from
Dec 22, 2023
Merged

Conversation

vanruesc
Copy link
Contributor

Shader

Added the missing defines property to the Shader interface which is often needed for use cases involving Material.onBeforeCompile hooks. The property is optional because not all materials have defines:

UniformsUtils

The params and return values of UniformsUtils.clone() and UniformsUtils.merge() now use concrete types to avoid troubles with any in user code.

@Methuselah96
Copy link
Contributor

Don't worry about the test-examples build failing too much, working with the examples testing is not well documented, so I can fix those.

import { IUniform } from './UniformsLib.js';

export interface Shader {
export interface Shader extends WebGLProgramParameters {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename Shader to WebGLProgramParametersWithUniforms? I think Shader works too as a parameter type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was actually thinking we'd keep Shader as-is, since it's really just intended to describe the type of shaders in ShaderLib. Maybe it should be renamed to ShaderLibShader to make that more clear? I was thinking we'd export a WebGLProgramParametersWithUniforms interface from WebGLRenderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've exported WebGLProgramParametersWithUniforms from WebGLPrograms since it's used there as a param type for acquireProgram. Is there a specific reason why it should be in WebGLRenderer?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that should be fine, thanks.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Dec 21, 2023

Nice work, thanks for taking that on! Will review soon.

Copy link
Contributor

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

Thanks, that was more work than anticipated!

@vanruesc
Copy link
Contributor Author

Yeah, and those mixed types were throwing me off 😅 Thanks for the help!

@Methuselah96 Methuselah96 merged commit ae80186 into three-types:master Dec 22, 2023
3 checks passed
@vanruesc vanruesc deleted the feat/shaders branch December 22, 2023 02:12
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.

None yet

2 participants