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

Various cleanups #3

Merged
merged 4 commits into from
May 13, 2020
Merged

Various cleanups #3

merged 4 commits into from
May 13, 2020

Conversation

jnbrunet
Copy link
Collaborator

@jnbrunet jnbrunet commented May 5, 2020

This PR does some cleaning and fixes some bugs:

  1. The downcasting dictionary now takes into account the template name. This was causing a bug when registering an object with different templates, only the first one registered was returned by the downcasting mechanism.

  2. Improved debug message for when the downcasting mechanisms do not find a registered class for an object deriving from Base. It will now output the tree list of parent classes for which downcasting tried to find a registered binding.

  3. When writing into a BaseData, if the data have no compatible DataTypeInfo, it will now fallback to the data's stringstream "assignment" (only if the python value of the data is convertible to a string). This allows for example the assignment of the bbox data of a node from a python List object. (bbox is a BoundingBox type with no DataTypeInfo set, but that has a data's stringstream "assignment" since it is mandatory for Data types.

  4. Switch the "custom" pointers used for the memory management of python object (that was doing nothing but to be non destructible) for a more standard way to do it with unique_ptr<T, pybind::nodelete>. This type is automatically understood by pybind11 and can be easily derived later on.

  5. There was duplicated binding code with the Binding_Simulation.h inside the Bindings.Core target and the Bindings.Simulation target.

  6. Fix a bug where the Sofa.Gui wasn't linked properly to the SofaGuiMain, resulting with no GUIs detected.

namespace sofapython3 {

void moduleAddBoundingBox(py::module& m)
{
py::class_<BoundingBoxData, BaseData,
raw_ptr<BoundingBoxData> > bbox(m, "BoundingBox");
py::class_<sofa::Data<BoundingBox>, sofa::core::objectmodel::BaseData,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About that, I'm not sure I'm for Data instead of XXData or DataXX. The drawback I see with Data is that there is no way to detect that the AbstractTypeInfo and Name/name() are there or not. But this is for a general discussion at sofa-dev meeting and outside the scope of the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you mean here.

Using an empty class BoundingBoxData here forced us to do the reinterpret_cast later on, which cause a segmentation faults half the time (weird right? I guess this is what you get for using dangerous casts).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why using an inherited type would be more/less safe. Actually this is how DataFileName is implemented does it means we cannot bind it ?

Copy link
Collaborator Author

@jnbrunet jnbrunet May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are casting an object to a class of another type.

Node::BoundingBox has been instanciated as a sofa::Data<BoundingBox>, not as a BoundingBoxData, even if you make BoundingBoxData inherit sofa::Data<BoundingBox>, it isn't making Node::BoundingBox more of a BoundingBoxData.

Forcing it to be a BoundingBoxData, which it is not, is undefined behavior. My guess would be that the two structures did not align in memory (BoundingBoxData was probably having an empty byte of padding which is common in empty struct). Hence the reinterpret_cast segmentation fault.

nfo.setScalarValue(guard.ptr, 0, py::cast<double>(o));
} else {
// Custom datatype that has no DataTypeInfo. Let's see if we can convert it to a string
Copy link
Contributor

@damienmarchal damienmarchal May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welll, this is open question but I'm not very fond of going through a string to convert the data. I prefer we spend more time implementing a proper data conversion through AbstracTypeInfo or any other general mecanism. For performance reason I prefer that this kind of code to rise an error and if someone really want to go with string he do that explicitely with myData.read(str(xxx))

Copy link
Collaborator Author

@jnbrunet jnbrunet May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very fond of going through a string to convert the data

Watch out there, you are talking about the ground base of the ObjectFactory ;-) which sucks a lot because we can't use node.addObject("MechanicalObject", position=myHugeNumpyArray.tolist()) without having to wait an eternity for it to convert it to a string and then to an array...

Anyway, I'm not sure what to do for this here, since some Data are not that trivial to convert to a compatible AbstracTypeInfo. I did this change for the BoundingBox, since it is an object that has two "array" (min and max), hence it is not a nfo.Container. Since all Data are supposed to be able to construct from a string, this change will allow us to write:

root.bbox = [x0, y0, z0, x1, y1, z2]

I guess we could also do

root.bbox.read(" ".join([str(x) for x in [x0, y0, z0, x1, y1, z2]])))

but it add yet another way to assign values to data, which start to be confusing...

@damienmarchal
Copy link
Contributor

Hi @jnbrunet

Thanks for the PR.
Appart from the implementation of 1-3 using typeName instead of className and the BoundingBox discussion it looks ok to me.

@jnbrunet
Copy link
Collaborator Author

@damienmarchal is this good to go?

@damienmarchal
Copy link
Contributor

Sorry I had no time to give it a proper look. I will try to find some asap.

@damienmarchal damienmarchal merged commit 88d5501 into sofa-framework:master May 13, 2020
@damienmarchal
Copy link
Contributor

Ok, so I had time to test the PR. Thank for it @jnbrunet,

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.

None yet

4 participants