subagent transcript
session 18e25685 · workflow wf_23df5b1c
delegated task
You are an adversarial verifier for a FreeCAD-wasm code review. A reviewer claims: SEVERITY: medium CATEGORY: lighting-correctness FILE: src/Gui/WasmG
▸ Magik
You are an adversarial verifier for a FreeCAD-wasm code review. A reviewer claims:
SEVERITY: medium
CATEGORY: lighting-correctness
FILE: src/Gui/WasmGLFixedFunc.cpp
SUMMARY: Normal matrix is the plain upper-left 3x3 of the modelview, not its inverse-transpose, so any non-uniform scale (SoScale, mirror/section transforms) produces incorrect lighting.
DETAIL/EVIDENCE: normalMat3 (lines 110-112) just extracts m[0,1,2,4,5,6,8,9,10]. This is correct only for rigid or uniformly-scaled modelviews. Scenes with SoScale, mirrored placements, or non-uniform transforms shade with skewed normals (wrong brightness, possibly inverted). For a simple box under orbit/pan/zoom (rigid camera) it is fine, so this does not block the box, but it is a latent correctness bug for general scenes. FIX: compute the inverse-transpose of the modelview upper-left 3x3 in JS and upload that as uNormalMat (the shader already re-normalizes). The core matrix math (mul, translate/scale/rotate, ortho, frustum, MVP = pr*mv, column-major conventions) was verified correct.
RECOMMENDATION: (none)
Independently verify by reading the actual file(s) and running git/grep as needed (repo at /home/magik6k/lcad-wasm/freecad-port/FreeCAD, toolchain at /opt/toolchains, deploy at /home/magik6k/lcad-wasm/freecad-port/deploy-gui). Default to REFUTED if you cannot confirm the evidence. Adjust severity to what the evidence actually supports.
Fable
thinking
I should check the file to confirm what's actually there.
Fable
⚙ Read {"file_path": "/home/magik6k/lcad-wasm/freecad-port/FreeCAD/src/Gui/WasmGLFixedFunc.cpp"}
Fable
thinking
Looking at the code, the `normalMat3` function extracts just the upper-left 3x3 portion of the modelview matrix, which works fine for rigid or uniform scaling. The shader then applies `normalize(uNormalMat * aNormal)` to re-normalize the result, so even with non-uniform scaling it handles the transformation correctly.
However, this is technically a latent bug since FreeCAD does use non-uniform scaling in some cases — through SoScale nodes, mirrored placements, and section transforms. For non-uniform scales, the mathematically correct approach would be to use the inverse-transpose of the 3x3 matrix. The medium severity rating seems reasonable given that it's a correctness issue for general s …
Fable
The claim is directly verifiable in the source.
Lines 110-112:
```js
normalMat3(m) { // upper-left 3x3 of modelview (good enough for rigid/uniform)
return [m[0],m[1],m[2], m[4],m[5],m[6], m[8],m[9],m[10]];
},
```
This extracts the plain upper-left 3x3 of the modelview, exactly as described. The comment even self-documents "good enough for rigid/uniform." The shader (line 87) does `normalize(uNormalMat * aNormal)`, so it re-normalizes — meaning uniform scale is handled correctly by normalization, but non-uniform scale is not (the correct transform is the inverse-transpose). The normal matrix is uploaded from this function at lines 124 (array draws) and 421 (immediate mode). The MVP math and the claim that this doesn't affect the rigid-camera box case are consistent with what I see.
Assessment on severity: the technical claim is fully correct. However, impact is limited — the lighting model here is already a crude approximation (fixed 0.35 ambient + 0.65*abs(dot) two-sided with a single directional light), and the defect only manifests for non-uniformly-scaled geometry (pure rotation, translation, and uniform scale are all correct after normalization; even a pure mirror/negative-uniform scale is unaffected). It is a genuine latent correctness bug but low practical visual impact, and it explicitly does not affect the box scene under orbit/pan/zoom. Adjusting from medium to low.
Fable
⚙ StructuredOutput {"verdict": "CONFIRMED", "reason": "Verified directly in src/Gui/WasmGLFixedFunc.cpp. normalMat3 (lines 110-112) returns the plain upper-left 3x3 of the modelview [m0,m1,m2,m4,m5,m6,m8,m9,m10], not its inverse-transpose; the comment itself …