Part Geometry Extensions

[abdullah]

I am trying to work out a way to extend geometry objects with workbench specific information.

I have implemented the basics of a first approach and I would like to share it with you and get some feedback.

So far I have tried to keep it simple. There is no Python interfacing yet. Just an initial approach.

The code is here:


There is a first commit with the Geometry level implementation and a second commit with an example geometry extension for the sketcher.

Of course, the extension is not fully integrated into the Sketcher yet. I did a serialisation test after adding the extension like this (dirty trial just in the middle of toggleConstruction function to see if it worked):

Code: Select all

    if(!geoNew->hasExtension(SketchGeometryExtension::getClassTypeId())) {
        SketchGeometryExtension *ext = new SketchGeometryExtension();
        ext->id = 5;
        geoNew->setExtension(ext);
    }

It saves and restores nicely. In the file it looks like:

Code: Select all

<Property name="Geometry" type="Part::PropertyGeometryList">
    <GeometryList count="1">
        <Geometry  type="Part::GeomCircle">
            <GeoExtensions count="1">
                <GeoExtension type="Sketcher::SketchGeometryExtension" id="5"/>
            </GeoExtensions>
            <Construction value="1"/>
            <Circle CenterX="-111.9" CenterY="5.8" CenterZ="0.0" NormalX="0.0" NormalY="0.0" NormalZ="1.0" AngleXU="-0.0" Radius="29.6"/>
        </Geometry>
    </GeometryList>
</Property>

A FC with GeoExtension support opens fine any file saved with a previous version of FreeCAD.

A FC without GeoExtension support, for example:

OS: Ubuntu 18.04.1 LTS
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.18.15221 (Git) AppImage
Build type: Release
Branch: master
Hash: fc5b6288c6eb4fe89192f63f1fbdf4a43c0fad27
Python version: 3.6.6
Qt version: 5.6.2
Coin version: 4.0.0a
OCC version: 7.3.0

opens the file just fine.

The nice part of all this trick is that the Geometry is used as always before, but workbenches may add extended information, even if Geometry does not know about this extended information at all.

At this point, the challenges are:
a) Python support for extensions (not my strongest point, but if this is ok I will try my best),
b) There is no support for a Python written extension (for a Python written workbench). I think this would be great, but I will have to dive into how ickby managed to make that work for normal (container/property) extensions.

Any feedback is welcome.

... ping ...

how bad is this? how could it be better? Am I too much off?

Off Topic: For some reason the latest PPA crashes in my system when trying to open any file:
OS: Ubuntu 18.04.1 LTS
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.18.15609 (Git)
Build type: Release
Branch: master
Hash: a14af0f2bd26f980d71be80a799ee48127bc7619
Python version: 2.7.15rc1
Qt version: 5.9.5
Coin version: 4.0.0a
OCC version: 7.3.0

[wmayer]

how bad is this? how could it be better? Am I too much off?

The basic idea of ickby's extension framework is to use the composition pattern and thus to significantly reduce the number of sub-classes.

The Part::Geometry at the moment is only used by the sketcher and it would have been fine to directly add the further attributes there. But who knows what the future will bring and since you have your extension framework already working we could add it for v0.19.

However, there are a couple of issues that are not really clear with your implementation:

1. What's the purpose of using a std::map instead of a std::vector or std::list? A map has the behaviour that it sorts its elements by key and thus the last added element won't be necessarily at the end which may lead to some undesired effects.
2. The way an extension is added my lead to a memory leak because there might already an element of the given type and won't destroy it. Also it may happen that you need two or more extensions of a certain type. This is not possible with a map.
3. I assume the extensions belong to the Geometry but at the moment it doesn't delete them upon destruction.
4. I wonder if we should use shared and weak pointers to avoid possible dangling pointers.

[wmayer]

b) There is no support for a Python written extension (for a Python written workbench). I think this would be great, but I will have to dive into how ickby managed to make that work for normal (container/property) extensions.

The magic happens here:
https://github.com/FreeCAD/FreeCAD/blob ... mp.cpp#L52
https://github.com/FreeCAD/FreeCAD/blob ... p.cpp#L113

[abdullah]

Thanks for replying

The Part::Geometry at the moment is only used by the sketcher and it would have been fine to directly add the further attributes there. But who knows what the future will bring and since you have your extension framework already working we could add it for v0.19.

While for generic staff, like the unique identifier we have now, directly in geometry is a perfect place, in cases where the need applies to only some geometry elements it is overkill to put everything at Geometry level. I must admit I did not like it either. Part should not contain code that is specific for the Sketcher.

For the offset curves I will need an id (similar to the example). Extensions may also be solution for the "defining external geometry", in the sense that information about the links can be directly stored in the geometry. Realthunder has implemented a way to do it that also helps with the toponaming problem at sketcher level. He has put all these new members at Geometry level, by each addition of a member it makes less sense to me. Extensions may be a good solution (provided I manage to implement it properly).

Another important point for me when thinking about extensions, is to add that information in the right place, while still being seamlessly serializable together with the geometry. This is nice as it does not affect compatibility.

