Contributor

Member

Choose a reason for hiding this comment

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

Contributor

Choose a reason for hiding this comment

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

Nice feature. However, at db.WriteImage(image) the frame_id of the image will not be respected, which can cause big confusion (for example, for the following broken code).

Image image;
image.SetName("test");
image.SetCameraId(0);
image.SetFrameId(1);
Frame frame;
frame.SetRigId(rig.RigId());
frame.AddDataId(image.DataId());
frame.SetFrameId(database->WriteFrame(frame));
EXPECT_EQ(database->ReadImage(image.ImageId()), image);

Should we check at WriteImage that the frame_id in the input image is invalid? Or, add consistency check at either WriteImage or WriteFrame?

Contributor

Nice feature. However, at db.WriteImage(image) the frame_id of the image will not be respected, which can cause big confusion (for example, for the following broken code).

Image image;
image.SetName("test");
image.SetCameraId(0);
image.SetFrameId(1);
Frame frame;
frame.SetRigId(rig.RigId());
frame.AddDataId(image.DataId());
frame.SetFrameId(database->WriteFrame(frame));
EXPECT_EQ(database->ReadImage(image.ImageId()), image);

Should we check at WriteImage that the frame_id in the input image is invalid? Or, add consistency check at either WriteImage or WriteFrame?

Maybe align with the consistency check in Reconstruction::AddImage and Reconstruction::AddFrame? But then this will force the order of the insertion.

Contributor Author

Nice feature. However, at db.WriteImage(image) the frame_id of the image will not be respected, which can cause big confusion (for example, for the following broken code).

Image image;
image.SetName("test");
image.SetCameraId(0);
image.SetFrameId(1);
Frame frame;
frame.SetRigId(rig.RigId());
frame.AddDataId(image.DataId());
frame.SetFrameId(database->WriteFrame(frame));
EXPECT_EQ(database->ReadImage(image.ImageId()), image);

Should we check at WriteImage that the frame_id in the input image is invalid? Or, add consistency check at either WriteImage or WriteFrame?

I agree that this is confusing. However, the existing behavior is also confusing where one reads an image but the frame_id is not known?

Contributor

Nice feature. However, at db.WriteImage(image) the frame_id of the image will not be respected, which can cause big confusion (for example, for the following broken code).

Image image;
image.SetName("test");
image.SetCameraId(0);
image.SetFrameId(1);
Frame frame;
frame.SetRigId(rig.RigId());
frame.AddDataId(image.DataId());
frame.SetFrameId(database->WriteFrame(frame));
EXPECT_EQ(database->ReadImage(image.ImageId()), image);

Should we check at WriteImage that the frame_id in the input image is invalid? Or, add consistency check at either WriteImage or WriteFrame?

I agree that this is confusing. However, the existing behavior is also confusing where one reads an image but the frame_id is not known?

Indeed. Good point. This PR is a nice improvement.

Contributor Author

Should we check at WriteImage that the frame_id in the input image is invalid? Or, add consistency check at either WriteImage or WriteFrame?

I agree that this is confusing. However, the existing behavior is also confusing where one reads an image but the frame_id is not known?

Indeed. Good point. This PR is a nice improvement.

@B1ueber2y Take one more look. I tried to make it a bit safer. Now preventing to add an image with a specified frame_id if it doesn't exist.

Contributor

Choose a reason for hiding this comment

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