openigtlink / OpenIGTLink

Free, open-source network communication library for image-guided therapy
http://openigtlink.org
BSD 3-Clause "New" or "Revised" License
102 stars 184 forks source link

SensorMessage::SetXYZ methods don't set m_IsBodyPacked #224

Closed adeguet1 closed 4 years ago

adeguet1 commented 4 years ago

The methods SetValue, SetLength for igtl::SensorMessage don't update the flag m_IsBodyPacked so when trying to re-use a message, the Pack() has no effect even if the values are changed.

Meanwhile:

void TransformMessage::SetMatrix(Matrix4x4& mat)
{
  m_IsBodyPacked = false;
...
leochan2009 commented 4 years ago

Hi,

The library wasn't designed to reuse messages. you can always create a new message if your have new data coming. The messages are created using smart pointer, they will be automatically deleted after usage, so there won't be issue of memory leak. There is some inconsistency as you point out with the SetMatrix function in transformMessage, which is a legacy issue i think.

leochan2009 commented 4 years ago

@tokjun , what is your opinion about reuse the messages? should we make all message reusable or not? Thanks!

adeguet1 commented 4 years ago

First, I fixed my code to use new instances every time I send and it works fine. Thank you.

I might be abusing IGTL a bit or maybe stretching the field of applications but I'm currently trying to send small messages at rates between 100Hz and 1KHz. Modern PCs seem able to perform all the malloc/free calls without too much trouble but this seems a bit wasteful.

Maybe the best approach is not to replicate the "feature" in SetMatrix but add a mechanism for the user to explicitely force a "re"-Pack. Users who wish to re-use messages could do something like:

msg->ForcePack();

Where the method ForcePack would just unset the internal m_IsBodyPacked and then call Pack(). Alternatively you could have:

msg->UnsetIsBodyPacked();
msg->Pack();

On the other hand, when addressing issue #161 for SetMatrix, it seems that you opted to reset the flag m_IsBodyPacked automatically when the content is modified. This might required a bit more work since you will need to update all the methods that modify the message's content is all classed derived from the base message.

In any case, the issue is not critical since I can use the IGTL as-is.

leochan2009 commented 4 years ago

Hi,

Good to know your app works fine. I think there is no much computation resource been saved by implementing ForcePack(). In many cases (such as StringMessage, StatusMessage, or ImageMessage), the ForcePack() will anyway need to reallocate memory due to string length changes, new status or image size change.