The basic idea of ickby's extension framework is to use the composition pattern and thus to significantly reduce the number of sub-classes.

Thank you for the keyword "composition pattern". I did not know they existed:
https://en.wikipedia.org/wiki/Composite_pattern

Related:
https://en.wikipedia.org/wiki/Software_design_pattern

There is a lot to learn!

However, there are a couple of issues that are not really clear with your implementation:

It is only 4. I am definitely improving!!!

1. What's the purpose of using a std::map instead of a std::vector or std::list? A map has the behaviour that it sorts its elements by key and thus the last added element won't be necessarily at the end which may lead to some undesired effects.

The original implementation did use a std::vector. A std::map has the nice feature of having a single key (no duplicated keys), meaning a geometry object may only have an extension of one type. Then, for example in the sketcher, I could just get the extension I wanted from its type, like map[type], without needing to iterate the container. There may be different extensions interesting for a single workbench. For example, if we think about the Sketcher and external geometry, it is possible that the external geometry uses two different extensions, one for the "id" that all sketcher geometry has, and a second extension which contains link information and the state (defining, locked, ...).

All in all, I did it thinking of getting an advantage in the operation from the easy access. But of course I am open to other container.

2.The way an extension is added my lead to a memory leak because there might already an element of the given type and won't destroy it. Also it may happen that you need two or more extensions of a certain type. This is not possible with a map.

I started from the premise that I won't need two or more extensions of a certain type. Maybe that is not a good design option. I can still not come to an example where I would need several extensions of the same type. I know there is also multimap, but I would prefer not to use it unless necessary. Maybe a map is not the best option after all.

3. I assume the extensions belong to the Geometry but at the moment it doesn't delete them upon destruction.

Yup, I realised short after pushing it. In the meantime it got:

Geometry::~Geometry()
{
for(std::map<Base::Type, GeometryExtension *>::iterator it = extensions.begin(); it != extensions.end(); it++)
delete it->second;
}

I think this will work fine, as the only dynamically allocated memory is the "second".

4. I wonder if we should use shared and weak pointers to avoid possible dangling pointers.

At least I did know these existed!
https://stackoverflow.com/questions/498 ... ifferences

To be honest, I am not very acquainted with them. It belongs the set of "cool things" I do not know how to use. But it may be a good idea when extension's memory is accessible by the workbench user, who can release it trying to remove it.

However, I am not sure how I should implement it. Is it just defining the container as:
std::map<Base::Type, shared_ptr<GeometryExtension>> extensions;

and/or returning a shared_ptr in the accessor?

std::map<Base::Type, shared_ptr<GeometryExtension>> &getExtensions();
shared_ptr<GeometryExtension> getExtension(Base::Type type);

As you see I do not have a solid knowledge of how to make use of them.

The magic happens here:
https://github.com/FreeCAD/FreeCAD/blob ... mp.cpp#L52
https://github.com/FreeCAD/FreeCAD/blob ... p.cpp#L113

Thanks, I will take a look and see if I manage to understand it.

[wmayer]

The original implementation did use a std::vector. A std::map has the nice feature of having a single key (no duplicated keys), meaning a geometry object may only have an extension of one type. Then, for example in the sketcher, I could just get the extension I wanted from its type, like map[type], without needing to iterate the container.

I see this over and over again but it is just plain wrong to search in a map using the [] operator because if the key doesn't exist it will be added and in this case the value will be a null pointer. If the rest of the class does not explicitly expects null pointers it's a good chance to cause crashes because of dereferencing null pointers.

The correct way is to use the find() method.

To be honest, I am not very acquainted with them. It belongs the set of "cool things" I do not know how to use. But it may be a good idea when extension's memory is accessible by the workbench user, who can release it trying to remove it.

However, I am not sure how I should implement it. Is it just defining the container as:
std::map<Base::Type, shared_ptr<GeometryExtension>> extensions;

and/or returning a shared_ptr in the accessor?

It depends on the way an extension is supposed to be used:
If it's not clear who is the real owner of an extension (you access the extension, the Geometry will be destroyed but the extension should be still alive) you internally use a shared_ptr and everywhere on the interface you use a shared_ptr too. The extension will be destroyed as soon as all shared_ptr are deleted.

If the Geometry is the definite owner of the extension, i.e. it will be destroyed if the Geometry is being destroyed you internally use a shared_ptr and on the interface you use a weak_ptr. If the Geometry is destroyed and with it the extensions then the weak_ptr has a null pointer and not a dangling pointer.

This way you can reduce the likelihood of dangling pointers.

[abdullah]

The original implementation did use a std::vector. A std::map has the nice feature of having a single key (no duplicated keys), meaning a geometry object may only have an extension of one type. Then, for example in the sketcher, I could just get the extension I wanted from its type, like map[type], without needing to iterate the container.

I see this over and over again but it is just plain wrong to search in a map using the [] operator because if the key doesn't exist it will be added and in this case the value will be a null pointer. If the rest of the class does not explicitly expects null pointers it's a good chance to cause crashes because of dereferencing null pointers.

The correct way is to use the find() method.

I did know about this one (you told me sometime ago) and I keep forgetting it.

Probably that is the reason why you were asking in the first place. Then probably it is best to use a std::vector.

It depends on the way an extension is supposed to be used:
If it's not clear who is the real owner of an extension (you access the extension, the Geometry will be destroyed but the extension should be still alive) you internally use a shared_ptr and everywhere on the interface you use a shared_ptr too. The extension will be destroyed as soon as all shared_ptr are deleted.

If the Geometry is the definite owner of the extension, i.e. it will be destroyed if the Geometry is being destroyed you internally use a shared_ptr and on the interface you use a weak_ptr. If the Geometry is destroyed and with it the extensions then the weak_ptr has a null pointer and not a dangling pointer.

This way you can reduce the likelihood of dangling pointers.

Thank you for this explanation. Believe or not "ownership of the memory" was hindering me.

I tend to think that Geometry is the owner of the extension, so I will go with the shared/weak approach.

[abdullah]

It depends on the way an extension is supposed to be used:
If it's not clear who is the real owner of an extension (you access the extension, the Geometry will be destroyed but the extension should be still alive) you internally use a shared_ptr and everywhere on the interface you use a shared_ptr too. The extension will be destroyed as soon as all shared_ptr are deleted.

If the Geometry is the definite owner of the extension, i.e. it will be destroyed if the Geometry is being destroyed you internally use a shared_ptr and on the interface you use a weak_ptr. If the Geometry is destroyed and with it the extensions then the weak_ptr has a null pointer and not a dangling pointer.

This way you can reduce the likelihood of dangling pointers.

I come back to you for some advice.

First I was thinking of making it a shared_ptr inside the Geometry, and returning weak_ptr references, under the premise the Geometry object is the owner. Searching the Internet, I realised that I were to do that, when using it I would have to:

a) get the weak pointer
b) lock the weak pointer into a shared_pointer
c) check that the shared_pointer is not null (well FC is not multithread yet, that is only to make the multi thread safe)
d) do whatever I have to do with the pointer

Also, if I have a std::vector<shared_pointer<GeometryExtension>> extensions; and I want to return a std::vector<std::weak_pointer<GeometryExtension>> &getExtensions(); I would have to create a weak pointer vector out of the shared pointer vector in that function. I have not found an easy way of doing this.

This sounds like a lot of trouble. Am I lost? May I have your input?

Thanks in advance.

[wmayer]

You cannot return as a (const ?) reference. You must create a local vector of weak_ptr and return that.

[abdullah]

You cannot return as a (const ?) reference. You must create a local vector of weak_ptr and return that.

I have rewritten the thing to use smart pointers and a given ownership policy. I am not sure it is ok. That is why I am pinging you.

Geometry class with its extension interface:

Code: Select all

class PartExport Geometry: public Base::Persistence
{
...
public:
....
    const std::vector<std::weak_ptr<GeometryExtension>> getExtensions() const;

    bool hasExtension(Base::Type type) const;
    const std::weak_ptr<GeometryExtension> getExtension(Base::Type type) const;
    void setExtension(std::unique_ptr<GeometryExtension> geo);

protected:
    std::vector<std::shared_ptr<GeometryExtension>> extensions;
}

Geometry is the owner of the extensions and only "borrows" them as const weak pointers. If the user code wants to modify whatever it has to do a copy. This is consistent as Geometry gets copied to the undo buffer and it cannot be that a change in the extensions of the current geometry (of a sketch), affects the extensions of geometries in the undo buffer.

setExtension enforces a unique pointer, so Geometry wants to have have full ownership and does not allow to share unless it is via weak pointer. This also should be rather efficient as enforces a std::move syntax.

Geometry extension:

Code: Select all

class PartExport GeometryExtension: public Base::Persistence
{
    TYPESYSTEM_HEADER();
public:
    virtual ~GeometryExtension();

    // Persistence implementer ---------------------
    virtual unsigned int getMemSize(void) const = 0;
    virtual void Save(Base::Writer &/*writer*/) const = 0;
    virtual void Restore(Base::XMLReader &/*reader*/) = 0;

    virtual std::unique_ptr<GeometryExtension> copy(void) const = 0;

    virtual PyObject *getPyObject(void) = 0;
protected:
    GeometryExtension();
};

Has evolved to have the ability of making a full copy of any inherited class as a unique pointer to the base class (obviously with a new memory allocation but the same information). This enables to copy extensions when making geometry clones, so the extensions of the clones are memory separated copies.

An example how to add an extension:

Code: Select all

    if(!geoNew->hasExtension(SketchGeometryExtension::getClassTypeId())) {
        std::unique_ptr<SketchGeometryExtension> ext = std::make_unique<Sketcher::SketchGeometryExtension>();
        ext->id = 5;
        geoNew->setExtension(std::move(ext));
    }

The storage format is the same as before. The full code here

I am rather new to all this c++11 concepts so I expect corrections

[wmayer]

The code looks OK to me (but I didn't test it).

I am rather new to all this c++11 concepts so I expect corrections

Me too. I made my first C++11 experiences when working for v0.17.