Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: Fix embedded transform errors in vtkPlusOpenIGTLinkVideoSource #874

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

BUG: Fix embedded transform errors in vtkPlusOpenIGTLinkVideoSource #874

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/PlusDataCollection/OpenIGTLink/vtkPlusOpenIGTLinkVideoSource.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,25 @@ PlusStatus vtkPlusOpenIGTLinkVideoSource::InternalUpdate()
igsioTrackedFrame trackedFrame;
igtl::MessageBase::Pointer bodyMsg = this->MessageFactory->CreateReceiveMessage(headerMsg);

igsioTransformName embeddedTransformName = this->ImageMessageEmbeddedTransformName;
if (this->ImageMessageEmbeddedTransformName.From() == this->ImageMessageEmbeddedTransformName.To())
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should do it. It goes against the rules that every transform is defined by the coordinate systems it transforms between. SomethingToSomethingTransform is always identity. Why the user cannot just invent any other name for To transform?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the true error is that setting a transform to itself should be allowed, and it should be identity?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it should be allowed (I think it is already allowed). If the matrix is anything else than identity then it should be rejected, because there is nowhere to store that in the transform repository (we need two different coordinate system names for that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the user cannot just invent any other name for To transform?

Currently if the device is connecting to a PlusServer, the embedded transform name must be specified, and must be between two valid coordinate systems, otherwise no data will be sent. This is another bug tracked in #873, but I think that using ImageToImage for the embedded transform name should be allowed in any case.

I agree, it should be allowed (I think it is already allowed).

If a tracked frame contains a transform of the form SomethingToSomething, then it will throw an error at this point when PlusOpenIGTLinkServer is copying transforms from the tracked frames to the transform repository:
https://github.com/IGSIO/IGSIO/blob/470b1553768981f3a86b9a79bf0f9016bebe74ce/Source/IGSIOCommon/vtkIGSIOTransformRepository.cxx#L151-L155

{
// If the From and To coordinate frames are the same, then don't unpack the transform.
// Unpacking the transform would cause errors such as "Setting a transform to itself is not allowed" in vtkIGSIOTransformRepository.
embeddedTransformName = igsioTransformName();
}

if (typeid(*bodyMsg) == typeid(igtl::ImageMessage))
{
if (vtkPlusIgtlMessageCommon::UnpackImageMessage(bodyMsg, this->ClientSocket, trackedFrame, this->ImageMessageEmbeddedTransformName, this->IgtlMessageCrcCheckEnabled) != PLUS_SUCCESS)
if (vtkPlusIgtlMessageCommon::UnpackImageMessage(bodyMsg, this->ClientSocket, trackedFrame, embeddedTransformName, this->IgtlMessageCrcCheckEnabled) != PLUS_SUCCESS)
{
LOG_ERROR("Couldn't get image from OpenIGTLink server!");
return PLUS_FAIL;
}
}
else if (typeid(*bodyMsg) == typeid(igtl::PlusTrackedFrameMessage))
{
if (vtkPlusIgtlMessageCommon::UnpackTrackedFrameMessage(bodyMsg, this->ClientSocket, trackedFrame, this->ImageMessageEmbeddedTransformName, this->IgtlMessageCrcCheckEnabled) != PLUS_SUCCESS)
if (vtkPlusIgtlMessageCommon::UnpackTrackedFrameMessage(bodyMsg, this->ClientSocket, trackedFrame, embeddedTransformName, this->IgtlMessageCrcCheckEnabled) != PLUS_SUCCESS)
{
LOG_ERROR("Couldn't get tracked frame from OpenIGTLink server!");
return PLUS_FAIL;
Expand Down Expand Up @@ -140,6 +148,18 @@ PlusStatus vtkPlusOpenIGTLinkVideoSource::InternalUpdate()
aSource->SetImageType(videoFrame->GetImageType());
aSource->SetInputFrameSize(trackedFrame.GetFrameSize());
}

if (embeddedTransformName.IsValid())
{
std::string transformStatusField = embeddedTransformName.GetTransformName() + igsioTrackedFrame::TransformStatusPostfix;
std::string strStatus = trackedFrame.GetFrameField(transformStatusField);
if (strStatus.empty())
{
// Transform status has not been set on the frame, and is assumed to be OK.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a valid reason for a LOG_WARNING? (or LOG_WARNING once)

trackedFrame.SetFrameTransformStatus(embeddedTransformName, TOOL_OK);
}
}

igsioFieldMapType customFields = trackedFrame.GetCustomFields();
PlusStatus status = aSource->AddItem(trackedFrame.GetImageData(), this->FrameNumber, unfilteredTimestamp, filteredTimestamp, &customFields);
this->Modified();
Expand Down