Update envmap_fragment.glsl

[NNskelly]

Per #11367 (comment)
Equirectangular U sampling update to intuitively equiangular-per-raster sampling.

A quick visual test shows no change from the old version, which worries me a bit, since I'm not getting the trig-on-paper to give identical results. If the math DOES work out provably identically, the original version is certainly more efficient.

Edit: Ok, I'm dumb. Was looking at a straight-up UV-mapped skysphere, not an actual reflection map :-P Change in a proper reflection is subtle, but I think it's there.

[WestLangley]

The current code may be incorrect, but we have to use a formula to decode the map that is consistent with the formula used to encode the map. Do you have a reference and justification for this change?

/ping @MiiBond (original author)
/ping @bhouston
/ping @Mugen87

Also, if this change is correct, you have to at least do this:

sampleUV.y = asin( clamp( flipNormal * reflectVec.y, - 1, 1 ) ) * RECIPROCAL_PI + 0.5;

You can't take the arc-sin of a quantity whose absolute value may exceed 1.

[NNskelly]

The current code may be incorrect, but we have to use a formula to decode the map that is consistent with the formula used to encode the map.

Agreed.

Do you have a reference and justification for this change?

I asked the question on the thread cited; @mrdoob asked for a pull request with the change proposal. I do not have any further reference that it is consistent with the intent of the existing codebase, hence the original question.

You can't take the arc-sin of a quantity whose absolute value may exceed 1.

Good catch, although I believe the reflectVec is already normalized, and flipNormal is explicitly defined to +/-1. If protocol is not to assume prior input sanitization may change at a future date, then adding saturate would be appropriate.

[WestLangley]

I believe the reflectVec is already normalized

Can you show where that is happening?

Roundoff can produce values greater than 1. That is why we clamp prior to calling asin.

[NNskelly]

Line 5

vec3 cameraToVertex = normalize( vWorldPosition - cameraPosition );

explicitly normalizes cameraToVertex, which is then reflected/refracted about additional vectors to get reflectVec. I'm not working in an IDE that gives me a quick way to call up reflect() or refract(), but I would not expect those to change a vector's length other than, as you note, perhaps through fp rounding error.
That said, on closer inspection, there is an else fallthrough where reflectVec is simply set to vReflect, and I cannot in fact cite that vReflect is explicitly arriving normalized.

The only place a quick find-in search of the shaders directory is showing vReflect being assigned to is envmap_vertex.glsl, where all paths likewise generate vReflect by reflecting/refracting the normalized cameraToVertex from line 9

vec3 cameraToVertex = normalize( worldPosition.xyz - cameraPosition );

but I suppose that doesn't cover all possible places where the envmap chunk may be subsequently included.

I'm not against adding the saturate if the line is otherwise correct; or it may still be a moot point if the original design intent was for images that were angular-sampled on x but linear-sampled on y.

Wolfram looks like it has less information than Wikipedia on what constitutes a "proper" equirectangular projection, but if Wikipedia is right in that the y sampling is literally (phi - phi_1) where phi is an angular degrees latitude, I would think that each raster of the map would correspond to an angular wedge of the vertical arc, and thus be more accurately sampled based on the vertical angle of the reflection vector rather than simply the vertical component of the vector (which, at least by my intuition, would vertically squish data near the poles and stretch data near the equator).
https://en.wikipedia.org/wiki/Equirectangular_projection

[WestLangley]

which, at least by my intuition, would vertically squish data near the poles and stretch data near the equator

That is exactly why I think the current code is incorrect and your proposal makes sense.

[mrdoob]

Yep. I confirm the previous code was incorrect.

Before:

After:

[mrdoob]

Thanks!