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. 
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 
).
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 
@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 
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:
- 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).
- 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.
- 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).
- 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 
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
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:
- 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.
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" }};
...
}
- 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.
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);
...
}
-
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).
-
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.
-
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:
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():
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:
// 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
Thank you very much for the detailed explanation. I’ll begin migration in my branch soon. BTW, do you think it is appropriate time to integrate my list move changes we talked about earlier? You can find the commit here. I was about to submit this as PR, but then saw that you have pushed your migration changes already. If you ask me, I’ll say it can wait.
I have one question. I am curious about the choice of a std::set in setValues.
Are you trying to cover the possibility of duplicated pointers? or is it for erasure performance as opposed to std::remove on a std::vector?
Just trying to understand how you thought about it.
Just for convenience, to allow the caller to reuse existing list entry. And yes, std::vector can do the same, probably a bit faster too.
About the std::set vs std::vector, I understand there is a potentially higher creation cost (because of the ordering), but erasures should be faster in the std::set unless there is a good option for succesive removals for std::vector (range removal). Probably in the whole it would not make much difference, if any, with the sizes of geometrylist we normally handle.
For FreeCAD master, this will make a small PR and it is easy to review and merge. I do not expect problems arising from it, as the result internally for _lValueList is the same (the same objects end up in the list). I think there is substantially to win, as it avoids unnecessary clones and pointer deletions and it leads to cleaner Sketcher code.
For LinkStage3, I think you will have to “rewrite” (adapt a lot of) the code currently in master because of the migration. I think it would be a pitty that you do the work now and then you have to keep repeating it for other changes introduced in master that you want for LinkStage3 in the future. Because of the clone-all-before-move, this becomes inevitable for every single change brought into master. Diffs are not “clean” and it requires some effort (many times the manual boring kind of effort when eventually one is so tired that makes stupid mistakes that maybe are not so easy to debug, well maybe it does not happen to you, certainly it happens to me).
For these reasons, I think it makes sense, if you prepare the PR for the current FC master code, to merge it now. Then, it will be easier for you to merge the other sketcher changes into your branch too. Future differences will be cleaner too.
I learned sketch.Geometry.Tag sometime ago is ‘semi-persistent’ - it is persistent in a session, but will get reassigned on file restored.
>>> s.Geometry[0].Tag
'fc445b44-0091-400e-8fdb-b71b47b2195e'
>>>
Just notice there is an id in the Sketch.GeometryFacadeList (sound something like Realthunder’s ID?)
Have some test - it seems it is similar to Tag, ‘semi-persistent’, re-assigned upon document restored right ?
Thanks for any idea !
>>> s.GeometryFacadeList
[<GeometryFacade ( Id=409 ) >, <GeometryFacade ( Id=410 ) >, <GeometryFacade ( Id=411 ) >, <GeometryFacade ( Id=412 ) >, <GeometryFacade ( Id=413 ) >, <GeometryFacade ( Id=414 ) >]
>>> s1=s.GeometryFacadeList[0]
>>> s1.Id
409
Randomly delete an edge.
>>> s.GeometryFacadeList
[<GeometryFacade ( Id=409 ) >, <GeometryFacade ( Id=411 ) >, <GeometryFacade ( Id=412 ) >, <GeometryFacade ( Id=413 ) >, <GeometryFacade ( Id=414 ) >]
Re-open the file
>>> s.GeometryFacadeList
[<GeometryFacade ( Id=739 ) >, <GeometryFacade ( Id=740 ) >, <GeometryFacade ( Id=741 ) >, <GeometryFacade ( Id=742 ) >, <GeometryFacade ( Id=743 ) >, <GeometryFacade ( Id=744 ) >]
>>>
This ID field is reserved for my use for topo naming stuff. I don’t think the upstream is currently using it. I have already migrated to it in my branch.
Yup. I put it there to facilitate sketcher Development, merging from master to LinkStage3 and future RT’s integration of Sketcher features. Be wary of using it in current master, as it is neither used, nor tested.
Good to hear
Thanks both for explanation
Hi, a short question trying to use the extension
- Create a new Sketch
- Draw a few edges
- Without using extension like setExtension, getExtension() return below
>>> sg=s.Geometry
>>> s1=sg[0]
>>> s2=sg[1]
>>> s3=sg[2]
>>> s1.getExtensions()
[<SketchGeometryExtension ("447") >]
>>> s2.getExtensions()
[<SketchGeometryExtension ("454") >]
>>> s3.getExtensions()
[<SketchGeometryExtension ("485") >]
Curious what are the figures returned?
Thanks!
AppImage 0.19.23756 on Fedora 31
OS: Fedora 31 (Workstation Edition) (GNOME/gnome)
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.19.23756 (Git)
Build type: Release
Branch: master
Hash: 9c6e9184930a52b165a0b7274e3a45d1006bfe67
Python version: 3.8.6
Qt version: 5.12.5
Coin version: 4.0.0
OCC version: 7.4.0
Locale: English/United States (en_US)
Another question
>>> sg1.setExtension(Part.GeometryStringExtension("","Living Room"))
>>> sg1.setExtension(Part.GeometryStringExtension("","Dining Room"))
>>> sg1.getExtensions()
[<SketchGeometryExtension ("3611") >, <GeometryStringExtension ('Living Room', ) >, <GeometryStringExtension ('Dining Room', ) >]
But…
>>> es=sg1.getExtensionOfType("Part::GeometryStringExtension")
>>> es
<GeometryStringExtension ('Living Room', ) >
Is it supposedly to work like this? I can have more than 1 GeometryStringExtension and able to read it back right?
Thanks again