Sketcher Development - Integration of Extensions

[abdullah]

Did this somehow break setting an Equal constraint on two circles?

git commit 51189caba41a97fe5cf6b65c5828341b23aefff3.

[abdullah]

I gave a try at PR 4130.
I first thought it didn't work.
I created a bspline, and experienced the same jumping behaviour of the weight constraint as before.
BUT, if I move any pole of the bspline, it fixes the bug, and the weight constraint can be moved without jumping any more.
Further testing reveals that only the last created weight constraint "jumps" when grabbed.
If I create 4 bsplines in the sketch.
It shows 4 weight constraints.
The first 3 weight constraints can be grabbed normally, but the last one jumps.
If I move a pole, of any of the 4 bsplines, it fixes the jumping.

I want you to know that I am not ignoring you.

I need some extra solver information to implement this properly and I am achieving this while doing the highlighting of constrained elements.

[Chris_G]

I want you to know that I am not ignoring you.

No problem, Abdullah.
Thanks for your persevering work on the sketcher.

[abdullah]

I want you to know that I am not ignoring you.

No problem, Abdullah.
Thanks for your persevering work on the sketcher.

Welcome! The PR I have just merged should fix your issue too.

It also fixes a bad cosmetic effect happening when a B-Spline is constrained to be non-rational (all poles equality constraint, but no weight constraint fixing the weight value) and the edge is dragged. Some versions allow to drag the pole to get a higher weight, but the pole circle does not follow because OCCT will automatically normalise the weight values. The current version of master leverages the ability to identify all the parameters that are in a same dependency group as a provide element, so if all poles are in the same dependency group, then it is constrained to be non-rational and it won't allow you to drag the edge. This is different than actually being non-rational, because if it is non-rational it may well be that the poles are not constrained to be non-rational, then it should allow to drag the pole to make it rational (hopefully this is not the tongue twister it looks ).

[Chris_G]

(hopefully this is not the tongue twister it looks ).

No, it is not ... "Brain Blaster" would be more appropriate
I will read your explanation another hundred of times and see if I can comprehend it

[chennes]

