[OpenSmalltalk/opensmalltalk-vm] Uninitialized scale in B3DShaderPlugin>>computeDirection (#434)
As the compiler tells:
``` OpenSmalltalk/opensmalltalk-vm/src/plugins/Squeak3D/Squeak3D.c:2475:7: warning: variable 'scale' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] if (!((l2vDistance == 0.0) ^~~~~~~~~~~~~~~~~~~~~~ /media/psf/Home/Smalltalk/OpenSmalltalk/opensmalltalk-vm/src/plugins/Squeak3D/Squeak3D.c:2480:42: note: uninitialized use occurs here l2vDirection[0] = ((l2vDirection[0]) * scale); ^~~~~
```
That's effectively a bug in Smalltalk source, scale might be used uninitialized:
``` computeDirection "Compute the direction for the current light and vertex" | scale | <inline: true> <var: #scale type: #double> (lightFlags anyMask: FlagPositional) ifTrue:[ "Must compute the direction for this vertex" l2vDirection at: 0 put: (litVertex at: PrimVtxPositionX) - (primLight at: PrimLightPositionX). l2vDirection at: 1 put: (litVertex at: PrimVtxPositionY) - (primLight at: PrimLightPositionY). l2vDirection at: 2 put: (litVertex at: PrimVtxPositionZ) - (primLight at: PrimLightPositionZ). "l2vDistance := self dotProductOf: l2vDirection with: l2vDirection." l2vDistance := ((l2vDirection at: 0) * (l2vDirection at: 0)) + ((l2vDirection at: 1) * (l2vDirection at: 1)) + ((l2vDirection at: 2) * (l2vDirection at: 2)). (l2vDistance = 0.0 or:[l2vDistance = 1.0]) ifFalse:[ l2vDistance := l2vDistance sqrt. scale := -1.0/l2vDistance]. l2vDirection at: 0 put: (l2vDirection at: 0) * scale. l2vDirection at: 1 put: (l2vDirection at: 1) * scale. l2vDirection at: 2 put: (l2vDirection at: 2) * scale. ] ifFalse:[ (lightFlags anyMask: FlagDirectional) ifTrue:[ l2vDirection at: 0 put: (primLight at: PrimLightDirectionX). l2vDirection at: 1 put: (primLight at: PrimLightDirectionY). l2vDirection at: 2 put: (primLight at: PrimLightDirectionZ). ]. ] ```
IMO: - the scaling is not needed when distance is 0 or 1 - the scaling should be inside the block when distance is neither 0 nor 1 - the scaling should be positive
If I decipher correctly `l2vDirection` means light to vertex direction, that is:
LV vector = OV vector - OL vector
whatever origin O, and that is what we already do in code (PrimVtxPositionX - PrimLightPositionX). So I don't understand the -1.0 here... If there is a -1.0, then we must also initialize scale to -1.0 when distance is 1.
From what I understand, this is quite a mess!
- it can be used in dotProduct with `PrimLightDirection `in `computeSpotFactor` but then, the `cosAngle ` dotProduct is negated (after negation, it is expected positive, > minCos) - it can be used in dotProduct with `PrimVtxNormal` in `shadeVertex` for diffuse part again, the `cosAngle ` dotProduct is expected to be positive, - after being copied to `l2vSpecDir` and transformed into specular direction, it is also used for specular part again, the `cosAngle ` dotProduct is expected to be positive,
The light direction (pointed by `PrimLightDirection ` offset) is used as parameter `GL_SPOT_DIRECTION ` for the `glLightfv `function.
From what I understand, it sets the main direction of **propagation** of light (from light source which is at top of the cone, to center of enlighted surface which is a circular section of the cone).
Thus, the 1st usage suggests that the l2vDirection is vertex to light and not light to vertex as the name does NOT tell! The 2nd and 3rd usage confirm this - if ever the normal of vertex is oriented outside the visible face...
This would explain the `-1*scale`...
**But** for source at infinite distance, `(lightFlags anyMask: FlagDirectional)`, the light to vertex direction is constant and equal to the `PrimLightDirection`, so that's what is simply copied in `computeDirection`... I perfectly understand that, **EXCEPT THIS**, there is no -1 scale in this branch???
So I definitely understand nothing to this code, this is highly contradictory. Maybe it only works for `(vbFlags bitAnd: VBTwoSidedLighting)` which enables a `cosAngle` of opposite sign?
Despite knowing that the code is incorrect, it's very hard to modify a code that one does not understand, there is a high risk of breaking the fragile edifice.
What is required is test cases of some sort for non regression. Des that only exist?
vm-dev@lists.squeakfoundation.org
-
Nicolas Cellier