Improvements in ObjectIdentifier.

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Improvements in ObjectIdentifier.

Post by andrecaldas »

I would like to make some improvements in ObjectIdentifier.

In my opinion, ObjectIdentifier has a great potential, because I believe that internally FreeCAD references should be less dependent on OCCT generated shapes. I believe we should have more named references (or tagged with uuid) directly between "native types".

Regardless of if you agree or not with that, I would like to work on ObjectIdentifier. It provides a mechanism for referencing objects by "name". But not only objects, but individual values (member variables) on those. For example, theoretically, you can reference the "y" coordinate of the "starting point" of some "line segment" in a "sketch". I imagine a path like:
- sketch.geometries.TheLineINamed.Start.y

After the hard work of giants, it is now much easier to see the big picture. By no means I want to criticize those people or their work. Let me mention some simple things that I would like to improve:


Hard coded polimorphism in ObjectIdentifier::Component

Many classes in FreeCAD have their behaviour specified through the use of enum. This enum specifies the "type of object". All over the place, the logic of those classes get too complicated because things start to have different meaning according to the "type of object". One example is the Sketcher::Constraint. Many methods need engage in a very complicated logic to determine the meaning of the member variables, because the real meaning depends on the "constraint type". Just take a look on the size of ConstraintPy::PyInit(). There are 30 "else if" on it.

To start with something easier, I would like to specialize App::ObjectIdentifier::Component. From the documentation:
A component is a part of a Path object, and is used to either name a property or a field within a property. A component can be either a single entry, and array, or a map to other sub-fields.
Inside Component we have:

Code: Select all

        static Component SimpleComponent(const char * _component);

        static Component SimpleComponent(const String & _component);
        static Component SimpleComponent(String &&_component);

        static Component ArrayComponent(int _index);

        static Component RangeComponent(int _begin, int _end = INT_MAX, int _step=1);

        static Component MapComponent(const String &_key);
        static Component MapComponent(String &&_key);
And not only that... in order to allow hard coded polymorphic behaviour, we have:

Code: Select all

        bool isSimple() const { return type == SIMPLE; }
        bool isMap() const { return type == MAP; }
        bool isArray() const { return type == ARRAY; }
        bool isRange() const { return type == RANGE; }
And inside ObjectIdentifier we have:

Code: Select all

    static Component SimpleComponent(const char * _component)
        {return Component::SimpleComponent(_component);}

    static Component SimpleComponent(const String & _component)
        {return Component::SimpleComponent(_component);}

    static Component SimpleComponent(String &&_component)
        {return Component::SimpleComponent(std::move(_component));}

   static Component SimpleComponent(const std::string _component)
        {return Component::SimpleComponent(_component.c_str());}

    static Component ArrayComponent(int _index)
        {return Component::ArrayComponent(_index); }

    static Component RangeComponent(int _begin, int _end = INT_MAX, int _step=1)
        {return Component::RangeComponent(_begin,_end,_step);}

    static Component MapComponent(const String &_key)
        {return Component::MapComponent(_key);}

    static Component MapComponent(String &&_key)
        {return Component::MapComponent(_key);}
This all suggests to me that Component should be virtual with specializations for all those "types of components".

A small price to pay is that ObjectIdentifier cannot hold a std::vector<Component>. It will probably have to hold some std::vector<std::unique_ptr<Component>>, instead.


Hard coded polimorphism in ObjectIdentifier

Each ObjectIdentifier identifies a specific type of data. So, it should be specialized to avoid boost::any and, instead access the number, vector, property, etc.

Not only we would avoid lots of castings, but we can specify the expected type in many places. For example:
  1. This is an identifier for a number; or
  2. I expect an identifier for a number.
There is more... but I think this is a good start...

Anyone interested?

I can do all the conding... I just want people interested in the idea, to discuss stuff with... or to guide me and point out things I am not aware... or suggest approaches...
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Re: Improvements in ObjectIdentifier.

Post by andrecaldas »

In ObjectIdentifier, many classes are defined inside the ObjectIdentifier.

I understand this as a way to reuse the class's "namespace". I want to specialize some of the classes inside ObjectIdentifier. Since I don't want to get the main class too bloated, I think it would be a good idea to have a proper namespace.

One option is the following:
  1. Keep the class ObjectIdentifier in App.
  2. Create a namespace for the helper classes. Something like App::ObjectIdentifierHelpers.
Another one:
  1. Create a namespace like App::ObjectIdentifier.
  2. Put the class ObjectIdentifier in App::ObjectIdentifier.
  3. Put the helper classes in App::ObjectIdentifier.
And of course... just put everything inside the definition of ObjectIdentifier, just as it is now.

Or... leave what exists as it is now and put the new stuff directly in the App namespace.

Even though you might not believe in what I am trying to do... I would appreciate advices from more experienced developers. :-)
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Re: Improvements in ObjectIdentifier.

Post by andrecaldas »

I have been working on the ObjectIdentifier for some days.

I would like to show you what I am doing and what I plan to do. Also, I would like to expose my understanding of the ObjectIdentifier infrastructure. Please, do correct what I am misunderstanding. :-)

I would be very glad if more experienced developers could give-me some support. I do not really know anyone, so I will ping some people: @abdullah, @openBrain, @eivindkvedalen, @onekk . Please, forgive me if I was not supposed to ping you.

So, an ObjectIdentifier is a sequence of Components. Each component represents part of the "path" that identifies the "object". The path can also be expressed by a string that can be split into parts that represent each component of the path.

