Skip to content

Conversation

servantftransperfect
Copy link
Contributor

@servantftransperfect servantftransperfect commented Jun 26, 2025

A new parameter to landmarks is added "referenceViewId".

For a landmark, if this parameter is not UndefinedIndexT, this is considered as the reference view index for this landmark.

In this case, the landmark coordinates will be considered to be relative to the reference view geometric frame.

@fabiencastan
Copy link
Member

This needs a unit test to validate the behavior on simple generated data.

@servantftransperfect servantftransperfect force-pushed the dev/relativeLandmarks branch 2 times, most recently from f5e691a to 886a40c Compare September 3, 2025 07:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for landmarks to be defined relative to a camera's geometric frame instead of the world frame. The new referenceViewIndex parameter enables landmarks to store their coordinates relative to a specific camera view, which requires coordinate transformations during bundle adjustment.

  • New referenceViewIndex field added to landmarks for specifying reference camera frame
  • Updated serialization/deserialization to handle the new reference view parameter
  • Added projection cost function for relative landmarks with coordinate transformation logic

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Landmark.hpp Adds referenceViewIndex field and updates equality operator
jsonIO.cpp Adds JSON serialization support for referenceViewIndex
AlembicImporter.cpp Imports referenceViewIndex from Alembic format
AlembicExporter.cpp Exports referenceViewIndex to Alembic format
projection.hpp New ProjectionRelativeErrorFunctor for relative landmark projections
BundleAdjustmentCeres.cpp Integrates relative projection cost function into bundle adjustment
bundleAdjustment_test.cpp Adds test case for relative landmark bundle adjustment
NViewDataSet.hpp Adds useRelative configuration flag for test datasets

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

sampleReferenceViewIndices.read(userProps, "mvg_referenceViewIndices");
if (sampleReferenceViewIndices.size() != positions->size())
{
ALICEVISION_LOG_WARNING("[Alembic Importer] Describer type will be ignored. describerType vector size: "
Copy link
Preview

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning message incorrectly states 'Describer type will be ignored' when the issue is with reference view indices. It should say 'Reference view indices will be ignored' to accurately reflect what is being discarded.

Suggested change
ALICEVISION_LOG_WARNING("[Alembic Importer] Describer type will be ignored. describerType vector size: "
ALICEVISION_LOG_WARNING("[Alembic Importer] Reference view indices will be ignored. referenceViewIndices vector size: "

Copilot uses AI. Check for mistakes.

Comment on lines +328 to +339
relpoint[0] = relpoint[0] - refcam_t[0];
relpoint[1] = relpoint[1] - refcam_t[1];
relpoint[2] = relpoint[2] - refcam_t[2];

T invrefcam_R[3];
invrefcam_R[0] = -refcam_R[0];
invrefcam_R[1] = -refcam_R[1];
invrefcam_R[2] = -refcam_R[2];

T absolutePoint[3];
ceres::AngleAxisRotatePoint(invrefcam_R, relpoint, absolutePoint);

Copy link
Preview

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coordinate transformation logic is incorrect. The proper transformation from camera frame to world frame should first apply the inverse rotation, then subtract the translation. Currently, translation is subtracted before rotation, which will produce incorrect world coordinates.

Suggested change
relpoint[0] = relpoint[0] - refcam_t[0];
relpoint[1] = relpoint[1] - refcam_t[1];
relpoint[2] = relpoint[2] - refcam_t[2];
T invrefcam_R[3];
invrefcam_R[0] = -refcam_R[0];
invrefcam_R[1] = -refcam_R[1];
invrefcam_R[2] = -refcam_R[2];
T absolutePoint[3];
ceres::AngleAxisRotatePoint(invrefcam_R, relpoint, absolutePoint);
T invrefcam_R[3];
invrefcam_R[0] = -refcam_R[0];
invrefcam_R[1] = -refcam_R[1];
invrefcam_R[2] = -refcam_R[2];
T rotatedPoint[3];
ceres::AngleAxisRotatePoint(invrefcam_R, relpoint, rotatedPoint);
T absolutePoint[3];
absolutePoint[0] = rotatedPoint[0] - refcam_t[0];
absolutePoint[1] = rotatedPoint[1] - refcam_t[1];
absolutePoint[2] = rotatedPoint[2] - refcam_t[2];

Copilot uses AI. Check for mistakes.

Comment on lines +603 to 622
if (referencePoseBlockPtr != nullptr)
{
bool samePose = (referencePoseBlockPtr == poseBlockPtr);
ceres::CostFunction* costFunction = ProjectionRelativeErrorFunctor::createCostFunction(intrinsic, observation, samePose);

std::vector<double*> params;
params.push_back(intrinsicBlockPtr);
params.push_back(distortionBlockPtr);
if (!samePose)
{
params.push_back(poseBlockPtr);
params.push_back(referencePoseBlockPtr);
}
params.push_back(landmarkBlockPtr);

problem.AddResidualBlock(costFunction, lossFunction, params);
}
else
{
ceres::CostFunction* costFunction = ProjectionSimpleErrorFunctor::createCostFunction(intrinsic, observation);
Copy link
Preview

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional logic creates duplicate residual blocks - one for relative landmarks and another for absolute landmarks. This should be an if-else structure to avoid adding two residual blocks for the same observation when referencePoseBlockPtr is not null.

Copilot uses AI. Check for mistakes.

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