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

[WIP] Subclassing OpenSim classes in Python #710

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

chrisdembia
Copy link
Member

@chrisdembia chrisdembia commented Nov 18, 2015

I reproduced something that @aymanhab did a few months ago: subclassing an OpenSim class in Python and using it with a tool. OpenSim can recognize the new class, etc. I think @aseth1 and @klshrinidhi have expressed interest in this.

If we want to be able to do this, there is still a lot to iron out. It'd be nice if we could register this type (OpenSim::registerType()), but I'm running into memory ownership issues there; perhaps @aymanhab would know how to address such memory issues.


This change is Reviewable

@aymanhab
Copy link
Member

@chrisdembia Nice job indeed, I was planning to add a "Controller" instead but the same process holds. We'll have to decide what classes we should allow extending since there's a cost in code bloat due to SWIG directors. I don't think RegisterType is necessary (although possible) since that only plays into registration and XML serialization which shouldn't be needed for development/prototyping purposes.

What symptoms of memory problems did you run into? I saw nothing on AppVeyor, testComponents fails for an unrelated reason AFAIK.

@chrisdembia
Copy link
Member Author

What symptoms of memory problems did you run into?

Segfaults. I didn't try too hard to understand what was happening. It'd be good to discuss in person.

I also tried making a custom ModelComponent. I was able to implement a protected method but I was unable to call protected methods from it! This could present serious problems!

I agree that serialization/deserialization is not necessary; it would be nice icing on the cake though if we could get it to work. It'd be neat if we could give python users an equivalent of OpenSim_DECLARE_PROPERTY. I think it may be possible.

@aymanhab
Copy link
Member

@chrisdembia Thanks, why don't you add the code that causes the segfault as a test case so I can reproduce and we can discuss and make sure it stays fixed.
As you pointed out the Property macros is another big hole that we'll need to plug if we're to allow the extensions to have Properties, we should discuss in person.

@chrisdembia
Copy link
Member Author

why don't you add the code that causes the segfault as a test case so I can reproduce and we can discuss and make sure it stays fixed.

done

@aymanhab
Copy link
Member

Awesome, thanks a million @chrisdembia

@jenhicks jenhicks added this to the OpenSim 4.0 milestone Nov 24, 2015
@aymanhab
Copy link
Member

@chrisdembia Thought I'd document my findings and status of this PR since I spent some time on the investigation:

  1. The test case now has a working example of an Analysis written purely in Python that is executed during a forward simulation. That's a significant accomplishment, likely 80-90% of use cases.
  2. The cases that use "clone" rather than adopt fail and are worse examples for educational/practical purposes since you don't have access to the actual Analysis and awkward way to get a handle to it anyway due to the safeDowncast issues even if it worked. Anyway, clone fails for the following reason(s)
  • Base class Analysis is Abstract, SWIG by default doesn't wrap/expose Abstract classes (unless we use noabstract feature).
  • The clone method uses covariant return types, these were not supported for a while in languages other than C++ and as of now are not supported in SWIG even though they are in Java.

I can think of solutions around these issues (e.g. create non abstract class in the interface file that we can wrap/expose and have that as base classes e.g. PythonAnalysis etc. but that may complicate documentation etc. and will need to scale to other Components) Considering how we're handicapped by using Macros for Properties it probably make sense to stop here until there's a specific use-case/need, but let me know if you think otherwise.

@chrisdembia
Copy link
Member Author

  1. The cases that use "clone" rather than adopt fail and are worse examples for educational/practical purposes since you don't have access to the actual Analysis and awkward way to get a handle to it anyway due to the safeDowncast issues even if it worked.

I don't think we should support users calling clone(), but the rest of OpenSim may call clone() on the user-defined class (e.g., for registration). Thanks for investigating this. I agree that creating PythonAnalysis, etc. is a last resort. I think we should be able to come up with a way to implement clone() in python using the decorator. Perhaps we could sit in front of a computer together and look at the problem further?

I think we could find a workaround for properties using python decorators, so long as the addProperty methods are available (we'd have to create template instantiations, e.g., addOptionalPropertyDouble, ...). This would be fairly limiting (the available types for properties would be preset), so maybe it's not worthwhile.

@chrisdembia
Copy link
Member Author

@aymanhab , I updated this.

@aymanhab
Copy link
Member

aymanhab commented Dec 7, 2015

@chrisdembia Aside from the implementation of clone, this seems working now. Haven't touched the ModelComponent issues/tests but maybe similar fix (where base class methods that are used need to be declared as director methods). Please take a look when you have a chance. An example in Java with a pure implementation of clone in Jva is working perfectly as well.

@jenhicks jenhicks modified the milestones: Next Release, OpenSim 4.0 Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants