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

[Binding/Sofa.Core] Call init() by default when creating a python object #173

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

damienmarchal
Copy link
Contributor

@damienmarchal damienmarchal commented Sep 10, 2021

The default behavior should be the most common use case while more
rare scenario should be accessible but with explicit call api (opt-in vs opt-out strategy).

Currently the default case is:

   m = r.addObject("MechanicalObject", position=[1,2,3])
   m.init()
   l.rest_position() ## Rest position is created from position at init

This is a bit weird as one may expect that once "created" in python the object is
immediately accessible in an inited state (not doing so imply user to call init() at every lines).

After the PR the default case become:

   m = r.addObject("MechanicalObject", position=[1,2,3])
   l.rest_position()

But for rare use case where it makes sense the no init behavior can still be done explicitly:

   m = r.addObject("MechanicalObject", position=[1,2,3], __noInit=True)
   l.rest_position() # Rest position is "uninited"
   m.init()          # You can control when you init the object
   l.rest_position() #

Unit tests are provided to validate the behavior.

The PR use the Sofa.future module to let users to enable/disable the behavior.
By default, the SofaPython behavior is preserved (so no auto-init)

If you want to test you just need to do

from Sofa.Lifetime import __feature__

def createScene(root)
     with __feature__("object_auto_init", True):
               root.addObject(....)

This PR depends on #184

The default behavior should be the most common use case while more
rare scenario should be accessible through explicit call.

Currently the default case is:
   m = r.addObject("MechanicalObject", position=[1,2,3])
   m.init()
   l.rest_position() ## Rest position is created from position at init

This is a bit weird as one may expect that once "created" in python the object is
immediately accessible in an inited state (not doing so imply user to call init() at every lines).

After the PR the default case become:
   m = r.addObject("MechanicalObject", position=[1,2,3])
   l.rest_position()

But for rare use case where it makes sense the no init behavior can be done like that:
   m = r.addObject("MechanicalObject", position=[1,2,3], doNoInit=True)
   l.rest_position() # Rest position is "uninited"
   m.init()          # You can control when you init the object
   l.rest_position() #

Unit tests are provided to validate the behavior.
@damienmarchal damienmarchal changed the title [SofaPython3] Call init() by default when creating a python object. [Binding/Sofa.Core] Call init() by default when creating a python object. Sep 15, 2021
@hugtalbot hugtalbot changed the title [Binding/Sofa.Core] Call init() by default when creating a python object. [Binding/Sofa.Core] Call init() by default when creating a python object Sep 15, 2021
To control if users want to activate news features in the binding.
…tures.

The mechanisme is composed of three parts:
- SofaPython3::Config (the c++ part)
- SofaPython3::Binding::Config (the python binding)

Activating an existing futurefeature:
-------------------------------------
```python
import SofaPython3.__futurefeatures__
SofaPython3.__futurefeatures__.object_auto_init = True # To activate the auto init feature.
```

Adding a new futurefeature:
---------------------------
In binding/Sofa/package/__futurefeatures__.py, add you feature at the end of the
FeatureSet::__init__ constructor.

In your c++ code you can test if the feature is activated by doing
```c++
#include <SofaPython3/Config/futurefeatures.h>
if( sofapython3::config::futurefeatures::get("myFeature") )
	....
```

In your python code you can test if the feature is activated by doing
```python
import Sofa.__futurefeatures__
if Sofa.__futurefeatures__.myFeature:
	print("The feature is activated")
```

Signed-off-by: Damien Marchal <[email protected]>
At each scene load the loaded python modules should be cleaned so they can
be reloaded.

Signed-off-by: Damien Marchal <[email protected]>
To control if users want to activate news features in the binding.
…tures.

The mechanisme is composed of three parts:
- SofaPython3::Config (the c++ part)
- SofaPython3::Binding::Config (the python binding)

Activating an existing futurefeature:
-------------------------------------
```python
import SofaPython3.__futurefeatures__
SofaPython3.__futurefeatures__.object_auto_init = True # To activate the auto init feature.
```

Adding a new futurefeature:
---------------------------
In binding/Sofa/package/__futurefeatures__.py, add you feature at the end of the
FeatureSet::__init__ constructor.

In your c++ code you can test if the feature is activated by doing
```c++
if( sofapython3::config::futurefeatures::get("myFeature") )
	....
```

In your python code you can test if the feature is activated by doing
```python
import Sofa.__futurefeatures__
if Sofa.__futurefeatures__.myFeature:
	print("The feature is activated")
```

Signed-off-by: Damien Marchal <[email protected]>
…add-no-init-behavior-at-default

# Conflicts:
#	bindings/Sofa/package/future.py
…ture

# Conflicts:
#	bindings/Sofa/tests/PythonModule_Sofa_test.cpp
…time.future

To make that consistant with the "lifetime" namespace we already have in
Sofa.
…havior-at-default

# Conflicts:
#	bindings/Sofa/tests/PythonModule_Sofa_test.cpp
…add-no-init-behavior-at-default

# Conflicts:
#	bindings/Sofa/package/lifetime.py
@guparan guparan modified the milestones: v21.06, v21.12 Oct 26, 2021
@hugtalbot
Copy link
Contributor

@AlbanOdot @jnbrunet @RobinEnjalbert what do you think of this auto-init feature?
Your input would be valuable to us!

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Apr 11, 2022

@hugtalbot so as there was no answer it will be time to plan updating and merging ;)

@damienmarchal
Copy link
Contributor Author

Related to the need for init:
sofa-framework/sofa#3012

@hugtalbot
Copy link
Contributor

hugtalbot commented Jun 17, 2022

Reviews taken into account.

However, this scene often crashes with such error:

double free or corruption (out)

########## SIG 6 - SIGABRT: usually caused by an abort() or assert() ##########

I am suspecting the engine TextureInterpolation

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Jun 17, 2022

@hugtalbot

Thanks for the feedback, can you provide a scene that crash (or elaborate a bit more).

As the risk of incompatibility with some component we don't have time to fix is high this is why this PR rely on the use of #184

     with __feature__("autoinit"): 
              blabblah

@hugtalbot
Copy link
Contributor

the scene here crashes @damienmarchal quite randomly.

What I wrote regarding the init should work fine, shouldn't it?

@damienmarchal
Copy link
Contributor Author

@hugtalbot Sorry I'm lost, what do you mean by "here" ?

@hugtalbot
Copy link
Contributor

sorry I initially posted in the wrong PR (I was targeting #204) ! sorry for perturbating this one!

@alxbilger
Copy link
Contributor

To clarify the current situation of this PR, it depends on #184 which is still in wip.
So, let's focus on #184 to move forward on this one.

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.

4 participants