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

fix: Add new shadowIntensity value to mtoon shader #1431

Closed
wants to merge 1 commit into from

Conversation

ashconnell
Copy link
Contributor

@ashconnell ashconnell commented Jul 1, 2024

Three 166 introduces a new light.shadowIntensity value (mrdoob/three.js#28588) which breaks three-vrm as the shader won't compile.

This PR updates the MToon fragment shader to use the new value.

@0b5vr 0b5vr assigned 0b5vr and unassigned 0b5vr Jul 1, 2024
@0b5vr 0b5vr self-requested a review July 1, 2024 04:23
@0b5vr 0b5vr added the bug Something isn't working label Jul 1, 2024
@0b5vr 0b5vr added this to the next milestone Jul 1, 2024
@0b5vr
Copy link
Contributor

0b5vr commented Jul 1, 2024

@ashconnell Thank you for the contribution!

We usually provide broader compatibility against past Three.js versions, so compat codes are mandatory in MToon shader codes.
Would you add a compat code following the code example below?
You would probably use the expression THREE_VRM_THREE_VERSION >= 166.

// COMPAT: pre-r156 uses a struct GeometricContext
#if THREE_VRM_THREE_REVISION >= 157
getPointLightInfo( pointLight, geometryPosition, directLight );
#elif THREE_VRM_THREE_REVISION >= 132
getPointLightInfo( pointLight, geometry, directLight );
#else
getPointDirectLightIrradiance( pointLight, geometry, directLight );
#endif

After changing the code, please make sure that the code is working correctly on both r165 and r166.

@0b5vr
Copy link
Contributor

0b5vr commented Jul 11, 2024

@ashconnell Hello, could you make the change I mentioned above?

@0b5vr
Copy link
Contributor

0b5vr commented Jul 19, 2024

Superseded by #1441

@0b5vr 0b5vr closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants