Repository navigation
Conversation
trackback_c declared Ymin = Ymax = 0 and used them in the add-particle volume check, so 0 < y < 0 never passed and backward tracking never added a particle. The 3DPTV original (track.c, trackback_c) declares the same zeros but then fills them with volumedimension(); that call was lost in the port. tr_new() already computes them into run_info->ymin/ymax, which trackcorr_c_loop uses; trackback_c now does too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The shared Y bounds remain uninitialized in tr_new, and the corrected behavior lacks an effective regression test.
2 open findings
What changed in this PR
Fixes backward tracking so particle addition uses the observed volume’s Y bounds.
Changes:
- Replaces impossible zero-valued Y bounds with shared tracking-run bounds.
- Documents the original backward-addition failure.
| File | Description |
|---|---|
liboptv/src/track.c |
Uses calculated Y limits during backward particle addition. |
🧠 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
+1096
to
+1097
| Ymin = run_info->ymin; | ||
| Ymax = run_info->ymax; |
| tr_new(), as trackcorr_c_loop uses them. Left at 0 here (3DPTV filled | ||
| them with volumedimension()), the add-particle volume check | ||
| 0 < y < 0 never passed and backward tracking never added. */ | ||
| Ymin = run_info->ymin; |
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.
trackback_cdeclaresYmin = Ymax = 0and uses them in the add-particle volume check:0 < y < 0is never true, so backward tracking never adds a particle, whatevertpar->addis.The 3DPTV original declares the same zeros, but then fills them:
That call was lost in the port.
tr_new()already computes these limits intorun_info->ymin/ymax, whichtrackcorr_c_loopuses.trackback_cnow uses them too.Note that this changes results: with
add = 1, backward tracking will add particles again, as in 3DPTV.Local run of all
check_*.c(minimal stand-incheck.h, nolibcheckavailable): 84 ok, the same 3 failures as master. One of those,test_volumedimension, is the flaky test fixed in #232.Found while porting the same fix to openptv2, which had copied the zeros.
🤖 Generated with Claude Code