MMDLoader: Implement MMDToonMaterial. #21922

bill42362 · 2021-05-30T13:23:40Z

Related issue: #19609, #19517

Description

This PR basically follow's what described in #19609, implementing MMDToonMaterial inside MMDLoader.
Specifically, the MMDToonMaterial is extended from ShaderMaterial, use MeshPhongMaterial's shader, then merge lighting algorithms and uniforms from MeshToonShader and MeshMatcapMaterial.

Shader merging detail

Use MeshPhongMaterial's vertex shader.
Modify MeshPhongMaterial's fragment shader as following:
Declare matcap uniform.
Add gradientmap_pars_fragment. (from MMDToonMaterial)
Combine dotNL and gradient irradiances as total irradiance. Gradient irradiance came from lights_toon_pars_fragment.
(Replace lights_phong_pars_fragment with lights_mmd_toon_pars_fragment)
Add mmd_toon_matcap_fragment, which copy and modified from meshmatcap_frag.

Discussion

Move .envMap of models other than PMD to .matcap.
As @WestLangley pointed, the envMap of of my reference model (PMX) is actually point to a matcap image. So here I move .envMap to .matcap, and leave .envMap empty. Not sure what's PMD file's spec, so just leave it as is. (maybe the .envMap should referecne to scene.background?)
Add support of .specular color and .shininess value from MMD models.
Since MeshPhongMaterial support these two values, we could read these values from MMD models now. The .specular value is just copy from model file. However, the .shininess seems like a [0~1] range, and if I just map 1.0 to 30 (default of MeshPhongMaterial) it would be too bright. Here I choose to map 1.0 to 5 since it looks good on my reference model. Any suggestions?

Please leave your comment.

bill42362 · 2021-05-30T13:31:22Z

hmm..., the e2e test failed, I'll try to fix it.

takahirox · 2021-05-30T16:01:19Z

Thank you and nice work!

I would like to look into more deeply later but the basic direction looks good to me.

Would you mind if putting the screenshots of before and after? It will show to other devs how good this change is.

Here I choose to map 1.0 to 5 since it looks good on my reference model

Thanks for recovering specular support. I really wanted it. Mapping 1.0 to 5.0 sounds ok to me so far. We can modify it later if needed.

Just FYI, we directly set MMD shininess to MeshToonMaterial specular before. (MeshToonMaterial supported specular & shininess before but it has dropped them.)

https://github.com/mrdoob/three.js/blob/r111/examples/jsm/loaders/MMDLoader.js#L1065

hmm..., the e2e test failed, I'll try to fix it.

I think you need to update screenshots for MMD examples with $ npm run make-screenshot <example>

bill42362 · 2021-05-30T16:36:01Z

hmm..., my reference model is a R18 one, might not suitable to put here 😂

I'll try to find a safer PMX model and take screen shot.

The model in examples (miku_v2.pmd) is a PMD file, so the envMap part will remain unchanged.
Only specular changed.

bill42362 · 2021-05-30T16:46:24Z

It's too bright for miku_v2.pmd so it didn't pass the e2e test, i'll try to find a better specular argument.

bill42362 · 2021-05-30T18:07:47Z

e2e test screenshop updated, here is the difference:

webgl_loader_mmd
with MashToonMaterial

with MMDToonMaterial (this PR)
webgl_loader_mmd_pose
with MashToonMaterial

with MMDToonMaterial (this PR)

bill42362 · 2021-05-30T18:20:00Z

Here are some references of the .shininess map, use webgl_loader_mmd_pose screenshot as example:

Map [0-1] to [0-1] (original value in PMX model)
Map [0-1] to [0-5] (this PR)
Map [0-1] to [0-15] (this PR)
Map [0-1] to [0-30] (default MashPhongMaterial .shininess)

Mugen87 · 2021-05-31T10:59:23Z

For testing: https://raw.githack.com/bill42362/three.js/MMDLoader/examples/index.html?q=mmd

Mugen87 · 2021-05-31T11:00:30Z

IMO, the MMD examples now look a bit too shiny and overlit.

bill42362 · 2021-05-31T16:06:39Z

IMO, the MMD examples now look a bit too shiny and overlit.

Should I push a commit to set .shininess to its original value in MMD file?

takahirox · 2021-05-31T16:20:05Z

That model in the original MMD from https://ascii.jp/elem/000/000/714/714544/2/

I agree with @Mugen87, the new MMD examples looks a bit too shinny.

In the screenshot comparison @bill42362 posted, "webgl_loader_mmd
with MashToonMaterial" looks most closer to the original MMD.

But in the current Three.js examples, the model looks less shinny than your screenshot.

How did you make the screenshot? Did you use older Three.js?

takahirox · 2021-05-31T16:27:19Z

How did you make the screenshot? Did you use older Three.js?

Ah, ok I think you copied from examples/screenshots. I'm wondering why the e2e test have been passing...

https://github.com/mrdoob/three.js/blob/dev/examples/screenshots/webgl_loader_mmd.jpg

Should I push a commit to set .shininess to its original value in MMD file?

From the example model looking, I want to say yes. But it still somehow looks a bit brighter than the original MMD and our old example.

https://raw.githack.com/mrdoob/three.js/r111/examples/index.html?q=mmd#webgl_loader_mmd

bill42362 · 2021-05-31T16:39:06Z

I use npm run make-screenshot webgl_loader_mmd webgl_loader_mmd_pose for screenshots.

Here's a larger screenshot with local example server and take from my browser. And with original .shininess from MMD file.
Look like the lights are way too bright.

This one is original .shiness and make total irradiance as 0.5*(MeshToonMaterial + MashPhongMaterial).
In my PR it just MeshToonMaterial + MashPhongMaterial.
Looks much closer to the example?

bill42362 · 2021-05-31T16:58:31Z

Updated code and screenshots with the second example of my last comment.

It's always ok to revert to any commit here.

takahirox · 2021-06-02T07:02:08Z

Thanks for the screenshot. Probably it looks good.

In this PR the matcap is computed with view direction only, I'm wondering if it should be computed with light direction?

Not sure about MMD Sphere map(matcap) spec in the detail but I guess it would be good only with view direction? And we can update it later if needed.

I added comments for non-big deal like code formatting.

The last thing I want to discuss is MMDToonMaterial is extended from ShaderMaterial. It should be one of the good options. But we also have another option that MMDToonMaterial is extended from MeshPhongMaterial or MeshToonMaterial and insert and replace the shader code in onBeforeCompile like PBR specular glossiness material defined in the glTF loader. The implementation could be simpler. Have you already considered that option?

bill42362 · 2021-06-02T07:25:25Z

The last thing I want to discuss is MMDToonMaterial is extended from ShaderMaterial. It should be one of the good options. But we also have another option that MMDToonMaterial is extended from MeshPhongMaterial or MeshToonMaterial and insert and replace the shader code in onBeforeCompile like PBR specular glossiness material defined in the glTF loader. The implementation could be simpler. Have you already considered that option?

That's actually my start point, but after that I found that if I override the .onBeforeCompile in constructor of MMDToonMaterial, then other user will not able to override it again or they may break it.
I've also consider use plugin like https://github.com/Fyrestar/THREE.extendMaterial which could keep the ability of .onBeforeCompile but I' prefer use built-in functionality though. 🤔

bill42362 · 2021-06-02T09:51:54Z

Update codes according to comments except the filename comment.

WestLangley · 2021-06-02T11:46:35Z

Matcaps are view-dependent. Scene lights are view-independent. It seems odd to me to use them simultaneously.

It does seem reasonable for MeshToonMaterial to support scene lights only, and MMDToonMaterial to support matcap only.

bill42362 · 2021-06-02T12:13:35Z

Matcaps are view-dependent. Scene lights are view-independent. It seems odd to me to use them simultaneously.

Maybe that's the reason why PMX spec call it Environment index instead of Matcap index? 🤔

And in https://gist.github.com/felixjones/f8a06bd48f9da9a4539f#material-data it also noted as

Same as texture index, but for environment mapping

If we use scene light direction to compute "matcap" also, which means everything are view-independent, would that be more reasonable?

In this example it also looks like the matcap is view-independent, which the texture changes when the camera stays.
https://www.youtube.com/watch?v=Fj9ICecJkuk

WestLangley · 2021-06-03T05:26:41Z

If you render a sphere with a matcap, the rendered image does not change when the camera is rotated around the object. It is as if the "baked lights" are children of the camera, and hence move with the camera.

takahirox · 2021-06-03T07:05:29Z

