Skip to content

Fix Camera::setFront() having no effect - #92

Open
killerdevildog wants to merge 1 commit into
hotstreams:masterfrom
killerdevildog:fix/camera-set-front
Open

killerdevildog wants to merge 1 commit into
hotstreams:masterfrom
killerdevildog:fix/camera-set-front

Conversation

@killerdevildog

Copy link
Copy Markdown

Fixes #91.

Camera::setFront() writes the incoming vector to front, but updateView() recomputes front from yaw/pitch on the very next statement without reading the previous value. The assignment is a dead store, so the function has no effect for any input.

Since yaw/pitch are only written by the constructor and by mouseMove() (relative deltas), there is currently no way to set an absolute camera orientation from a direction vector.

Fix

Derive yaw/pitch from the direction, inverting the same euler convention updateView() already uses, so there is only one orientation representation in the class:

void Camera::setFront(const glm::vec3& _front) noexcept {
    const auto direction = glm::normalize(_front);

    pitch = glm::degrees(glm::asin(direction.y));
    yaw = glm::degrees(glm::atan(direction.z, direction.x));

    updateView();
}

Regression origin

setFront() worked when it was introduced. The original updateView() was quaternion-based and took front as an input:

front = glm::normalize(pitch_quaternion * yaw_quaternion * front);

7f7229a ("cube demo") replaced that with the euler formulation, moving front from the right-hand side to the left. The store in setFront() became dead at that point, silently and with no compiler warning.

Verification

Built a headless harness against the real Camera class (its constructor and updateView() are pure glm, no GL context required), linking camera.cpp before and after the change. Same test binary, only the object file swapped:

check before after
setFront({1,0,0})getFront() (0, -0.866, -0.5) (1, 0, 0)
forward recovered from getView() (0, -0.866, -0.5) (1, 0, 0)
9 arbitrary directions round-tripped 0/9 pass 9/9 pass
basis finite / orthogonal / unit-length pass pass
overall 11 failures 0 failures

Before the change every input returns the identical constructor default (0, -0.866, -0.5), including {0,1,0} and {0,-1,0} — the function is provably input-independent. After, all nine directions round-trip to within 1e-6.

The getView() row checks that the fix reaches the matrix that actually feeds scene_data, Frustum::fromCamera() and the cascade shadow splits, not just the member.

Notes

  • No pitch clamp is applied. It is not needed: glm::cos(glm::radians(90.0f)) is ≈ -4.37e-8 rather than exactly zero, so cross(front, world_up) does not degenerate and the straight-up/straight-down cases stay finite and exact. Verified in the table above.
  • In-tree this affects samples/gltf_viewer/gltf_viewer.cpp:110, which asks to look down +X and currently keeps the constructor defaults.
  • src/limitless/camera.cpp compiles clean under -Wall -Wextra with no new warnings.

Happy to add a Catch2 case under tests/ for this if you would like it covered by the suite.

updateView() recomputes front from yaw/pitch without reading the previous
value, so the assignment in setFront was a dead store and the function had
no effect. Set yaw/pitch from the direction instead, inverting the same
euler convention updateView uses.

Regression from 7f7229a, which replaced the quaternion updateView (where
front was an input) with the euler one (where it is an output).

Fixes hotstreams#91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Camera::setFront() is a no-op: the assignment is overwritten by updateView()

1 participant