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.

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

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

Closed
adeguet1 opened this issue May 26, 2020 · 4 comments
Closed

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

adeguet1 opened this issue May 26, 2020 · 4 comments

Comments

@adeguet1
Copy link
Contributor

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
Copy link
Contributor

leochan2009 commented May 27, 2020

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
Copy link
Contributor

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

@adeguet1
Copy link
Contributor Author

adeguet1 commented May 27, 2020

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants