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

BREAKING: WebGPU support (MToonNodeMaterial) #1384

Open
wants to merge 39 commits into
base: dev-v3
Choose a base branch
from
Open

BREAKING: WebGPU support (MToonNodeMaterial) #1384

wants to merge 39 commits into from

Conversation

0b5vr
Copy link
Contributor

@0b5vr 0b5vr commented Mar 25, 2024

This PR supports WebGPURenderer by adding MToonNodeMaterial, which is a NodeMaterial port of MToonMaterial.

After the change is published, you can use three-vrm on WebGPU by using MToonNodeMaterialLoaderPlugin.
To use the loader, specify the loader in the option of VRMLoaderPlugin.
See the webgpu-dnd.html example for details.

BREAKING

  • gltf.userData.vrmMToonMaterials might have materials other than MToonMaterial (namely, MToonNodeMaterial in this case).
    • This should not affect many users since the interface of MToonNodeMaterial is almost identical to MToonMaterial.

TODOs

- The type parameter of `THREE.Object3D` is no longer required (used to be `THREE.Object3D<THREE.Event>`
- `THREE.UniformsUtils.merge` now has more appropriate typing which breaks the existing code, workarounded with `as any`
Starting from r155, the light unit is set to physically based one by default.
We need to multiply the directional light intensity by PI because of this change.

See: mrdoob/three.js#26392
also removed an impossible compat code path
Built the MToon shader using NodeMaterial feature

I had to add `three/examples/jsm/nodes/Nodes.js` to rollup's `external` config. Is the solution legit?

We have a problem on the current `@types/three`, I had to patch-package. Some of them are already addressed as PR on `three-ts-types` repo

I applied temporary changes to several example html files, this should be reverted before merge
`parameters` have `depthWrite = false` when it's transparent, we have to overwrite this
It seems model normal was multiplied by 100 in UniGLTF 1.27, 1.28. Low confidence
also emit a warning if the model contains vertex colors
The outline was too bright when the outlineLightingMix is zero
This prevents opaque / cutout materials to go white
…LightingMix factor is 1

it should be multiplied by (1.0 / PI)
outline didn't respect alpha when outlineLightingMixFactor is not 1
outlineWidthMode is not required when it's not outline pass

also add comma
We should have the parametric rim result to a temporary variable
WebGPURenderer creates shader variants for each skeleton uuid
so we want to unify skeletons as much as possible
If there is a superset skeleton, reuse it
This reverts commit 33fc057.

boneInverse might be different between skeletons
and the new method does not work well with such cases.
The `Type of property 'cache' circularly references itself in mapped type` still persists
The error disappears once I bump TypeScript to v5.1.3 or above
revert several examples back to WebGL

`three/examples/jsm/*` now referred by `three/addons/*`; see `tsconfig.json` and `rollup.config.js`

`RotateUV` doesn't work properly with UV Animation Mask Texture on r162. Spin using a handmade rotation matrix instead
tested using VRMC_materials_mtoon_UV_Animation_Test.vrm
This is an option to decrease initial rendering time when the model has many materials
If you want to use three-vrm on WebGPU, use `MToonNodeMaterialLoaderPlugin` by specifying the loader in the option of `VRMLoaderPlugin`.
See the `webgpu-dnd.html` example for details

BREAKING: MToonMaterialLoaderPlugin might put materials other than MToonMaterial (namely, MToonNodeMaterial in this case) to `gltf.userData.vrmMToonMaterials`.
This shouldn't affect many users since the interface of MToonNodeMaterial is almost identical to MToonMaterial
@0b5vr 0b5vr added the enhancement New feature or request label Mar 25, 2024
@0b5vr 0b5vr self-assigned this Mar 25, 2024
@0b5vr 0b5vr added the Breaking Change Choose wisely label May 14, 2024
0b5vr added 3 commits May 14, 2024 17:16
…arately

We need to fix the issue from the `@types/three` side someday

This change and the previous r164 bump removes all patches to `@types/three`, the commit also removes `patch-package` 🎉
@0b5vr 0b5vr marked this pull request as ready for review May 14, 2024 09:58
@0b5vr
Copy link
Contributor Author

0b5vr commented May 14, 2024

Ready to merge!

We are going to bump the major since the breaking change occurs.

@0b5vr
Copy link
Contributor Author

0b5vr commented May 15, 2024

日本語で恐縮ですが、NodeMaterialを継承したマテリアルのコードを読む上で、NodeMaterialの動作の流れ(r164現在のもの)を以下のページにまとめてあります。レビューの際にご活用ください。

https://scrapbox.io/0b5vr/Three.js:_NodeMaterial

@0b5vr 0b5vr changed the title WIP: BREAKING: WebGPU support (MToonNodeMaterial) BREAKING: WebGPU support (MToonNodeMaterial) May 15, 2024
@ke456-png
Copy link
Contributor

The rendering result differs when there is no texture and only a normal map.

examples/feature-test
image

webgpu-feature-test
image

@ke456-png
Copy link
Contributor

Perhaps MToonMaterial was wrong?

const useUvInFrag =
this.map !== null ||
this.emissiveMap !== null ||
this.shadeMultiplyTexture !== null ||
this.shadingShiftTexture !== null ||
this.rimMultiplyTexture !== null ||
this.uvAnimationMaskTexture !== null;

 const useUvInFrag = 
...
      this.uvAnimationMaskTexture !== null ||
      this.normalMap !== null;

@0b5vr 0b5vr added this to the v3 milestone May 20, 2024
@0b5vr 0b5vr changed the base branch from dev to dev-v3 May 20, 2024 02:49
@0b5vr
Copy link
Contributor Author

0b5vr commented May 20, 2024

The rendering result differs when there is no texture and only a normal map.

It seems to be, yes! The blue ball in the example should have bumps on the surface since the normal map is assigned.
I will fix this in another PR.

@ke456-png
Copy link
Contributor

If possible, please incorporate this change.
25cc2ba

Comment on lines +209 to +212
// Set alpha to 1 if it is not transparent
if (this.transparent === false) {
Nodes.diffuseColor.a.assign(1.0);
}
Copy link
Contributor

@ke456-png ke456-png May 22, 2024

Choose a reason for hiding this comment

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

I need to confirm the conditions for making it opaque.

#ifdef OPAQUE
diffuseColor.a = 1.0;
#endif

https://github.com/mrdoob/three.js/blob/687a4caf8c5d633df691a90369c9c185a4acb0fc/src/renderers/webgl/WebGLPrograms.js#L250
opaque: material.transparent === false && material.blending === NormalBlending && material.alphaToCoverage === false,

@0b5vr
Copy link
Contributor Author

0b5vr commented May 24, 2024

Resolved conflict with dev-v3, it's now ready to build using esbuild.
I don't have to explicitly specify three/addons/nodes/Nodes.js, esbuild seems to handle this by only specifying three.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Choose wisely enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants