Smalltalk › Squeak › Squeak VM

[OpenSmalltalk/opensmalltalk-vm] Uninitialized scale in B3DShaderPlugin>>computeDirection (#434)

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

2 messages Options

David T Lewis

[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.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

David T Lewis

Re: [OpenSmalltalk/opensmalltalk-vm] Uninitialized scale in B3DShaderPlugin>>computeDirection (#434)

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?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.