Fix for issues #54, #55 and #56 #57
Conversation
…essage. Fixed by adding NumComponents
Encoding of POLYGON caused segfault.
From inspecting code, line 478: iteration over pdata should be cdata. (caused segfault)
Encoding a vtkDataSetAttributes containing attributes with GetName()==NULL caused segfault.
Incomplete cell data in message caused segfault
…encoding. Fixed by resetting the cached igtl::PolyDataMessage before each encode.
|
Thank you very much for the contribution and apologize for my delayed response. I'll merge the fix as soon as I test it on my environment. |
| } | ||
| // Overwrite old message because we want to clear arrays from previous | ||
| // calls that will not be set anew. | ||
| this->OutPolyDataMessage = igtl::PolyDataMessage::New(); |
There was a problem hiding this comment.
Thanks for reviewing the changes and picking up this change.
The original code tries to avoid repeated New() calls, when we stream messages to the network. It won't make a difference for PolyData messages, which are usually sent for a few times in a session, but can be significant for some other message types such as transforms and images, which are often streamed to the network at 10-100fps.
So, I would stay with the old code to keep the class consistent with other message types, unless these lines cause some issues. Was there any issue caused by the original code?
There was a problem hiding this comment.
Message unpack should clear all previous contents of the message (points, cells, attributes, etc).
@jcfr Have you experienced any issue when using the current version of the code (when existing object was reused)?
There was a problem hiding this comment.
@tokjun As @lassoan mentioned, content from the previous message must be cleared before receiving a new one. I experienced a bug here when two polydata messages arrived but contained different structures. In this case the last message got content from the previous one.
If performance is an issue, one could manually clear each content of the structure instead, IF the new message don't contain such a structure. The code would be less elegant but maybe more efficient.
There was a problem hiding this comment.
Thanks for the comments, @lassoan and @christiana.
The igtl::PolyDataMessage class has Clear() function to remove the previous data. Maybe we call this instead of calling New() every time? Still you can avoid some memory reallocations.
There was a problem hiding this comment.
Currently igtl::PolyDataMessage:Clear() has about the same effect as creating a new object, as it deallocates all memory. There are two options to improve performance:
A. Change behavior of igtl::PolyDataMessage:Clear() to keep allocated memory (by not deleting the m_Points, m_Vertices, etc. just calling clear() on them)
B. Add a igtl::PolyDataMessage:Reset() function (that calls clear on m_Points, m_Vertices, etc.) and call igtl::PolyDataMessage:Reset() instead of igtl::PolyDataMessage:Clear() in vtkIGTLToMRMLPolyData::MRMLToIGTL. It is similar to the Reset() function in VTK containters, such as vtkPoints, which changes the size of the container, but keeps the capacity.
Either case, it could be useful to add a Squeeze() method, which would free up unused memory (can be implemented by a std::vector:swap operation).
I've checked other igtl::*Message implementations and it seems that only igtl::PolyDataMessage deallocates memory on Clear(). The other exception is igtl::NDArrayMessage, which does not have a Clear() method just SetSize() and that always reallocates memory. Overall, to achieve consistent behavior over the entire library, I would suggest to make the following improvements:
- in igtl::PolyDataMessage: implement option A and add a Squeeze() method
- in igtl::NDArrayMessage: replace custom memory management of the buffer by a std::vector and add a Squeeze() method
- in vtkIGTLToMRMLPolyData::MRMLToIGTL: call igtl::PolyDataMessage:Clear() before reusing the message object
There was a problem hiding this comment.
After looking at igtl::PolyDataMessage:Clear(), it seems like the method does something else than it says. There are plenty of Clear() methods in the PolyDataPointArray/CellArray/Attribute-classes, but they are not used by igtl::PolyDataMessage:Clear(). Thus I agree with @lassoan .
I can
- Withdraw this pull request.
- Create a new pull request containing all the other proposed changed, excluding the debated line.
- Implement option A from @lassoan and then in vtkIGTLToMRMLPolyData::MRMLToIGTL: call igtl::PolyDataMessage:Clear() before reusing the message object. This will replace my original proposal.
This will take a few days as I must find my Slicer-test setup somewhere on my other Mac.
Although I agree on your other suggestions (Squeeze() and igtl::NDArrayMessage..), I dont have a use case for these and thus limited ability to test them.
There was a problem hiding this comment.
Sounds good to me.
I can pull @christiana 's change as it is, and then add Clear() call when I merge it to the trunk repository. I suggest we do this first before implementing the new igtl::PolyDataMessage:Clear(), because this could at least address the current issue, and we do not need to wait for the change in OpenIGTLink library, which may take some time. Also, the current igtl::PolyDataMessage:Clear() may still have some advantage because it does not re-allocate memory for serialized data and all member variables for the class.
If we end up just re-implementing igtl::PolyDataMessage:Clear(), we will not need to change the OpenIGTLink IF module later.
So, if both @christiana and @lassoan agree, I'll merge this change ASAP, and will start working on the OpenIGTLink library.
…ata::MRMLToIGTL() to clear the content of OutPolyDataMessage, call igtl::PolyDataMessage::Clear(). igtl::PolyDataMessage::Clear() will be reimplemented based on the discussion for Issue openigtlink#57.
|
I merged the change and added igtl::PolyDataMessage:Clear() call. The code was tentatively pushed to: I have tested on Linux and will test on Mac OS X (just made sure that it sends poly data without error). Can anyone test it on Windows before merging it to the main repository? |
|
I've tested https://github.com/tokjun/OpenIGTLinkIF/tree/54-PolyData-SINTEFMedtek on Windows and it builds without any problems and works fine (sends/receives polydata). Note that the OpenIGTLinkIF version in the current Slicer nightly build does not work (it does not send the polygons), so once the 54-PolyData-SINTEFMedtek branch is merged to the master, we should update Slicer to use that. |
|
Thank you. I'll merge the code into the master repository. |
|
I updated the Git Tag in Slicer and sent a pull request. |
Fixes #4095
$ git shortlog 3ad1f67..6e297b7 --no-merges
Christian Askeland (3):
BUG: openigtlink/OpenIGTLinkIF#54 Color not supported in vtkImageData->igtl::ImageMessage.
BUG: Fixed openigtlink/OpenIGTLinkIF#55 Segfaults in encode/decode vtkPolyData
BUG: Fixed openigtlink/OpenIGTLinkIF#56 vtkPolyData encoding reuses data from last encoding.
Junichi Tokuda (1):
Instead of creating a new igtl::PolyDataMessage in vtkIGTLToMRMLPolyData::MRMLToIGTL()
to clear the content of OutPolyDataMessage, call igtl::PolyDataMessage::Clear().
igtl::PolyDataMessage::Clear() will be reimplemented based on the discussion for
Issue openigtlink/OpenIGTLinkIF#57.
From: Junichi Tokuda <[email protected]>
git-svn-id: http://svn.slicer.org/Slicer4/trunk@24802 3bd1e089-480b-0410-8dfb-8563597acbee
Fixes for several issues encountered while sending vtkPolyData and vtkImageData out from and into Slicer3D. See issues #54, #55 and #56 for details.
The pull request has been tested manually on the nightly Slicer build, MacOSX only. Another version of the code (in https://github.com/SINTEFMedtek/CustusX/tree/master/source/resource/OpenIGTLinkUtilities) has also been tested on Ubuntu14.04 and Windows 8.