-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaPython] Small fix & new features. #656
[SofaPython] Small fix & new features. #656
Conversation
to avoid having this code duplicated all around.
The current behavior was to not set a sys.argv if there where not argument passed in the Sofa command line. The consequence was that in the python code it was needed to do if hasattr(sys, "argv"): ... Which is rather uncommon. This Commit change this behavior by setting systematically sys.argv and additionally fix typos (ending white spaces)
This code brings an unified syntax to query members of a Node. So you can write: root.child1.child2.child3.mechanicalobject.position instead of: root.getChild("child1").getChild("child2").getChild("child3").getObject("mechanicalobject").position In case of similar names the predecessance rule is: Data, Link, Child then Object.
This returns the list of categories an object belong to (as they are defined in in the GUI & Modeler).
… no object is found. The existing code is rising an exception (without properly setting the exception cause) but other getSomething methods of the binding returns None when the path query does not match so it is better not to rise en exception and be consistant.
docstring is the proper way to document python code, so let's use it.
[ci-build][with-scene-tests] |
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.
Everything seems good to me!
2 tests are failing is this normal ? |
Cleary not. I'm investigating because I have no idea where the problem could come from. OK. I Got it...it is an interesting one so I report the details. In the code comments. |
for(const std::string& arg : arguments) { | ||
argv.push_back(arg.c_str()); | ||
} | ||
|
||
Py_SetProgramName((char*) argv[0]); // TODO check what it is doing exactly |
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.
This line is not correct... according to python documentation Py_SetProgramName want a char* to a persistant char* (it does not copy the data)).
This code is reading everywhere in memory but the memory access is not detected in our CI because the line is only exectued when runSofa is started with specific command line option (--argv). Which our CI does not.
In this PR we set-up the argument whatever the --argv option so the code is executed in the CI and (we are lucky) this make some tests to fails.
The fix is easy...we simply needs to have the program name in the PythonEnvironment and not a stack allocated variable.
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'm not sure exactly why we are setting the program name here. As per the python doc, it should be set one time before the first call to Py_Initialize and, as you said, should be persistent throughout the execution. Running a python file should not change this value as the interpreter stays the same (runSofa's internal interpreter or eventually an external one when we will be able to include the sofa lib inside an external python script).
for(const std::string& arg : arguments) { | ||
argv.push_back(arg.c_str()); | ||
} | ||
|
||
Py_SetProgramName((char*) argv[0]); // TODO check what it is doing exactly | ||
PySys_SetArgv(argv.size(), (char**)argv.data()); |
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 fear this line is also incorrect.
See PySys_SetArgvEx in python documentation: https://docs.python.org/2/c-api/init.html#c.PySys_SetArgvEx
@hugtalbot & @jnbrunet |
…t) which containes the component. This is useful to build introspection tools using python. The corresponding test is also added.
So that we can get a target without having actually a instance of an object.
ASSERT_EQ( rootNode.child1.child2.dofs.position, [[1,2,3], [4,5,6]]) | ||
|
||
## Check the 'generalized' setattr | ||
rootNode.child1.child2.dofs.position = [[2,2,2], [4,4,4]] |
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'm not sure how this line tests the setattr of a node? It seems to test the setattr of a MechanicalObject (and the getattr of the parent nodes).
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.
Correct, fixed, thanks.
[ci-build][with-scene-tests] |
✔️ to merge with these changes |
Output of the failing test on MacOS:
|
One last test is failing (indice out of range) for Python_test |
The problem was that runFile is used either in the SceneLoader and to load a controller but the behavior should be different regarding the way the sys.argv have been set. The solution was to revert (with small refactoring) to the initial runFile behavior but adding a call to setArgument in the python SceneLoader.
746f26e
to
cf4cc2a
Compare
I don't understand why the compilation is failing... |
[ci-build][with-scene-tests] |
The failing tests are now fixed... ready when the compile is over ? |
@guparan & @hugtalbot ready to merge ? |
I restart [ci-build][with-scene-tests] to take into account the recent merge in master. |
So it is time to merge. |
[ci-build][with-scene-tests] |
This PR contains a set of small changes on SofaPython that we are using in our projects.
CHANGELOG:
There is tests for all features.
This PR:
Reviewers will merge only if all these checks are true.