-
Notifications
You must be signed in to change notification settings - Fork 325
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
[WIP] Allow live ik #2843
base: main
Are you sure you want to change the base?
[WIP] Allow live ik #2843
Conversation
…cessed by clients
…public method to allow adding live Orientation data
@chrisdembia would love to hear your thoughts/feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @aymanhab I think it'll be so neat when we can conduct IK with streaming data. I'll have more comments when the implementation is closer to what we want long-term.
Reviewed 3 of 7 files at r1.
Reviewable status: 3 of 7 files reviewed, 5 unresolved discussions (waiting on @aymanhab)
OpenSim/Common/DataQueue.h, line 52 at r2 (raw file):
{ _timeStamp = other.getTimeStamp(); };
I don't understand how this is different from the default constructor.
OpenSim/Common/DataQueue.h, line 106 at r2 (raw file):
} bool empty() const { return m_data_queue.empty();
Should this be guarded as well? What if one thread checks empty()
while another pops off the last element?
OpenSim/Simulation/BufferedOrientationsReference.h, line 74 at r2 (raw file):
* the client provided data that was queued earlier using putValues call. */ void getValues(const SimTK::State& s, SimTK::Array_<SimTK::Rotation_<double>>& values) const override;
This no longer seems like the appropriate interface for InverseKinematicsSolver
to use. I would think InverseKinematicsSolver
would want to invoke something like getNext()
on the reference.
OpenSim/Simulation/InverseKinematicsSolver.h, line 233 at r2 (raw file):
// The orientation reference values and weightings BufferedOrientationsReference _orientationsReference;
We would want InverseKinematicsSolver
to use the base class reference (the abstract interface).
OpenSim/Simulation/InverseKinematicsSolver.cpp, line 58 at r2 (raw file):
_orientationsReference(orientationsReference) { // InverseKinematicsSolver has its own internal copy of the References to track _markersReference = markersReference;
Might as well move initialization of _markersReference
outside of the body of the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @aymanhab and @chrisdembia)
OpenSim/Common/DataQueue.h, line 106 at r2 (raw file):
Previously, chrisdembia (Christopher Dembia) wrote…
Should this be guarded as well? What if one thread checks
empty()
while another pops off the last element?
As of now this called exclusively from a guarded block already, could make the method private and add a separate public method that guards if needed.
OpenSim/Simulation/BufferedOrientationsReference.h, line 74 at r2 (raw file):
Previously, chrisdembia (Christopher Dembia) wrote…
This no longer seems like the appropriate interface for
InverseKinematicsSolver
to use. I would thinkInverseKinematicsSolver
would want to invoke something likegetNext()
on the reference.
@chrisdembia if refactoring the solver, we can definitely use a pointer/reference to the *Reference object(s) and use a generic interface. As it stands an actual object/copy is maintained inside the solver. Depending on how much we want to keep/change the interface or maintain backward compatibility I agree a getNextValues() that returns both time and Values would be the better long term solution.
…RowVector from RowVectorView in Scripting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 9 files reviewed, 5 unresolved discussions (waiting on @aymanhab and @chrisdembia)
OpenSim/Common/DataQueue.h, line 106 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
As of now this called exclusively from a guarded block already, could make the method private and add a separate public method that guards if needed.
I like that idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 9 files reviewed, 6 unresolved discussions (waiting on @aymanhab and @chrisdembia)
Bindings/SWIGSimTK/BigMatrix.h, line 750 at r3 (raw file):
VectorView_<ELT> operator()(int j) const {return col(j);} VectorView_<ELT> operator()(int j) {return updCol(j);} #ifndef SWIG
Does this cause SWIG to generate more warnings?
Fixes issue #<issue_number>
Brief summary of changes
Introduce BufferedOrietationsReference that maintains a Queue of live data that a usar can add to.
Testing I've completed
Looking for feedback on...
CHANGELOG.md (choose one)
This change is