Fix negative-focal-length lens reflection; use ideal ray-slope thin-lens law (#102)#106
Merged
Merged
Conversation
The thin-lens deflection used the two-argument math.atan2(y, f) to compute the exact focusing angle. For f < 0 (diverging lenses), atan2 returns an angle in the rear hemisphere, so theta_out landed in the backward-facing cone and the reconstructed direction pointed opposite the propagation direction — the lens behaved like a mirror. Switch to the single-argument math.atan(y / f), matching the documented formula theta_out = theta_in - arctan(y/f). This keeps the deflection in (-pi/2, pi/2) for either sign of f, so a negative-focal-length lens stays transmissive and diverges the beam (virtual focus on the incident side at |f|). Applied the same fix to the Gaussian-beam q transform for parity. Adds a regression test tracing an off-axis collimated ray through a -50mm lens: it must transmit forward and diverge with slope |y/f|. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gine Replaces the arctan angle form (θ_out = θ_in − arctan(y/f)) with the ray-slope law tan(θ_out) = tan(θ_in) − y/f, building the exit direction as normalize(normal + slope·tangent). Why this is the correct ideal-lens law, not just a fix: - A parallel bundle at ANY incidence angle now focuses to a single point in the focal plane at f·tan(θ_in), for every ray height y. The arctan form only focused on-axis bundles and smeared off-axis ones (verified: ~4 mm spread at 17°, now 0). - The exit direction's forward component is always positive, so the lens is structurally transmissive for either sign of f. This fully resolves #102 (negative-f reflection) and also removes the extreme-grazing backward-ray artifact the arctan form could still produce. - Byte-identical (2e-16) to the web tracers across a grid of (f, θ_in, y), so desktop and web now agree. Converging-lens behavior at normal incidence is unchanged. transform_q derives its exit direction from the same slope law so the Gaussian ABCD matches web GeometricEngine.lensQ. Drops the now-unused math import. Adds a regression test asserting an oblique parallel bundle focuses to one point; the #102 test now documents the slope-law rationale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
beneaze
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #102.
Problem
Entering a negative focal length on a lens made it act as a mirror — rays bounced back toward the source instead of transmitting and diverging.
Root cause & fix
LensElementdeflected rays with the angle formθ_out = θ_in − arctan(y/f)(viaatan2). This both (a) sent rays backward forf < 0, and (b) is not actually the correct ideal-lens law.This PR switches to the ray-slope law used by the web engine:
Why this is the right law, not just a patch:
f·tan(θ_in), for every ray heighty. The oldarctanform only focused on-axis bundles and smeared off-axis ones (~4 mm spread at 17° incidence → now 0).normal·1 + tangent·slope, so its forward component is always positive for either sign off. This fully resolves [Bug report] Negative focal length lenses reflecting instead of diverging rays #102 and removes an extreme-grazing backward-ray artifact thearctanform could still produce.2e-16) tooptiverse-web'sGeometricEngine.thinLens/optiverse_engine._thin_lensacross a grid of(f, θ_in, y). Converging-lens behavior at normal incidence is unchanged.transform_qderives its exit direction from the same slope law, so the Gaussian ABCD matches the web'sGeometricEngine.lensQ.Verification
test_oblique_parallel_bundle_focuses_to_pointlocks in thef·tanθfocusing property (fails on the oldarctanform).test_negative_focal_length_diverges_forward([Bug report] Negative focal length lenses reflecting instead of diverging rays #102 regression):-50 mmlens transmits forward and diverges.-50 mmlens): all rays forward, zero backward.tests/raytracing/pass; ruff + mypy clean.optiverse-web
No port needed — the web engine already uses this ray-slope law; this PR brings the desktop into agreement with it.