@abdullah, is it possible that this bug is related to your recent work? It only occurs when you have the spline nodes selected during a mirroring operation (per the reporter's instructions): just selecting the spline by itself does not cause it. I'm not getting quite the same crash as the reporter, but I do get a crash.

[abdullah]

@abdullah, is it possible that this bug is related to your recent work? It only occurs when you have the spline nodes selected during a mirroring operation (per the reporter's instructions): just selecting the spline by itself does not cause it. I'm not getting quite the same crash as the reporter, but I do get a crash.

Yes, the crash is mine. However, in old sketcher it did not work either (it did not crash though). You get a nice result, but the poles are not poles anymore, but circles that you can move all around. When B-Spline was introduced, I forgot to adapt this routine. It is not an straightforward fix because of the interdependency of poles, knots with bsplines, InternalAlignment geometry states and constraints. I am working now on it. Thanks for making me aware

Because you are looking into these tickets, allow me one tip. When ViewProvidersketch crashes the culprit is mostly SketchObject. Either it is asking for something it should not ask (like in this case addSymmetric), or it is asking for something it should ask but at the wrong time (geometry was updated but constraints not yet and an update is being triggered when it should be blocked and triggered after constraints updated).

With the new changes of the sketcher, is more likely to get crashes if there were "hidden" bugs before. Which is good and bad. We do not like crashes, we do like bugs removed, but crashes increase reporting, which increases bugs removed

[abdullah]

Well, this post is rather to inform about the status of the integration and what I am currently doing.

Regarding bugs and fixes introduced by this integration, I think everything has been addressed (if something is left open feel free to remind me, my memory was never great and I am getting old).

I have some code improvements, code clean up and documentation deferred. It makes a nice list for new year resolutions. Hopefully it won't be forgotten on 2nd January.

I would like to deal with some Sketcher PRs before the end of the year.

Currently I am working on improving the synchronisation between solver and SketchObject, specially related to dragging operations. Dragging operations have been a reliable source of crashes this year, providing one fresh crash to substitute any previously fixed one. Most relates to the lack of responsibility for synchronising solver and SketchObject. To fix this, I will centralise the responsibility on SketchObject. This will hopefully lead to a more stable framework.

Realthunder was very nice to report a nasty bug in the PR section of GitHub (which partly relates to this lack of synchronisation). While arriving to a working fix was not difficult, working on it was the last drop which made me realise a proper framework was necessary, better sooner than later.

In the effort, I am taking a look to the plethora of mechanisms in the Sketcher to reduce calls to solve() and recompute() and keep track of synchronisation in different situations. Most, if not all, were introduced by previous versions of myself over the years, in an effort to tame a wild sketcher that at the beginning I only partly understood. If anybody was wondering, I count the following mechanisms:

1. The managedoperation lock, to trigger data validity check on user properties provided by the user via Python (well rather to avoid triggering it if the changes were done by the sketcher).
2. The internaltransaction lock, to prevent calculations and extra signaling reacting to changes on constraints or geometries when both are going to be changed. This additionally allows to synchronise Geometry and Constraint modifications.
3. The SolverNeedsUpdate flag, which originally was created for operations modifying the geometry but not the DoF of the solver (such as construction), so to avoid extra solving operations which may even become unnecessary (some rudimentary form of lazy evaluation).
4. The NoRecomputes flag, the first mechanism to reduce solving operations introduced several years ago, which mostly relates to addition of geometry and not triggering several recomputes in a row, and even just solving instead of recomputing everything (once upon a time a change of the sketcher in edit mode would trigger a recompute of the sketch, and all PartDesign touched properties).

Well, enough chatting... back to work

[abdullah]

In the German forum a bug was reported that when using the Block constraint extensively in a Sketch, the previous algorithm would fail.

The problem is not simple in that there are several possible solutions to block the geometry (without incurring in redundant constraints, so allowing to have dimensional constraints at the same time at the block constraint and still enforcing them). Some solutions simply do not block the geometry. Other solutions would hypothetically work, but ultimately lead to partially redundant constraints, which are ignored by the solver, causing the blocked geometry to move. This is very bad, because it devoids the constraint of any meaning and leads to a lot of frustration as a user.

I have been several days finding a solution and I have just merged a new algorithm in commit. It would be great if somebody is available for some testing (should be tomorrow in the PPA).

I have tested it with several cases, but I rely on you for making it sweat

[abdullah]

I take note that you have ideas regarding the serialisation of properties and thus your strong preference to keep this serialisation within the properties. I would like to facilitate your work. I think it is important that when there are decisions to be made that have an important impact on the project they are discussed. Preferably early in the implementation so as to enable meaningful feedback. I certainly appreciate it.

In my own defence, I posted this as soon as I did the initial implementation. There isn't much response gathered. Additionally, there is one dev wanted something that may have potential conflict, I decide to wait. As for the impact, as long as each object let the property class handle its own save and restore, it shall be fine.

This reply was long overdue. Sorry for that.

You do not need a defence. Certainly not with me. We all try our best.

In the meantime, the idea of the migration extension has been implemented and used to migrate the boolean for construction in Part::Geometry to the Sketch. I know you are very capable and I am sure you would figure it out this all by yourself, but I thought it still makes sense to save you some time.

If you decide to migrate that extra geometry information you serialise in PropertyGeometryList to geometry extensions, which would be great to merge your Sketcher functionality into master, this is how it is currently implemented in master:

1. The class ExternalGeometryExtension is currently unused, just waiting for you. It has the same "Flag" you use LinkStage3. This extension is intended to be inserted only into Part::Geometry-s that are external geometry. There is another extension that is currently inserted in all the geometries SketchGeometryExtension. Of course, feel free to add what you need to ExternalGeometryExtension and SketchGeometryExtension to implement your functionality.

Code: Select all

class SketcherExport ExternalGeometryExtension : public Part::GeometryPersistenceExtension, private ISketchExternalGeometryExtension
{
    TYPESYSTEM_HEADER_WITH_OVERRIDE();
public:
    // START_CREDIT_BLOCK: Credit under LGPL for this block to Zheng, Lei (realthunder) <realthunder.dev@gmail.com>
    enum Flag {
        Defining = 0,   // allow an external geometry to build shape
        Frozen = 1,     // freeze an external geometry
        Detached = 2,   // signal the intentions of detaching the geometry from external reference
        Missing = 3,    // geometry with missing external reference
        Sync = 4,       // signal the intention to synchronize a frozen geometry
        NumFlags        // Must be the last type
    };
    // END_CREDIT_BLOCK: Credit under LGPL for this block to Zheng, Lei (realthunder) <realthunder.dev@gmail.com>

    constexpr static std::array<const char *,NumFlags> flag2str {{ "Defining", "Frozen", "Detached","Missing", "Sync" }};
    ...
}

2. The class SketcherGeometryExtension is currently in use. Each Geometry has one. It has a "Id" that is reserved for you. Currently a correlative number is assigned (using the atomic counter as in LinkStage3). I have intentionally left this Id without serialisation, not to interfere with your implementation. So you would not read an Id that maybe is not what you expect an interferes with your implementation. This extension now stores the Construction flag, a new Blocked flag (relating to block constraint) and an InternalAlignmentType flag (relating to InternalAlignment constraint). Any data that you need to add to geometry for all geometries (internal and external), is intended to go here.

Code: Select all

class SketcherExport SketchGeometryExtension : public Part::GeometryPersistenceExtension, private ISketchGeometryExtension
{
    TYPESYSTEM_HEADER_WITH_OVERRIDE();
public:

    SketchGeometryExtension();
    SketchGeometryExtension(long cid);
    virtual ~SketchGeometryExtension() override = default;

    // Persistence implementer ---------------------
    virtual void Save(Base::Writer &/*writer*/) const override;
    virtual void Restore(Base::XMLReader &/*reader*/) override;

    virtual std::unique_ptr<Part::GeometryExtension> copy(void) const override;

    virtual PyObject *getPyObject(void) override;

    virtual long getId() const override {return Id;}
    virtual void setId(long id) override {Id = id;}

    virtual InternalType::InternalType getInternalType() const override {return InternalGeometryType;}
    virtual void setInternalType(InternalType::InternalType type) override {InternalGeometryType = type;}

    virtual bool testGeometryMode(int flag) const override { return GeometryModeFlags.test((size_t)(flag)); };
    virtual void setGeometryMode(int flag, bool v=true) override { GeometryModeFlags.set((size_t)(flag), v); };

    constexpr static std::array<const char *,InternalType::NumInternalGeometryType> internaltype2str {{ "None", "EllipseMajorDiameter", "EllipseMinorDiameter","EllipseFocus1", "EllipseFocus2", "HyperbolaMajor", "HyperbolaMinor", "HyperbolaFocus", "ParabolaFocus", "BSplineControlPoint", "BSplineKnotPoint" }};

    constexpr static std::array<const char *,GeometryMode::NumGeometryMode> geometrymode2str {{ "Blocked", "Construction" }};

    static bool getInternalTypeFromName(std::string str, InternalType::InternalType &type);

    static bool getGeometryModeFromName(std::string str, GeometryMode::GeometryMode &type);
...    
}

3. These extensions have corresponding facades Sketcher::GeometryFacade and Sketcher::ExternalGeometryFacade. It is how the information about these extensions is currently retrieved and stored in, for example, SketchObject (also in the rest of the Sketcher).

4. I do not think it will have any use for you for the functionality above, but there two other runtime extensions (not serializable), SolverExtension, which stores information about whether a geometry is fully constrained by the solver or not, and whether each of the elements (none, start, end, mid) are constrained individually. This is mostly used to draw the lines in the constrained color. It is inserted by Sketch.cpp and it is its responsibility. ViewProviderSketchGeometryExtension is only available for GeomPoint that are BSpline poles and it is only available if FreeCAD is in UI mode. It stores a factor of scale for BSpline weights. The master Sketcher has a new constraint Weight that is adimensional and is used for the weights of the poles. The scale factor is a factor that facilitates the representation of the circle of the pole, and relates to the length of the BSpline. It is the responsibility of ViewProviderSketch to set this extension (in ViewProviderSketch::draw(bool ...)). Unlike other extensions, this one is inserted on a const Part::Geometry by const-casting it, it is assumed to work in a kind of "inmutable" way, so is understood that it does not modify the Geometry object. These extensions are not implemented in the Facades.

5. Coming back to migration. For migration of information from Part to Sketcher, there is a temporary extension Part::Geometry. It is a runtime-only extension (not serializable), you set a flag in MigrationType for your migration, create appropriate data members and interface and it will store your information until it arrives to the Sketcher:

Code: Select all

class PartExport GeometryMigrationExtension : public Part::GeometryExtension
{
    TYPESYSTEM_HEADER_WITH_OVERRIDE();
public:

    // Indicates the type of migration to be performed, it is stored as a bitset, so several
    // migrations may take place in a single extension.
    // It is intended to support also LinkStage3 migration with a single framework (Id, Ref, ...)
    enum MigrationType {
            None                    = 0,
            Construction            = 1,
            NumMigrationType        // Must be the last
    };

    GeometryMigrationExtension();
    virtual ~GeometryMigrationExtension() override = default;

    virtual std::unique_ptr<Part::GeometryExtension> copy(void) const override;

    virtual PyObject *getPyObject(void) override;


    virtual bool getConstruction() const {return ConstructionState;}
    virtual void setConstruction(bool construction) {ConstructionState = construction;}

    virtual bool testMigrationType(int flag) const { return GeometryMigrationFlags.test((size_t)(flag)); };
    virtual void setMigrationType(int flag, bool v=true) { GeometryMigrationFlags.set((size_t)(flag), v); };


private:
    GeometryMigrationExtension(const GeometryMigrationExtension&) = default;

private:
    using MigrationTypeFlagType = std::bitset<32>;
    MigrationTypeFlagType           GeometryMigrationFlags;
    bool                            ConstructionState;

};

5.1. This is an example how to load the information into the extension on Restore():

Code: Select all

void Geometry::Restore(Base::XMLReader &reader)
{
    // In legacy file format, there are no extensions and there is a construction XML tag
    // In the new format, this is migrated into extensions, and we get an array with extensions
    reader.readElement();

    if(strcmp(reader.localName(),"GeoExtensions") == 0) { // new format

        int count = reader.getAttributeAsInteger("count");

        for (int i = 0; i < count; i++) {
            reader.readElement("GeoExtension");
            const char* TypeName = reader.getAttribute("type");
            Base::Type type = Base::Type::fromName(TypeName);
            GeometryPersistenceExtension *newE = (GeometryPersistenceExtension *)type.createInstance();
            newE->Restore(reader);

            extensions.push_back(std::shared_ptr<GeometryExtension>(newE));
        }

        reader.readEndElement("GeoExtensions");
    }
    else if(strcmp(reader.localName(),"Construction") == 0) { // legacy

        bool construction = (int)reader.getAttributeAsInteger("value")==0?false:true;

        // prepare migration
        if(!this->hasExtension(GeometryMigrationExtension::getClassTypeId()))
            this->setExtension(std::make_unique<GeometryMigrationExtension>());

        auto ext = std::static_pointer_cast<GeometryMigrationExtension>(this->getExtension(GeometryMigrationExtension::getClassTypeId()).lock());

        ext->setMigrationType(GeometryMigrationExtension::Construction);
        ext->setConstruction(construction);

    }

}

5.2. The migration extension is read in SketchObject::migrateSketch(), which is called from SketchObject::restoreFinished(). This is the relevant excerpt corresponding to the example above:

Code: Select all

        // Construction migration to extension
        for( auto g : Geometry.getValues()) {

            if(g->hasExtension(Part::GeometryMigrationExtension::getClassTypeId()))
            {
                auto ext = std::static_pointer_cast<Part::GeometryMigrationExtension>(
                                g->getExtension(Part::GeometryMigrationExtension::getClassTypeId()).lock());

                if(ext->testMigrationType(Part::GeometryMigrationExtension::Construction))
                {
                    GeometryFacade::setConstruction(g, ext->getConstruction());
                }

                g->deleteExtension(Part::GeometryMigrationExtension::getClassTypeId());
            }

        }

5.3. Upon restoring the information into the right GeometryExtension of the Sketcher, the Part::GeometryMigrationExtension is removed as per your idea.

Hopefully, this will suit your needs