Repository navigation
Conversation
The bilinear lookup needs all four corners (ir..ir+1, iz..iz+1) inside the table, but the bounds test was ir > nr / iz > nz / index > nr*nz. It accepted the top row (iz = nz-1) and the row above it (iz = nz): there the "z+1" corners wrap into the next radial row, and at one corner the read is one element past the end. The result is used as the multimedia radial shift, so points near the top of the volume were projected with a wrong refraction factor. Now the point must lie inside the table's cells, else 0 is returned and multimed_r_nlay falls back to the iterative solve. New check test test_mmlut_matches_iterative compares the LUT with the iterative solve over the whole table: master differs by up to 0.53 (radial shift factor), the fix by < 1e-4. It skips the first radial cell, where the R = 0 row holds multimed_r_nlay's r == 0 value 1.0 rather than the r -> 0 limit (a separate issue). The same bounds bug was in openptv2's tracking kernels, where it moved predictions 16-35 px for points in that band. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
volumedimension() only widens xmax/xmin/ymax/ymin, so test_volumedimension's uninitialized locals made its result depend on stack leftovers from earlier tests (seen as xmax = 100.0 instead of 73.02). Start them empty. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Points slightly below the LUT origin still bypass fallback due to C’s truncation of negative fractional indices.
1 open finding
What changed in this PR
Fixes multimedia LUT edge access and stabilizes volume-dimension testing.
Changes:
- Restricts bilinear interpolation to valid LUT cells.
- Adds iterative-solver comparison coverage.
- Initializes volume extrema deterministically.
| File | Description |
|---|---|
liboptv/src/multimed.c |
Tightens LUT interpolation bounds. |
liboptv/tests/check_multimed.c |
Adds regression coverage and deterministic initialization. |
🧠 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
+372
to
373
| if (iz < 0 || iz + 1 > nz - 1) | ||
| return (0); |
multimed_r_nlay returns 1.0 at r == 0, a placeholder (the factor multiplies r there, so any value is exact for that point). init_mmlut tabulated it as the R = 0 row, and bilinear interpolation across the first radial cell pulled every point within rw of the glass-normal axis toward 1.0: up to 0.12 off the iterative solve on testing_fodder cam2. The factor is even in r, so the row is now (4 f(rw) - f(2 rw)) / 3. Tests: - test_mmlut_matches_iterative now covers the first radial cell (5e-4 there, the iterative solve's own 0.001 mm stop near the axis; 0.12 before); - test_init_mmLUT: data[0] is the r -> 0 limit (was asserted == 1); - test_get_mmf_mmLUT: (1,1,1) is in the first cell; 1.00382 was pulled toward the placeholder, the converged solve gives 1.005406; - test_multimed_nlay: Xq, Yq from the converged mmf (0.67565088, 0.68906505; were 0.74811917, 0.75977975). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

Two bugs in the multimedia look-up table (
mmlut) and one flaky test.1.
get_mmf_from_mmlutused the table's edge rowsThe bilinear lookup needs all four corners (ir..ir+1, iz..iz+1) inside the table, but the bounds test was
That accepts the top row (
iz = nz-1) and the row above it (iz = nz). There the "z+1" corners wrap into the next radial row, and at one corner the read isdata[nr*nz], one past the end. The result is used as the radial shift, so points near the top of the volume got a wrong refraction factor and no fallback to the iterative solve.Fix: return 0 unless all four corners are table entries.
multimed_r_nlaythen falls back to the iterative solve.2. The R = 0 row held a placeholder (commit 80aa038)
multimed_r_nlayreturns 1.0 atr == 0. That is a placeholder: the factor multiplies r there, so any value is exact for that point.init_mmluttabulated it as the R = 0 row, so interpolation across the first radial cell pulled every point withinrwof the glass-normal axis toward 1.0.Fix: the factor is even in r, so the row is now
(4 f(rw) − f(2 rw)) / 3.3.
test_volumedimensionwas flakyIt passed uninitialized
xmax/xmin/ymax/ymintovolumedimension(), which only widens them. The result depended on stack leftovers; I sawxmax = 100.0instead of 73.02. They now start empty.Tests
test_mmlut_matches_iterative(new): compares the LUT with the iterative solve over the whole table.Near the axis the reference's own fixed 0.001 mm stop limits accuracy, which is why that region has a looser tolerance.
Three existing tests encoded the first-cell error and now check converged values:
test_init_mmLUTdata[0] == 1data[0]is the r → 0 limittest_get_mmf_mmLUTat (1,1,1), R = 1.41 < rwtest_multimed_nlay(Xq, Yq)Full local run (all
check_*.ccompiled againstliboptv/srcwith a minimal stand-incheck.h, sincelibcheckisn't available here):The 2 remaining failures,
track3d_test_cavityandtrack3d_test_burgers, also fail on master.The same three fixes are in openptv2
main(76c4416a and earlier).Related: #230 (trafo
scx), #231 (orient glass fit,.addparprecision), #233 (trackback Y limits).🤖 Generated with Claude Code