Skip to content

[sfm] Fix bugs in the expanding pipeline and its local bundle adjustment - #2224

Merged
servantftransperfect merged 12 commits into
developfrom
fix/sfmBugs
Oct 28, 2026
Merged

servantftransperfect merged 12 commits into
developfrom
fix/sfmBugs

Conversation

@servantftransperfect

Copy link
Copy Markdown
Contributor

Description

Bug fixes in the expanding SfM pipeline (sfmExpanding) and the Ceres bundle adjustment. Each commit is one self-contained fix and can be reviewed on its own.

Bundle adjustment

  • Loss function ownership — the Ceres problem now owns the loss functions. The shared Huber loss is created lazily per problem, and weighted observations get their own ScaledLoss. CeresOptions stores the Huber threshold instead of a loss object.

Local bundle adjustment strategy

  • Landmark states restored on each LBA build — a landmark ignored by a previous local bundle could stay ignored for good.
  • LBA distances stored by pose id — rig views sharing a pose keep the smallest distance.
  • Exact cocardinalities — the number of landmarks shared by each pair of views was only computed when a view was first added, and from tracks rather than from the current observations. It is now updated incrementally from the landmarks that changed, at a cost linear in the number of observations.
  • Reference pose of mesh-based landmarks — a landmark whose reference pose was ignored by the local strategy could stay active, and the bundle adjustment then threw std::out_of_range.
  • Choice of local strategy — the local strategy is now used whenever an LBA policy is set (and the scene is large enough). If the policy cannot be built, it falls back to a full bundle.

Scene cleanup

  • Rig views get their own pose id when their rig pose is removed — before, they kept sharing the pose id while being flagged as independent.
  • eraseUnstablePosesAndObservations loops again when observations are removed — the result of the observation filter was overwritten by the landmark filter.

Expansion

  • Failed re-triangulation removes the old landmark — before, a track that could not be re-triangulated kept its previous landmark.
  • The expansion stops when a chunk fails — ExpansionChunk::process now returns EChunkResult (success / skipped / error). Before, a failed bundle left an unrefined scene and the expansion could still succeed. sfmExpanding still exits with a failure, but first saves the partial scene to the output, so the work can be resumed.
  • lockAllIntrinsics is honored — the option was parsed but never used. All intrinsics are now locked, so they stay constant in the bundle and do not link every view in the connexity graph.

Behavior changes for users

  • With lockAllIntrinsics, the lock flag is now saved in the output sfmData.
  • When a chunk fails, sfmExpanding fails instead of finishing with an unrefined scene. The partial scene is still written to the output file.

Tests

New sfm_lbaPolicyConnexity test:

  • ConnexityGraph_incrementalUpdate: reuses one graph across builds while landmarks are added, modified and removed, and compares it with a new graph.
  • LbaPolicyConnexity_ignoredReferencePose: a landmark whose reference pose is ignored must be ignored as well.

Both tests fail when their fix is reverted.

🤖 Generated with Claude Code

Fabien Servant and others added 12 commits October 28, 2026 08:26
Weighted observations created a ScaledLoss per residual while the problem
was configured with DO_NOT_TAKE_OWNERSHIP, so these objects were never
freed (leak at each adjust). Each ScaledLoss also took ownership of the
HuberLoss shared through CeresOptions, which would have caused a double
free if they had been released.

CeresOptions now stores the Huber parameter instead of a loss object.
Losses are created per problem through getLossFunction() and owned by the
ceres problem (shared pointers are deleted once by ceres).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The landmark state was only ever set to IGNORED and never reset, so once
a landmark was ignored by the local strategy it stayed ignored for the
rest of the expansion (resetParameterStates is no longer called once LBA
is active). Always assign the computed state.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ig pose is removed

Regression from "Non removable poses concept": rig views were set as
independent but kept the removed shared pose id. When one of them was
localized again, all its siblings appeared reconstructed with the same
(wrong) pose. Restore the previous behavior using the view id.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… are removed

The result of eraseObservationsWithMissingPoses was overwritten by
eraseLandmarksWithMissingReferenceView, so removing observations never
triggered a new pass. Poses could be left with fewer points than
minPointsPerPose. Also remove redundant flags: observations are only
removed as a consequence of removed poses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Distances were looked up by pose id but stored by view id. For rig views
sharing a pose, the pose was never found, got an infinite distance and
was ignored by the local bundle adjustment (with all its landmarks).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The option was parsed but never used, so intrinsics were always refined.
Lock all intrinsics when it is set. Locked intrinsics are constant in the
bundle adjustment and in the local strategy: their state is CONSTANT, so
the connexity graph does not link all the views sharing them, which would
otherwise make the local bundle refine the whole scene.

The lock flag is saved in the output sfmData, as the option requests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_useLBA was hard-coded to true, so with useLocalBA disabled and enough
poses, neither the local strategy nor resetParameterStates was applied
and the parameter states were left as is. The LBA policy build result
was also ignored. Use the local strategy only when a policy is set and
fall back to a full bundle if it cannot be built.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…update

Cocardinalities were only computed when a view was first added, so the
counts between existing views were never updated when landmarks were
added, re-triangulated or removed by the outlier filters. They were also
computed from tracks, counting landmarks where the view observation had
been rejected.

Keep the observers of each landmark from the previous update and only
update the pairs of landmarks which changed. The counts are now always
equal to the current landmarks observations, for a cost linear in the
number of observations. Also skip edges for views which are not in the
graph.

Add a unit test reusing a graph across builds while landmarks are added,
modified and removed, and comparing it with a new graph.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A re-evaluated track whose new landmark was rejected lost its previous
landmark, but a track whose triangulation itself failed kept it. Remove
the previous landmark in both cases. The depth prior pass may still
provide a landmark for these tracks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ExpansionChunk::process returned false both when no view could be
resected and when triangulation or bundle adjustment failed after the
scene was modified. ExpansionIteration skipped both cases, so a failed
bundle left unrefined poses and landmarks in the scene and the expansion
could finish successfully without any refinement.

Return an EChunkResult to distinguish success, skipped chunk and error.
ExpansionIteration now stops on errors, like the other callers already
did.

sfmExpanding still exits with a failure, but saves the partial scene
first, so that the work done so far is not lost. It can be resumed, as
an existing scene is bundled again on input.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nored

Mesh-based landmarks are parameterized in their reference view frame and
the bundle adjustment requires the reference pose block. When the
reference view observation had been removed, the landmark could stay
active while its reference pose was ignored by the local strategy, and
the bundle adjustment threw std::out_of_range. Take the reference pose
into account when computing the landmark state.

Add a unit test with a landmark whose reference pose is ignored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ready in use

Using the view id as the new pose id of a detached rig view could collide
with another surviving pose, making the view appear reconstructed with an
unrelated pose. Use the view id only when it is free, otherwise the next
unused id. Add a regression test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@servantftransperfect
servantftransperfect merged commit 77aea58 into develop Oct 28, 2026
4 of 5 checks passed
@servantftransperfect
servantftransperfect deleted the fix/sfmBugs branch October 28, 2026 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants