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

Add type to shader material constructor #14908

Merged
merged 8 commits into from
Mar 28, 2024

Conversation

stefnotch
Copy link
Contributor

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 25, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@stefnotch
Copy link
Contributor Author

One CI failure is from

params.mainDrawWrapper.effect = engine.createEffect(
{
vertexSource: vertexCode,
fragmentSource: fragmentCode,
vertexToken: params.token,
fragmentToken: params.token,
. A property named vertexToken is being used there, and I have no idea if that's intentional or if it's a bug.
The property was undocumented.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@Popov72
Copy link
Contributor

Popov72 commented Mar 26, 2024

A property named vertexToken is being used there, and I have no idea if that's intentional or if it's a bug.
The property was undocumented.

Yes, this is intentional, it's a way of uniquely identifying a vertex+fragment shader regardless of its defines list (normally, a vertex+fragment shader pair is uniquely defined by the defines list it uses). I think you can document it as @internal at this point, users probably won't need it.

Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

Good job!

You will also have to update the WebGPUEngine.createEffect method.

packages/dev/core/src/Engines/thinEngine.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Materials/shaderMaterial.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Materials/shaderMaterial.ts Outdated Show resolved Hide resolved
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 26, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 26, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 26, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14908/merge/testResults/webgpuplaywright/index.html

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 26, 2024

@ryantrem
Copy link
Member

A property named vertexToken is being used there, and I have no idea if that's intentional or if it's a bug.
The property was undocumented.

Yes, this is intentional, it's a way of uniquely identifying a vertex+fragment shader regardless of its defines list (normally, a vertex+fragment shader pair is uniquely defined by the defines list it uses). I think you can document it as @internal at this point, users probably won't need it.

@Popov72 if vertexToken/fragmentToken are intended to be internal only, would it make sense to leave them out of IShaderPath such that they are not expected to be passed into the Effect constructor, and have the baseName parameter of the createEffect engine functions be IShaderPath & { vertexToken?: string; fragmentToken?: string}? This would be effectively assuming Engine.createEffect is intended to be for internal use, and the Effect constructor is intended to be the public API.

@Popov72
Copy link
Contributor

Popov72 commented Mar 26, 2024

Good idea @ryantrem!

@stefnotch
Copy link
Contributor Author

Good idea, I changed it to IShaderPath & { vertexToken?: string; fragmentToken?: string}.

While I was at it, I noticed that the same sort of type could be added to the compute shaders. So I introduced a IComputeShaderPath and did all the changes there as well.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 27, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14908/merge/testResults/webgpuplaywright/index.html

@RaananW RaananW merged commit 3072ce9 into BabylonJS:master Mar 28, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants