Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Throw exception when image.SetFramePtr() sets a frame without the image listed as its data.#3540

Merged
ahojnnes merged 8 commits into
colmap:maincolmap/colmap:mainfrom
B1ueber2y:fix/add_imageB1ueber2y/colmap:fix/add_imageCopy head branch name to clipboard
Aug 7, 2025
Merged

Throw exception when image.SetFramePtr() sets a frame without the image listed as its data.#3540
ahojnnes merged 8 commits into
colmap:maincolmap/colmap:mainfrom
B1ueber2y:fix/add_imageB1ueber2y/colmap:fix/add_imageCopy head branch name to clipboard

Conversation

@B1ueber2y

@B1ueber2y B1ueber2y commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Otherwise the reconstruction data structure is broken, and the round trip wont write out the images.

Seems to catch issues in some of the test construction.

@B1ueber2y B1ueber2y changed the title Raise error when reconstruction.AddImage() add a non-existent image in the corresponding frame. Raise error when reconstruction.AddImage add a non-existent image in the corresponding frame. Aug 5, 2025
@B1ueber2y
B1ueber2y requested review from ahojnnes and sarlinpe August 5, 2025 14:33
ahojnnes
ahojnnes reviewed Aug 6, 2025
View reviewed changes

@ahojnnes ahojnnes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, LGTM apart from turning this into an exception instead of a hard crash of the process.

Comment thread src/colmap/scene/reconstruction.cc Outdated
Comment thread src/colmap/scene/reconstruction.cc Outdated
@B1ueber2y B1ueber2y changed the title Raise error when reconstruction.AddImage add a non-existent image in the corresponding frame. Raise error when image does not exist in its associated frame. Aug 6, 2025
@B1ueber2y B1ueber2y changed the title Raise error when image does not exist in its associated frame. Throw exception when image does not exist in its associated frame. Aug 6, 2025
@B1ueber2y

B1ueber2y commented Aug 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks. Added an update here 9b4d3b8 to throw exception at image.SetFramePtr as well and updated the PR title. PTAL @ahojnnes

Will follow up with the same safety check for Frame.SetRigPtr and reconstruction.AddFrame() later tonight in a separate PR.

@B1ueber2y

B1ueber2y commented Aug 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

One thing that is still extremely unsafe for the reconstruction class is that, there is no guarantee that all images in frame.DataIds() exist in the reconstruction. This is conceptually fine, as we can accept that only partial set of associated data exist in certain reconstruction, but we would need to update a lot of logic for iterating on frame.ImageIds() to include a check with reconstruction.ExistsImage(). For example, NumRegImages() will currently break when such case happens.

@B1ueber2y B1ueber2y changed the title Throw exception when image does not exist in its associated frame. Throw exception when image.SetFramePtr() sets a wrong frame. Aug 6, 2025
@B1ueber2y B1ueber2y mentioned this pull request Aug 6, 2025
Merged
@B1ueber2y B1ueber2y changed the title Throw exception when image.SetFramePtr() sets a wrong frame. Throw exception when image.SetFramePtr() sets a frame without the image listed as its data. Aug 6, 2025
@ahojnnes
ahojnnes merged commit 910177d into colmap:main Aug 7, 2025
14 checks passed
@B1ueber2y
B1ueber2y deleted the fix/add_image branch August 7, 2025 08:29
B1ueber2y added a commit that referenced this pull request Aug 7, 2025
@B1ueber2y @ahojnnes
Avoid frame having data from a sensor that does not exist in its asso…
4af57c2 
…ciated rig. (#3543)

As a follow up to #3540 for safety
improvement, this PR includes the following fixes:

* ``AddFrame`` should not add a frame with empty data as its never
useful in the reconstruction. NumData() is added for the frame class.
* ``Frame.SetRigPtr`` should set a rig that has all sensors in the frame
data supported.
* Conversely, when a rig pointer is available for the frame,
``Frame.AddDataId`` should add data that with the sensor available in
the associated rig.

Related tests are added.

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
@B1ueber2y B1ueber2y mentioned this pull request Aug 7, 2025
Merged
B1ueber2y added a commit that referenced this pull request Aug 8, 2025
@B1ueber2y @ahojnnes
Throw exception at counting registered images when an image in the fr…
8c60e36 
…ame does not exist in the reconstruction. (#3546)

Follow up to #3540 and
#3543

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
tavislocus pushed a commit to tavislocus/colmap_6dof that referenced this pull request Aug 19, 2025
@B1ueber2y @ahojnnes @tavislocus
Throw exception when image.SetFramePtr() sets a frame without the ima…
9af5f1b 
…ge listed as its data. (colmap#3540)

Otherwise the reconstruction data structure is broken, and the round
trip wont write out the images.

Seems to catch issues in some of the test construction.

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
tavislocus pushed a commit to tavislocus/colmap_6dof that referenced this pull request Aug 19, 2025
@B1ueber2y @ahojnnes @tavislocus
Avoid frame having data from a sensor that does not exist in its asso…
ab2cf56 
…ciated rig. (colmap#3543)

As a follow up to colmap#3540 for safety
improvement, this PR includes the following fixes:

* ``AddFrame`` should not add a frame with empty data as its never
useful in the reconstruction. NumData() is added for the frame class.
* ``Frame.SetRigPtr`` should set a rig that has all sensors in the frame
data supported.
* Conversely, when a rig pointer is available for the frame,
``Frame.AddDataId`` should add data that with the sensor available in
the associated rig.

Related tests are added.

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
tavislocus pushed a commit to tavislocus/colmap_6dof that referenced this pull request Aug 19, 2025
@B1ueber2y @ahojnnes @tavislocus
Throw exception at counting registered images when an image in the fr…
e538c25 
…ame does not exist in the reconstruction. (colmap#3546)

Follow up to colmap#3540 and
colmap#3543

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
ahojnnes added a commit that referenced this pull request Aug 22, 2025
@B1ueber2y @ahojnnes
Throw exception when image.SetFramePtr() sets a frame without the ima…
8fa862b 
…ge listed as its data. (#3540)

Otherwise the reconstruction data structure is broken, and the round
trip wont write out the images.

Seems to catch issues in some of the test construction.

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
ahojnnes added a commit that referenced this pull request Aug 22, 2025
@ahojnnes
Avoid frame having data from a sensor that does not exist in its asso…
3999cc1 
…ciated rig. (#3543)

As a follow up to #3540 for safety
improvement, this PR includes the following fixes:

* ``AddFrame`` should not add a frame with empty data as its never
useful in the reconstruction. NumData() is added for the frame class.
* ``Frame.SetRigPtr`` should set a rig that has all sensors in the frame
data supported.
* Conversely, when a rig pointer is available for the frame,
``Frame.AddDataId`` should add data that with the sensor available in
the associated rig.

Related tests are added.

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
ahojnnes added a commit that referenced this pull request Aug 22, 2025
@B1ueber2y @ahojnnes
Throw exception at counting registered images when an image in the fr…
da64155 
…ame does not exist in the reconstruction. (#3546)

Follow up to #3540 and
#3543

---------

Co-authored-by: Johannes Schönberger <jsch@demuc.de>
@BrewTestBot BrewTestBot mentioned this pull request Nov 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahojnnes ahojnnes ahojnnes approved these changes
@sarlinpe sarlinpe Awaiting requested review from sarlinpe

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@B1ueber2y @ahojnnes
Morty Proxy This is a proxified and sanitized view of the page, visit original site.