Repository navigation
Conversation
Two bugs, both from the 3DPTV original, made the glass-interface fit
ineffective:
1. safety_x/y/z, the glass vector the numeric derivatives restore to, was
captured once before the iteration loop. Every restore then wiped the
previous iteration's glass update, so the glass never moved. Now captured
at the start of each iteration (as openptv2 already does).
2. The tilt directions e1, e2 were mistyped cross products:
e1 = (2z - 3x, 3x - z, y - 2y) instead of (1,2,3) x g = (2z - 3y, 3x - z, y - 2x)
e2_x uses e1_z*x for e1_z*y, e2_z uses e1_y*y for e1_y*x
For g along Z the wrong terms vanish. For a tilted glass e1, e2 had
components along the normal, so a tilt step moved the glass plane and one
tilt direction could not be represented. Unchanged for g along Z.
New check test test_orient_tilted_glass: glass at 20 deg about Y, start 1 deg
further. master: orient does not converge (glass stays 1 deg off); with fix 1
only: 0.021 deg off; with both: < 0.005 deg.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
write_ori wrote the seven .addpar values with "%.8f": k3 ~ 1e-9 became 0
and k2 ~ 1e-6 kept three significant digits. Now "%.10e"; read_ori's
fscanf("%lf") reads both forms, so existing files and readers are unaffected.
New check test test_write_addpar_precision: realistic k1..she survive a
write/read to 1e-9 relative.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The tangent basis remains degenerate and stale for some orientations, while .addpar output still lacks full double precision.
3 open findings
What changed in this PR
Fixes tilted-glass orientation fitting and improves .addpar serialization precision.
Changes:
- Corrects glass-normal tangent directions.
- Preserves iterative glass updates.
- Adds regression tests for tilted glass and coefficient precision.
| File | Description |
|---|---|
liboptv/src/orientation.c |
Corrects and retains glass fitting updates. |
liboptv/src/calibration.c |
Writes coefficients using exponent notation. |
liboptv/tests/check_orientation.c |
Tests tilted-glass convergence. |
liboptv/tests/check_calibration.c |
Tests small-coefficient round trips. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+350
to
+356
| /* The glass vector the numeric derivatives restore to. Captured each | ||
| iteration: captured once before the loop, every restore wiped the | ||
| previous iteration's glass update (interfflag) and the glass never | ||
| moved. */ | ||
| safety_x = cal->glass_par.vec_x; | ||
| safety_y = cal->glass_par.vec_y; | ||
| safety_z = cal->glass_par.vec_z; |
Comment on lines
+54
to
+55
| fprintf (fp, "%.10e %.10e %.10e %.10e %.10e %.10e %.10e", ap.k1, ap.k2, ap.k3, | ||
| ap.p1, ap.p2, ap.scx, ap.she); |
Comment on lines
+313
to
317
| e1_x = 2*cal->glass_par.vec_z - 3*cal->glass_par.vec_y; | ||
| e1_y = 3*cal->glass_par.vec_x - 1*cal->glass_par.vec_z; | ||
| e1_z = 1*cal->glass_par.vec_y - 2*cal->glass_par.vec_y; | ||
| e1_z = 1*cal->glass_par.vec_y - 2*cal->glass_par.vec_x; | ||
| vec_set(tmp_vec, e1_x, e1_y, e1_z); | ||
| unit_vector(tmp_vec, e1); |
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
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.
1. The glass-interface fit (
interfflag) never moved the glass, and could not fit a tilted oneBoth bugs come from the 3DPTV original.
a. Glass updates were wiped every iteration.
safety_x/y/zholds the glass vector that the numeric derivatives restore to. It was captured once, before the iteration loop, so every restore discarded the previous iteration's glass update. It is now captured at the start of each iteration.b. Mistyped tilt directions.
e1ande2are meant to be two directions perpendicular to the glass normalg:e1 = (1,2,3) × g,e2 = e1 × g. The code hadFor
galong Z the wrong terms are zero, so nothing changes in the usual setup. For a tilted glass,e1ande2had components along the normal (e2·ĝ = 0.47at 30°). A tilt step therefore also moved the glass plane, and one tilt direction could not be represented at all.New check test
test_orient_tilted_glass: glass tilted 20° about Y, start 1° further about Y.orientdoes not converge; glass stays 1° off2.
.addparprecisionwrite_oriused"%.8f", which wrote k3 ≈ 1e-9 as 0 and kept three significant digits of k2 ≈ 1e-6. It now uses"%.10e".read_ori'sfscanf("%lf")reads both forms, so existing files and readers are unaffected. New testtest_write_addpar_precision.Testing
libcheckisn't available on my machine. I compiled everyliboptv/tests/check_*.cagainst all ofliboptv/srcusing a minimal stand-incheck.hthat runs each test directly:scx)The 3 failures (
test_volumedimension,track3d_test_cavity,track3d_test_burgers) already fail on master. CI with the reallibcheckis still the authority.The same fixes are in openptv2 (
orientation.glass_basis; openptv2 already had fix 1a).🤖 Generated with Claude Code