Remove property maps

Hi, the MeshWrapper class defines some property maps, which maps the names of all the python-defined properties to the corresponding property handles.

I guess they are redundant, since you can do everything with the core OpenMesh functions.

This MR has an implementation using only OpenMesh functions.

Seems to work fine in Debug mode but fails in Release mode.

Why is this necessary?

The idea was, to check if a property with the given name has the type of py::none and not something like a C++ Vector3d (e.g. vertex positions).

But OpenMesh disables the check in Release mode. (see PropertyContainer.hh and every entry with OM_FORCE_STATIC_CAST)

changed this line in version 2 of the diff

@moeller: Thanks for the merge request. Please try to keep the indentation consistent with the original style of the files.

@adielen, @born: Any further comments?

Found the problem. In Release mode, OM_FORCE_STATIC_CAST is defined in openmesh. This will prevent type checking when getting a property and the new deepcopy function tries to copy every property non-python, ignoring the type.

Will post a patch later.

Created an isse: OpenMesh#54.

Will wait for the answer before trying to fix it.

edit: improved ref

Something that I noticed is that this makes it possible to do the following:

>>> import openmesh as om
>>> mesh = om.read_trimesh('bunny.ply')
>>> mesh.vertex_property('v:points', om.VertexHandle(0))
Segmentation fault

Of course this can be handled with additional checks.

@adielen Should be the same issue with the failing tests (missing type check on "get_property")

v:points is something like Vec3d and you request a vertex property of type py::none (btw. why py::none and not py::object?).

Unlike boost::python::object, pybind::object does not default construct to None and we wanted new properties to be None if not explicitly initialized by the user.

ah, thx :)

added 1 commit

• cb8872e2 - use property directly instead of requesting a handle for property access

Compare with previous version

ok, not exactly a workaround. Since we iterate through the properties already with the prop iterator, why should we request a PropertyHandle? So, i am doing my own type checking directly on the property.

This also results in less function calls and string cmps.

added 1 commit

• ee1c7978 - more expressive code

Compare with previous version

It looks like py_has_property(), py_remove_property() and py_prop_on_demand() all have the same problem that py_deepcopy_prop() had, namely that get_property_handle() does not check the property type.

This causes the following problems:

>>> import openmesh as om
>>> mesh = om.TriMesh()
>>> mesh.has_vertex_property("v:points")
True # should be false

>>> mesh = om.read_trimesh("bunny.ply")
>>> mesh.remove_vertex_property("v:points")
>>> mesh.points()
Segmentation fault

>>> mesh = om.read_trimesh("bunny.ply")
>>> mesh.set_vertex_property("v:points", om.VertexHandle(0), [1,2,3])
Segmentation fault

Maybe wait and see what happens with OpenMesh#54? Otherwise we will need our own version of get_property_handle() that checks the type.

Yes, we should definetly wait for OpenMesh#54, all functions based on get_property_handle() are broken, which are all except of deepcopy.

The code for type checking would be something like this:

template<typename PropHandle>
bool is_py_type(PropHandle p) const
{
  const BaseProperty* bp = &Mesh::property(std::move(p));
  return dynamic_cast<const PropertyT<py::none>*>(bp) != nullptr;
}

I really hope OpenMesh provides some functionallity.

added 1 commit

• f0576f91 - add type checking + test

Compare with previous version

Since OpenMesh seems to take some time, fixing the problem with type checking on all platforms (See OpenMesh#54 OpenMesh!176 (closed) and OpenFlipper#136 ) and it is questionable, when or if it is fixed, i added the type checking.

I'm not entirely happy with the notion that we can have python- and c++-properties.

In my opinion in the case where you are using openmesh-python, c++-properties should be of no concern.

Is access to properties like v:points really necessary?

Apart from that some functions that rely on the py_prop_is_py_type silently fail, which is not ideal.

@adielen what are your thoughts on this?

@lim It's neither necessary nor desired. The last commit (f0576f91) is in fact an attempt to hide v:points from python.

It does not work though because get_property_handle() returns the first property with the requested name. For v:points this is the C++ property. The python property with the same name is lost. To make this work we need our own version of get_property_handle() that checks the type and keeps searching after a C++ property with the correct name is found.

I don't think it's worth it though. The property maps may be redundant, but they are clean and simple. I also think the overhead is negligible.

added 12 commits

• f0576f91...56c54adb - 4 commits from branch master
• 6874d381 - substitutes prop maps with openmesh internal managment
• deae6b47 - use property directly instead of requesting a handle for property access
• 33dbbe4c - more expressive code
• 995f13d4 - add type checking + test
• bd925aa4 - Revert "add type checking + test"
• 74ba65b0 - add test accessing already existing property (with c++ type)
• 66df9d2c - enable type check in release mode
• f3a3221f - Merge branch 'remove-property-map' of…

Compare with previous version

get_property_handle redirects now to a version with type checking, when type checking is in OM disabled (default in OpenMesh in release, you cannot switch that off). This can be done, because OpenMesh is linked statically into OpenMesh-Python and no other library access it. It is also save, when OpenMesh is linked dynamically, because dynamic_cast will always fail for the default properties (which is also true now, since there are no default python types in OpenMesh).

Removing the property maps does not only help to remove redundancy, but also makes sure, that the Meshes from OpenMesh and OpenMesh-Python have the same memory layout, which can be handy.

I'm still not happy that we have to check for python properties via dynamic_cast every time we access a property.

What is the scenario where we can have c++ properties apart from stuff like 'v:points'?

Can't we just ignore/overwrite those default properties and only assume we have python properties?

A further nitpick: @moeller can you please make sure that the indentation (tabs vs. spaces, etc.) that you use for your changes are consistent with the rest of the files?

added 1 commit

• 2a895832 - tabified

Compare with previous version

Since you don't load any mesh with OM::IO::Options::Custom, I don't think there are any other occurrences of C++ Props, except "v:...", "h:..." etc. (which are btw. reserved in OpenMesh

About indentation in the changes: tabified them.

Thanks for the merge request, Matthias! I agree that the additional bookkeeping for properties in the MeshWrapperT class is somewhat redundant. On the other hand, I also agree with Alex that the previous implementation is simpler and likely more robust (since it depends on a smaller and higher-level interface).

In terms of performance / memory complexity, the difference between the two implementations in negligible.

Concerning the memory layout: Since MeshWrapperT is a subclass of the respective OpenMesh type, it should be straightforward to get at the memory layout of the wrapped OpenMesh via casting / slicing, right?

Currently, I'm inclined to stick with the previous, simpler implementation unless there is a compelling reason for this change. Matthias, do you have a specific use case that motivates these changes?

Hi,

the memory layout requirement comes from my OpenFlipper Plugin for OpenMesh-Python. In OpenFlipper, I create a Mesh and due to the same memory layout, i can cast the OpenFlipper-Mesh to a Python-Mesh. This is not possible, when the Python-Mesh requires more memory, since i can not access the OpenFlipper-Mesh pointer and realloc it. So i am bound to the memory of a normal OpenFlipper-Mesh (which is a standard OpenMesh-Mesh).

Since it is required to compile the python module into the plugin, I can "patch" the openmesh-python submodule via CMake in my project and hope I can stay in sync.

But you are right, there is no real use-case for mixed properties in pure python (while when accessing Meshes in OpenFlipper, it can occur very frequent).

So, all in all, if you don't like it, close it.

closed

Reopening this because Python and the OpenMesh Bindings are going to be integrated into the official OpenFlipper release.

reopened

Doesn't look like I can restore the deleted branch so I manually downloaded the last commit and pushed everything to openflipper-integration (see daa84d6f).

Friendly reminder, that I used 3 patches:

• substitude property maps
• seperate MeshType
• separated init function