Skip to content

FastMath::approximateSqrt is unimplemented — Vector3::length() is essentially constant, silently breaks PHONG normalization #6

@jetpax

Description

@jetpax

Summary

FastMath::approximateSqrt (FastMath.hpp:34) and its initialEstimate helper (FastMath.hpp:27) are placeholders:

inline int64_t initialEstimate(int64_t value) {
    return 1 << 16; // Placeholder value; replace with a better estimate
}

inline int64_t approximateSqrt(int64_t value) {
    int64_t approx = initialEstimate(value);
    approx = (approx + value / approx) >> 1;
    return approx;
}

One Newton step from a constant 65536 gives a result that lands around 32768 for almost any positive input. The "approximate sqrt" is essentially independent of its argument.

Vector3::length() (Shader.hpp:96) wraps this, so any code that relies on Vector3::length() to be even loosely correct gets a wrong number. The most visible consumer is the PHONG per-pixel normalize inside drawTriangle:

auto normalLength = pixelNormal.length();
if (normalLength > 0) {
    pixelNormal = (pixelNormal * FIXED_POINT_SCALE) / normalLength;
}

With FIXED_POINT_SCALE = 1024 and a unit-magnitude normal (mag² = 1024² = 1048576), length() returns ~32768. The divide then shrinks the normal by ~33× — magnitude lands around 30. N·L in the subsequent jetShadeBrightness therefore comes out ~33× smaller than it should, brightness clamps to a small value, and PHONG-lit geometry reads as uniformly dim / nearly black across the whole visible hemisphere.

FLAT and GOURAUD aren't affected because their paths use the unmodified vertex normal (already unit-magnitude at construction) rather than running through length().

Repro

Any sphere with a PHONG material under a single DirectionalLight. The diffuse gradient that should sweep across the surface is essentially missing — surface reads as a flat dimmed colour.

Runtime trace from a debug build (arm64, Zephyr) printing N / L / dotProduct for a few sphere triangles, ambient = 0, light intensity = 255:

[js #0] N=(-1,31,0)    L=(391,537,-776) lit=16256
[js #1] N=(-38,1021,-61)  L=(391,537,-776) lit=580755   <-- after the fix below
[js #2] N=(-31,1018,-97)  L=(391,537,-776) lit=609817   <--

N magnitudes before the fix are ~30. After the fix they sit at ~1024 as the normalize loop intends, and the dot product moves into the hundreds-of-thousands range where Lambert + specular produce visible gradient.

Fix

Replace the stub body with a real sqrt. std::sqrt is a single hardware instruction on every architecture Jet targets (ARM/ARM64, Xtensa LX7 with FPU, x86), so this isn't a perf regression vs the existing one-Newton-step approximation:

inline int64_t approximateSqrt(int64_t value)
{
    if (value <= 0) return 0;
    return (int64_t)std::sqrt((double)value);
}

(initialEstimate and approximateInverseSqrt look like they also have placeholder implementations; haven't audited what else calls them.)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions