-
Notifications
You must be signed in to change notification settings - Fork 327
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
Template instantiate SetFunctionBasedPath to remove spurious classes #3608
Conversation
@nickbianco this was necessary to make the ModelFactory method usable |
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.
One minor suggestion for the set name.
@@ -12,6 +12,7 @@ | |||
%include <OpenSim/Actuators/Millard2012AccelerationMuscle.h> | |||
%include <OpenSim/Actuators/McKibbenActuator.h> | |||
%include <OpenSim/Actuators/DeGrooteFregly2016Muscle.h> | |||
%template (SetFunctionBasedPaths) OpenSim::Set<OpenSim::FunctionBasedPath>; |
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.
To follow the naming convention of other sets --> FuctionBasedPathSet
?
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.
In the past, naming for classes made up for swig interface started with Set while ones that existed in C++ used the C++ name which ended in Set, so we have the mix but if you feel strongly about it or plan to typedef in C++ I will make the change so let me know.
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.
Ah okay, I was mixing up name conventions. One more question, should the template name be pluralized, or just be SetFunctionBasedPath
?
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.
I went back and forth on that and you'll see I initially had it without but looking at simulation.i I realized we most often use plurals
opensim-core/Bindings/simulation.i
Line 149 in 7703592
%template(SetMarkers) OpenSim::Set<OpenSim::Marker, OpenSim::ModelComponent>; |
Thanks @nickbianco 🥇 |
Fixes issue #0
Brief summary of changes
template instantiate SetFunctionBasedPath in interface file so swig results are usable in scripting and not bloated with unusable classes
Testing I've completed
None other than the change/fix f=to the java produced classes
Looking for feedback on...
CHANGELOG.md (choose one)
This change is