That's actually my start point, but after that I found that if I override the .onBeforeCompile in constructor of MMDToonMaterial, then other user will not able to override it again or they may break it.

Hm, ok. Then let's go with ShaderMaterial so far. And let's revisit it if we encounter some problems.

Matcaps are view-dependent. Scene lights are view-independent.

Yes.

MMDToonMaterial to support matcap only.

I'm sure MMD toon material supports both scene lights and sphere map (matcap).

https://seiga.nicovideo.jp/seiga/im2077494 (in Japanese)

Even if it sounds odd, I want our MMDToonMaterial to follow the MMD toon material spec.

bill42362 · 2021-06-04T08:31:24Z

Move map filename store position from texture object to material .userData.MMD.

takahirox · 2021-06-04T08:52:41Z

examples/jsm/loaders/MMDLoader.js

@@ -1198,19 +1204,28 @@ class MaterialBuilder {
 				if ( material.textureIndex !== - 1 ) {

 					params.map = this._loadTexture( data.textures[ material.textureIndex ], textures );
+					params.map.name = material.name;


Setting texture's name as material's name is a bit weird to me... Do you want to use the name for the mapping?

uh, that's a mistake, I'll remove it.

bill42362 · 2021-06-04T09:10:08Z

Remove extra texture name assign.

takahirox · 2021-06-05T05:48:49Z

examples/jsm/shaders/MMDToonShader.js

+ * Combining steps:
+ *  * Declare matcap uniform.
+ *  * Add gradientmap_pars_fragment.
+ *  * Combine dotNL and gradient irradiances as total irradiance.


I guess this comment Combine dotNL and gradient irradiances as total irradiance is outdated?
Because irradiances are calculated only from gradient now.

takahirox · 2021-06-05T06:03:54Z

examples/jsm/loaders/MMDLoader.js

+		parameters.uniforms.specular.value = parameters.specular;
+		parameters.uniforms.shininess.value = parameters.shininess;
+		parameters.uniforms.emissive.value = parameters.emissive;
+		parameters.uniforms.gradientMap.value = parameters.gradientMap;


Should we have getters and setters like

get shininess() { return this.uniforms.shininess.value; } set shininess(value) { this.uniforms.shininess.value = value; } ...

?

Otherwise users need to be aware that new MMD toon material is based on ShaderMaterial and they need to rewrite their code from material.param = value to material.uniforms.param.value = value. I don't think we should break the compatibility.

Not use setter/getter on production before, let me try it.

bill42362 · 2021-06-05T09:52:27Z

Update comment and use setter/getters to improve compatibility.

@takahirox not sure if I'm doing right of the setter/getters, please take a look when you have time.

takahirox · 2021-06-05T22:01:42Z

examples/jsm/loaders/MMDLoader.js

+			Object.defineProperties( this, {
+
+				[ uniformName ]: {
+


I think it would be simpler to use Object.defineProperty

Object.defineProperty( this, uniformName, { get: ..., set: ... } );

takahirox · 2021-06-05T23:19:50Z

examples/jsm/loaders/MMDLoader.js

+		} );
+
+		this.uniforms = UniformsUtils.clone( MMDToonShader.uniforms );
+		for ( const uniformName in this.uniforms ) {


I first thought of using getter/setter in class like

class MMDToonMaterial { constructor(params) { ... } get foo() { return this.uniforms.foo.value; } set foo(value) { this.uniforms.foo.value = value; } }

but your Object.defineProperty approach would be simpler.

You set up getter/setter for all uniforms but uniform includes some properties which don't need to be exposed like directionalLightShadows. So I think it would be better to set up getter/setter only for properties which should be exposed.

const propertyNames = [ ... ]; for ( const propName of propertyNames ) { ... }

Would it be better if I choose like this?

const excludedProtertyNames = [ 'directionalLightShadows', ... ]; for ( const propertyName of propertyNames ) { if ( !excludedProtertyNames.includes( propertyName ) ) { ... } }

Make it default expose in case there will be more uniforms need to be exposed from MeshToon/Phong/MatcapMaterial in the future?

I think it's pros and cons. Cons of the exclude approach is if new uniforms which shouldn't be exposed will be added to toon/phong/matcap uniforms they will be exposed.

Personally I prefer mine because it would be easier to notice the need for the update. But no strong opinion.

I think yours should be better since it should be more understandable for someone wan't to modify this code.

bill42362 · 2021-06-06T08:13:12Z

Updated property exposing pattern.

takahirox

Looking good to me. Awesome work!

mrdoob · 2021-06-07T07:52:25Z

Thanks!

bill42362 added 9 commits May 30, 2021 19:48

Add prototype of MMDToonMaterial.

6d85cb6

Try to use ShaderMaterial to simulate MeshToonMaterial.

f001c87

Done simulate MeshToonMaterial with ShaderMaterial.

28c80c3

Clean up MMDToonMaterial.

709c500

Add envMap uniforms.

c41fa6d

Enable merge gradient map with phong material but got warning.

f0bd885

Enable specular and shininess in MMDToonMaterial.

19aeecd

Done combining Phong/Toon/Matcap materials.

4fb4c74

Pre PR chores.

dabe530

bill42362 changed the title ~~Mmd loader~~ MMDLoader: Implement MMDToonMaterial. May 30, 2021

fix typos.

0d066be

Mugen87 requested a review from takahirox May 30, 2021 15:51

Set color to diffuse uniform.

4e43857

bill42362 force-pushed the MMDLoader branch from cb810ae to 0487264 Compare May 30, 2021 18:01

Update e2e test screenshots.

f6524de

bill42362 force-pushed the MMDLoader branch from 0487264 to f6524de Compare May 30, 2021 18:06

Set .shininess to original and cut half irradiance.

be40619

Fix with PR comments.

e0e42fc

Modify maps file name storing position.

1a9aa64

takahirox reviewed Jun 4, 2021

View reviewed changes

mrdoob added this to the r130 milestone Jun 4, 2021

Clean extra code.

4679f0e

takahirox reviewed Jun 5, 2021

View reviewed changes

Update comment and use setter/getters to improve compatibility.

8cfed75

takahirox reviewed Jun 5, 2021

View reviewed changes

bill42362 added 2 commits June 6, 2021 14:37

Simplify defineProterties with defineProterty.

f48652f

Improve exposing property pattern.

bd7ab44

Simplify defineProterties with defineProterty.

64749f3

takahirox approved these changes Jun 6, 2021

View reviewed changes

mrdoob merged commit 5d1d306 into mrdoob:dev Jun 7, 2021

takahirox mentioned this pull request Jun 7, 2021

Implement MMD specific material in MMDLoader #19609

Closed

13 tasks

joshuaellis mentioned this pull request Jun 13, 2021

r130 three-types/three-ts-types#87

Closed

5 tasks

Xposeder mentioned this pull request Jan 10, 2022

Display errors encountered when loading mmd models in threejs higher than 0.130 #23190

Closed

MMDLoader: Implement MMDToonMaterial. #21922

MMDLoader: Implement MMDToonMaterial. #21922

Conversation

bill42362 commented May 30, 2021

bill42362 commented May 30, 2021

takahirox commented May 30, 2021 • edited Loading

bill42362 commented May 30, 2021

bill42362 commented May 30, 2021

bill42362 commented May 30, 2021

bill42362 commented May 30, 2021

Mugen87 commented May 31, 2021

Mugen87 commented May 31, 2021 • edited Loading

bill42362 commented May 31, 2021

takahirox commented May 31, 2021

takahirox commented May 31, 2021

bill42362 commented May 31, 2021

bill42362 commented May 31, 2021

takahirox commented Jun 2, 2021 • edited Loading

bill42362 commented Jun 2, 2021

bill42362 commented Jun 2, 2021

WestLangley commented Jun 2, 2021 • edited Loading

bill42362 commented Jun 2, 2021 • edited Loading

WestLangley commented Jun 3, 2021

takahirox commented Jun 3, 2021 • edited Loading

bill42362 commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bill42362 commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bill42362 commented Jun 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bill42362 Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bill42362 commented Jun 6, 2021

takahirox left a comment

Choose a reason for hiding this comment

mrdoob commented Jun 7, 2021

takahirox commented May 30, 2021 •

edited

Loading

Mugen87 commented May 31, 2021 •

edited

Loading

takahirox commented Jun 2, 2021 •

edited

Loading

WestLangley commented Jun 2, 2021 •

edited

Loading

bill42362 commented Jun 2, 2021 •

edited

Loading

takahirox commented Jun 3, 2021 •

edited

Loading

bill42362 Jun 6, 2021 •

edited

Loading