arkavo-org / arkavo-org/VRMMetalKit

MToon debug mode 15 uses Half-Lambert while production path uses raw Lambert

Open Beginner friendly
#209 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Swift
Stars
6
Forks
2
Avg merge
18h 51m
Merged PRs (30d)
26

Description

## Summary

Surfaced by the PR #207 review (vrm-conformance issue #205 follow-up).

`MToonShader.metal` debug mode 15 (`lightingFactor` visualization, also debug mode 14 `shadowStep`) still applies a Half-Lambert remap to `NdotL` before the linearstep:

\`\`\`metal
float rawNdotL = dot(normal, -uniforms.lightDirection.xyz);
float NdotL = rawNdotL * 0.5 + 0.5; // half-Lambert remap
...
float shadowStep = linearstep(-1.0 + toony, 1.0 - toony, shading);
\`\`\`

The inline comment at line 356 literally says `// Half-Lambert to match main lighting`, but #183 removed Half-Lambert from the main production path (line 605 now uses raw `dot(normal, -lightDir)`). The debug visualization is therefore showing a different formula than the live shader runs — a faithful debug overlay should mirror production.

## Why it matters

`Tests/VRMMetalKitTests/MToonLightingFactorTests.swift` drives `debugUVs=15` to verify lighting behaviour. Because the debug path uses Half-Lambert, the test currently validates a formula production doesn't run. It can pass while the real lighting is broken (or vice versa).

## Repro

\`\`\`bash
grep -n "Half-Lambert" Sources/VRMMetalKit/Shaders/MToonShader.metal
# Debug modes 14 and 15 still have the remap; main path (around line 605) does not.
\`\`\`

## Suggested fix

Replace the `rawNdotL * 0.5 + 0.5` remap in debug modes 14 and 15 with the raw `dot(normal, -lightDirection.xyz)` so the debug visualization mirrors the production `shadowStep` / `lightingFactor`. Update the misleading comment at line 356. Re-baseline any `MToonLightingFactorTests` assertions that were tuned against the Half-Lambert range as needed.

## Out-of-scope context

This was discovered while reviewing #207 (1/π BRDF Lambert normalization for #205) — the debug paths are bypassed by the production fix, so #207 didn't need to touch them. Filing here so the debug ↔ production parity stays an explicit task.

Contributor guide

Open the contributing guide

Research direction

Start in Sources/VRMMetalKit/Shaders/MToonShader.metal, comparing debug modes 14 and 15 with the production lighting path around line 605. Run Tests/VRMMetalKitTests/MToonLightingFactorTests.swift and review assertions affected by the current remap. Done means the debug visualization uses the same lighting formula as production and the tests pass with accurate expectations.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
computer-graphics
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.