opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
783 stars 316 forks source link

Add support for list `Socket`s #3652

Closed nickbianco closed 8 months ago

nickbianco commented 9 months ago

Partially fixes issue #3605

Brief summary of changes

This PR adds support for list Sockets via the new macro OpenSim_DECLARE_LIST_SOCKET. This addition is motivated by current work to support Controllers in Moco. The need for this change is explained in #3605.

In addition to the macro, Component and Socket has new getConnectee() signatures that accept an index for list socket support. Most of the remaining changes are implementation details (largely based on how list Inputs are constructed).

Testing I've completed

Updated a few tests in testComponentInterface.cpp to test list Sockets. These tests include serialization/deserialization.

Looking for feedback on...

Some of the checks on the connectee paths could probably be simplified (see the TODOs).

CHANGELOG.md (choose one)


This change is Reviewable

adamkewley commented 9 months ago

Ah, I wasn't aware that Input had that behavior: it's not an API I've used!

nickbianco commented 9 months ago

After comparing to Property, I can see how it can be a bit confusing. Part of the current design might be because Sockets and Inputs add an internally managed property to handle serialization and deserialization. Although, I think we could probably bring these APIs closer together (in a future PR, of course 😄).

adamkewley commented 9 months ago

I'm unsure what's best, it isn't an API I have used (yet).

I'd vote for designing it to be logical, rather than backwards-compatible, in this case. I can't imagine that there are many downstream implementors of the virtual API (connect) outside of this repository, and I can't imagine that many codebases are using the Input API much (yet). E.g. even the Output API (arguably, easier to integrate) is mostly only used by OSC right now?

nickbianco commented 8 months ago

@adamkewley, @aymanhab ready for review.

nickbianco commented 8 months ago

@adamkewley good to merge?

nickbianco commented 8 months ago

Thanks @adamkewley and @aymanhab for the detailed reviews! This one was tricky.