Doubt: I would like to know exactly how a string is split. What would be a legal and and ilegal string, in the sense that it might or might not rpresent an object.

ObjectIdentifier::Component

Today, Component is a class declared inside ObjectIdentifier. One thing that I did was to remove the size of ObjectIdentifier.h by removing many classes from inside it. I think that many .h files are too monolithic. Those classes are defined inside ObjectIdentifier for the single purpose of using its "namespace" (in a boader sense). I hope that in the future we can slowly try to split many of those .h files. But this is not the subject of this post.

As many structures in FreeCAD, Component is a monolithic class with polymorphic behaviour.

I am implementing a virtual base Component class. Then, instead of using some enum typeEnum, I have subclasses. This makes the code much easier to understand, because the knowledge of each specialized class is encapsulated in each class (as in OOP).

With the polymorphic monolitic Component, it is very hard to understand what are the possible states of each instance. For example, is an "array", a "map" or a "range" supposed to have a "name"? A Component has a "name", "begin", "end" and "step". The meaning of each variable depends on its "typeEnum".

With real polymorphism, an ArrayComponent has an "index". A RangeComponent has "begin", "end" and "step". And a MapComponent has a "key".


Expressions

Each component has specific data: "name", "key", "index", "begin", etc. With the use of "expressions", those data can be "generated" by expressions. Because of that, the class Expression implements an Expression::Component. Those (should) represent a mutable ObjectIdentifier::Component. But it is not a subclass. Now, it is a class that contains an object identifier component. The data in those components are manually updated, and the expression component has to manage a correspondent object identifier component.

In the code I am implementing, Expression::Component is a virtual base class. And the specializations are derived from it and from ArrayComponent, MapComponent, etc. The base array component class has a virtual getIndex() that, in the case of the expression component, getIndex() returns the result of evaluating the correspondent expression.

With the use of constant expressions, every comonent could be an expression component, but I am not doing it right now.


Smart pointers

I am also trying to follow the guidelines suggested by @chennes , and gradually I am substituting pointer for smart pointers.


updateLabelReference()

Right now, I am trying to understand updateLabelReference(). I do not really understand the role of ObjectIdentifier::Component::name. As far as I understand, ObjectIdentifier should forward the call to its component. Each component should forward the call to the expressions.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Improvements in ObjectIdentifier.

Post by openBrain »

Saw the ping but unfortunately not enough time to dig into this.
Could you provide a very simplified snippet showing kind of changes you're proposing ?

If he has some time, maybe @wmayer may have insights as he is (among other things) the core maintainer.
User avatar
onekk
Veteran
Posts: 6146
Joined: Sat Jan 17, 2015 7:48 am
Contact:

Re: Improvements in ObjectIdentifier.

Post by onekk »

I have very little knowledge of C++, minimal skills to follow a code block, but not much more.

Sad to not be of any help.

Regards

Carlo D.
GitHub page: https://github.com/onekk/freecad-doc.
- In deep articles on FreeCAD.
- Learning how to model with scripting.
- Various other stuffs.

Blog: https://okkmkblog.wordpress.com/
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Re: Improvements in ObjectIdentifier.

Post by andrecaldas »

openBrain wrote: Mon Mar 27, 2023 4:53 am Could you provide a very simplified snippet showing kind of changes you're proposing ?
The basic idea is to make ObjectIdentifier::Component into a virtual class. Take a look at Component::get(). Simplifying the code, this is basically:

Code: Select all

Py::Object ObjectIdentifier::Component::get(const Py::Object &pyobj) const {
    if(isSimple()) {
        return pyobj.getAttr(getName());
    } else if(isArray()) {
        if(pyobj.isMapping())
            return Py::Mapping(pyobj).getItem(Py::Int(begin));
        else
            return Py::Sequence(pyobj).getItem(begin);
    }else if(isMap())
        return Py::Mapping(pyobj).getItem(getName());

    Py::Object slice(PySlice_New(Py::Int(begin).ptr(),
                                end!=INT_MAX?Py::Int(end).ptr():nullptr,
                                step!=1?Py::Int(step).ptr():nullptr),true);
    PyObject *r = PyObject_GetItem(pyobj.ptr(),slice.ptr());
    if(!r)
        Base::PyException::ThrowException();
    return Py::asObject(r);
}
Notice all those "else if"! With real polymorphism, now Component::get() is:
https://github.com/andre-caldas/FreeCAD ... nent.h#L61

Code: Select all

    virtual Py::Object get(const Py::Object& pyobj) const = 0;
And, for example... (simplifying the code)...

Code: Select all

Py::Object ArrayComponent::get(const Py::Object& pyobj) const
{
    auto _index = getIndex();
    if(pyobj.isMapping())
        return Py::Mapping(pyobj).getItem(Py::Int(_index));
    return Py::Sequence(pyobj).getItem(_index);
}
Also, if you are already working with an ArrayComponent, you can use ArrayComponent::getIndex(). But if it is NOT an array, then you cannot call getIndex().

The same is true for Expression::Component. The logic is all very complicated because you never know if it is a simple component, an array, a map or a range. What is the meaning of the following code?

Code: Select all

if(!comp.isRange() && !e2 && !e3) {...}
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Improvements in ObjectIdentifier.

Post by openBrain »

andrecaldas wrote: Mon Mar 27, 2023 8:24 pm The basic idea is to make ObjectIdentifier::Component into a virtual class.
Well, seems to make sense. Some other classes in FC are already managed that way, and making full usage of OOP is a sensible idea. :)
Post Reply