Sketcher Development - Integration of Extensions

General Development
1

Hi there!

A word on development

Since last week I am beginning to reintegrate into development. From now on, I think I should be able to do some continuous development even if it is at an slower pace.

What is geometry extensions

At this point I would like to work to develop sketcher geometry extensions. To refresh the memory, the idea behind geometry extensions is to extend Part::Geometry to store (workbench/application/macro specific) additional information (and provide additional functionality) to Part::Geometry without inserting this information in the Part workbench. The ultimate reason being not to bloat Part::Geometry with information specific to other workbenches and enforcing a clear separation between workbenches.

Although I believe it is not widely used (maybe nobody uses it at all), since a couple of years it is possible to add basic type information (bool/string/long) to geometry from Python, for example to mark geometry with indices or strings, to be used by macros or workbenches. So, for example if you would like to mark a line segment in the sketcher with the human readable comment “North Wall”, you can do this already in FreeCAD, it will be saved and you could retrieve this information, for example from a macro, upon opening the saved file.

However, the main idea behind geometry extensions was to enable to create more complex structures supporting more complex functionalities, for which Part::Geometry was too constrained. My focus is the Sketcher.

Development ideas

There are several functionalities that I would like to have in the Sketcher.

One is “defining external geometry” (the ability to make an imported geometry aka “external geometry” to behave as normal geometry, in other words, making a magenta sketcher line to behave as a white sketcher line). Once I had a functional PR that did this in another way, where the outcome was not satisfactory from a coding point of view and was not merged. The idea is also implemented in realthunder’s linkstage3 branch.

Another idea is sketcher “Geometry Layers”. The idea behind this is to allow to structure sketcher geometry into logical layers. This would enable among others independent visualisation of groups of geometry (for example, to hide the geometry “copied” via carbon copy while still using it not to clutter the view). From a functional point of view, the grouping in layers could enable extended carbon copy functionality, such as the ability to adapt or synchronise on demand the geometry “copied” via carbon copy. This should help a lot with the major drawback of “carbon copy”, if have to change the original sketch, you have a lot of tedious manual work to do in every single copy. There are other ideas and surely others will come.

Integration or Divergence with realthunder

I already mentioned above that realthunder has some work done in his branch in the sketcher. Aside from the “defining geometry”, there is an improved toponaming and a geometry export functionality. Aside from the functionality that the user can see, he has some improvements, for example, he has also a mechanism for propertylists to avoid copying/moving elements that did not change.

I am not a hateful person, but if there is something that I really hate is wasting effort. I think we should strive to be efficient and take advantage of any synergy we can find. So I would really like to integrate as many improvements as possible from everywhere, but specially from realthunder’s branch. This should facilitate realthunder’s work of integrating the commits he wants from FreeCAD and our work to integrate his commits that we want to integrate. Even when “improvements” cannot be merged (for example in cases that there may be disagreements on code structure or architecture), I would really like to have this disagreements encapsulated with homogeneous interfaces that are used in the same way. This should facilitate realthunder’s work and ours when integrating each other’s improvements. Needless to say, I strive not to have disagreements, as there is no better situation that not having to maintain “disagreements” at all.

With this in mind, the first think I have done is to see the differences of key sketcher components between realthunder’s branch and mainstream FreeCAD. Some changes in the Sketcher come or rely on changes expanding the whole FreeCAD project. One such change is the improvement on propertylists I was referring above, which leads to a substantial change in how to use the interface of the property for moving operations (master today clones the whole list at the Sketcher and moves the whole std::vector to the property, whereas RT’s work moves a mixture of unchanged geometry and changed geometry, where only the latter is cloned at the sketcher and it relies on the propertylist to compare that is new to properly handle pointers and perform memory management). This is a rather common operation in the sketcher and it will lead to having differences and rewriting code when trying to bring one functionality between the fork and FreeCAD master. This rewritting on merge is also error prone. I think this is a kind of functionality that could be accepted in master and would result in less maintenance effort for everybody.

Another change that I have not had yet the time to properly investigate relates to the ability to access sketcher individual elements from the outside name/subname. This I think only relies to the geometry export functionality of RT’s work.

Another change that I have not yet properly investigating relates to the toponaming.

One could think: well, there are only three or four changes, it is not that bad. But the reality is that the changes start to be not so small and it starts to diverge more than I would like to and the prospect is only to get worse, specially when I implement defining geometry and layers, if we were to end up using different mechanisms. That may be a point of no return or at least one that accelerates the divergence in a way that it will make future integration way much worser. I would really like to discuss to avoid this situation.

Way forward

I have done initial work to integrate sketcher geometry extensions to provide almost the same interface realthunder is using in Part::Geometry. Because it appears to me that the needs of normal geometry and the geometry obtained from external geometry are different, I have created different extensions for these two groups. I still have some work to do and I will post in a couple of days (within a week) with more detail, but the best solution I have found so far and the one that I currently pursuing is to agree with realthunder to use geometry extensions for sketcher specific data instead of Part::Geometry directly. Then have most, if not all of, his sketcher code integrated with this new interface. I am willing to review sketcher PR within a reasonable time to this end.

For cross project changes that appear to be helpful for everybody and affect the sketcher code, the issue with propertylists above comes to mind at this moment, I would really like to arrive to an agreement to merge something that would enable to use the interface in the same way. I am willing to put effort on my side and review and test such project-wide PRs, discuss them and seek Werner’s approval.

Of course, I come with an open mind an I am willing to commit a reasonable amount of time and effort to this. Sorry for the long post.

2

Hi @abdullah, although being far from a real programmer, I have been trying to using the Part Geometry Extensions, with my very limited time and very beginner python knowledge, and thanks for your earlier time in mentoring indeed :smiley:


Exactly the idea I tried to test - currently, I only use a dict in SketchPythonObject to e.g. mark each edge ‘Wall Width, Align’.


This is great to have this ‘built-in’ - I do something similar again in SketchPythonObject, with your earlier again help. The crux of this feature to me is 3-folded - 1st and 2nd is the ‘link’ and ‘linked edge’ are ‘toponaming’-tolerant (learned Realthunder’s branch did this), and 3rd, to me, is the ability to ‘link’ also to the Construction Edge of a Sketch (Realthundar’s branch seem not able to do this atm :wink: )

(Just have discussed in point 3 of this post today)


Long awaited feature to me again :smiley:

And indeed, good to avoiding double-handling and/or incorporate @Realthunder’s already done features !

3

abdullah, your long post is definitely worth reading! I fully agree with what you say about usage wasting effort. Furthermore, the ideas and improvements you’ve described are going to be very useful from my point of view.

4

Thank you for the feedback.

Geometry extension development: Geometry facade

Today I have merged sketcher code that develops the concept of sketcher geometry extensions to be able to treat extended geometry more conveniently for c++ developers (GeometryFacade and ExternalGeometryFacade). There are also Python counterparts of these as I want to provide convenient access to all this universe for Python developers.

The following is meant mainly for developers, but I will try to explain it as simpler as possible.

Until now there was geometry Part::Geometry, there were extensions (parent class Part::GeometryExtension), there were unused sketcher extensions (Sketcher::SketchGeometryExtension and Sketcher::ExternalGeometryExtension). This code was a previous effort to provide an interface compatible with Realthunder defining geometry. However access to this extensions implied obtaining the extension from each geometry reading or mangling it and setting back the extension. The fact that sketcher geometry relevant data members are spread over a geometry object (for example construction), and potentially several extensions (a defining external geometry will be have an ID and layer information, which is or will be stored in the SketchGeometryExtension, and information due to its defining status, for which it will have a ExternalGeometryExtension too), leads to overcomplicated difficult to read, error prone and cumbersome to maintain code. One solution is to use a Facade object that integrates in one object the interfaces of geometry and extensions.

A Facade is a well-known design pattern that provides an interface for a subsystem (in this case the Part::Geometry and the extensions). The intent is to enable to interface with a (potentially complex) subsystem via a simpler interface (which may not necessarily offer all the functionality of each of the components of the subsystem).

A first Facade (Sketcher::GeometryFacade) object operates on a Part::Geometry object and its SketchGeometryExtension, and inherits the extension interface (in full), while it only offers the part of the functionality of Part::Geometry relevant for common sketcher operations. Currently SketchGeometryExtension only features an Id (which is unused and is intended to support RT’s long Id in his Part::Geometry), but it will have for example a layer index indicating the layer where a given geometry is to be drawn. An example:

const std::vector< Part::Geometry * > &vals = getInternalGeometry();
auto gf = GeometryFacade::getFacade(vals[GeoId]);
auto id = gf->getId();

This Facade also offers some convenience utility functions, for example copying an Id from one Geometry to another can be as simple as this (last line only relevant the others provided for context):

const Part::Geometry *geo = getGeometry(GeoId);
std::unique_ptr<Part::GeomBSplineCurve> bspline(new Part::GeomBSplineCurve(curve));
Part::GeomBSplineCurve * gbsc = bspline.release();
GeometryFacade::copyId(geo, gbsc);

A second facade Sketcher::ExternalGeometryFacade, inherits from both SketchGeometryExtension and ExternalGeometryExtension, as an external geometry as an Id as normal geometry does, plus it has additional status flags (defining, detached, missing). These are based on realthunder’s interface to support some of the additional functionalities of his branch.

Divergence/Integration with LinkStage3

The PR above does not create extensions (unless explicitly commanded from Python), as it is not currently invoked by SketchObject. So this PR neither diverges, nor integrates any work.

Following the GEN_ID macro of Realthunder, but using a real function instead, a placeholder (no actual calculation of Ids) could look like this.

Merge of functionalities of LinkStage3

In the meantime I have tried to explore the amount of work necessary to integrate realthunder’s functionality.

There are some tens of commits difference over 3 years only for the sketcher (source git log HEAD..realthunder/LinkStage3 src/Mod/Sketcher):

commit 6c43b147bf5a77601e51ba48d87dad0c4f105fc7
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Oct 22 10:18:44 2020 +0800

    Sketcher: shorten constraint property display

commit 654f1d5e437ebd1b938fb2575cc62ada3fea012d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Oct 22 10:18:04 2020 +0800

    Sketcher: add sanity check on constraint expression when restore

    Related #54

commit 9a04770542a669b8c17bc4057c924d77e5940d5d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Oct 22 10:17:35 2020 +0800

    Sketcher: fix potential crash in rebuildExternalGeometry()

commit 456f9b236cc82e72455efcd73617304093a1abcc
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Oct 21 09:55:10 2020 +0800

    Sketcher: fix missing transaction for undo/redo in EditDatumDialog

commit 999405b33189252b4bb7ce564ec127039db332fc
Merge: 95367eb253 bc6fefe3a0
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Oct 16 05:39:34 2020 +0800

    Merge 'upstream/master' into LinkStage3

commit f1f2ea088215d1ae5f090c892dde6489e55b0853
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Oct 9 13:03:59 2020 +0800

    Sketcher: fix CommandSketcherBSpline build Windows

    Fixes realthunder/FreeCAD_Assembly3#321

commit 39514734b6ed396d97f987637c9bd36588c53fa2
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Oct 4 18:05:24 2020 +0800

    Sketcher: fix potential crash


commit ab88228d823feab9ca7e4fab19ff83b8c197e3bf
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Sep 28 12:33:36 2020 +0800

    Part/Sketcher: support auto grid scale in ViewProvider2DObjectGrid

commit 2695ba34d0d50d1c02e3d7cfd1587c2d2bcff1be
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Sep 26 07:13:34 2020 +0000

    Sketcher: fix pixmap cursor hot spot

commit 5a632c21f75d9347c38db03866f5f2a1aa2d1d59
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Sep 26 04:05:56 2020 +0000

    Sketcher: update all open documents on constraint renaming

    In case of external linking

commit 4d81bc69ee5690e8d98dea055f5e3cab7a3974fc
Merge: 921df5da81 0306c237ed
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Sep 25 13:09:53 2020 +0800

    Merge 'upstream/master' into LinkMerge

commit 3a270db626c3e311059176290504fb4a1e423f78
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Aug 16 19:37:25 2020 +0800

    Part/PartDesign: improve topological naming

    Reduce direct use of TopoDS_Shape as much as possible, in favor of
    TopoShape, which carries the topological name mapping.

commit 281f8a5236501a6d8c08e2b89ade018779b291c1
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Aug 5 09:46:45 2020 +0800

    Sketcher: fix build warning

commit fa664bad77956db24cbdde84df2510308f7c6c8d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Jul 27 11:09:05 2020 +0800

    Sketcher: reduce unnecessary recompute on editing finish


commit 902ccd15bddf6543107a2b6d94ac90a8dc702fc6
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Jul 17 21:24:36 2020 +0800

    App/Sketcher: fix linked ObjectIdentifier::canonicalPath()

    Fixes realthunder/FreeCAD_Assembly3#311

commit 07c1eff2cbbfe55b9fe4ad34b472a138b9ceb530
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Jul 17 17:29:11 2020 +0800

    Sketcher: fix undo/redo handling

commit 6bbc742ebd081c5c8e92f85672f2a74ce5956dbe
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Jul 17 17:27:36 2020 +0800

    Sketcher: fix PropertyConstraintList related problems

    Fixes realthunder/FreeCAD_Assembly3#310

commit 08d98012bc060b20f7f237931d2b54a3123f0002
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Jul 17 08:36:16 2020 +0800

    Sketcher: fix addConstraints()

    Error caused by merging with upstream

    Fixes realthunder/FreeCAD_Assembly3#309

commit 7087d2647deb6b4fc5a26de96dc35f990134fea0
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Jul 15 07:43:00 2020 +0000

    Sketcher: enable PropertyConstraintList::isSame()

    Because sketcher constrain is often used with expression. Enable
    isSame() can greatly reduce recomputation due to unrelated changes in
    spreadsheet, such as add/remove rows.

    Related realthunder/FreeCAD_Assembly3#306

commit d2fa6450cbde623936840604274a45e01eff3ce3
Merge: c1d8e4dc91 6e40c19f73
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Jul 11 12:50:51 2020 +0800

    Merge 'upstream/master' into LinkMerge

commit 58609b6fdbc4eef82f44ff58c30d65658b95e415
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Jul 4 16:45:46 2020 +0800

    Sketcher: temporary fix of external reference losing mapped topo name

commit c8043169fa5027834f14aab64326bd9cabfae869
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Jun 4 04:29:41 2020 +0000

    Sketcher: fix selection on editing

    The editing sketch may live in a non-active document

commit 204a7e3740ecf41535df42f76b1720b3e647d2e1
Merge: 94e99c6a99 99a99f48ef
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu May 28 17:20:11 2020 +0800

    Merge 'upstream/master' into LinkMerge

commit 6ff8bb2fcfb2c8b734e0c1a8ed47843fc71bf2a2
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed May 13 05:15:31 2020 +0000

    Sketcher: relax migration error check

commit 43874d7136a91d391683a81a2c2e13b614a21650
Merge: 4c2fc7ce60 419e8f7a99
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun May 3 10:08:20 2020 +0800

    Merge 'upstream/master' into LinkMerge

commit 53cea697b13c96905e8e10cf634b5c0af6941b99
Merge: 5c425a09b7 2d29ac5fc1
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Apr 16 08:00:13 2020 +0800

    Merge 'upstream/master' into LinkDev

commit e4584793c323fe76a39fe631ebd1008c0c3f32dd
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Apr 8 15:11:37 2020 +0800

    Disable copyBeforeChange() in some property

    Because it causes ill behavior and requires more changes in
    cooresponding workbench.

commit 604b89308f410df3b02da6ff510596cef89d5b1e
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Mar 31 13:15:03 2020 +0800

    App: improve document recomputation

    The recomputation optimization is enabled by parameter
    BaseApp/Preferences/Document/OptimizeRecompute (default to true). It
    works like this,

    Property::aboutToSetValue() will make a copy of the property using
    Property::copyBeforeChange(). Property::hasSetValue() will then compare
    the current property value with the saved copy using Property::isSame().
    If the values are the same, the property will not be touched, and hence
    not call PropertyContainer::onChanged().

    DocumentObject adds a new _revision that will increase inside its
    onChanged() function, unless the changed property is marked as
    Property::Output/NoRecompute. Document::_recomputeFeature() is optimized
    to call the features recomput() if and only if any of the unmarked
    property is touched.

    The net effect of this optimization is so that the object is recomputed
    only if any of its property has actually be changed, or any of its
    linked object is changed.

    Property::isTouched/purgeTouched() are changed to virtual to allow
    customization. PropertyLinkBase overrides these function to report
    touched in case the linked object's revision number changes.

    Property::isSame() has been changed to pure virtual function, forcing
    every property to provide a more efficient implementation. For
    properties that do not support comparision, it shall always return
    'false' from isSame(), and also return nullptr from copyBeforeChange().

    User code can force touch the property by calling Property::touch(),
    or force recompute object by calling DocumentObject::enforceRecompute().

commit 06d2efcefdf0fb3b19a45aadc92521739c556c45
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Mar 18 07:09:02 2020 +0800

    Sketcher: fix typo during merge

commit 96f97995235cc66f458ac6f40e7cd83071a8f0b1
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Mar 7 13:01:11 2020 +0800

    Fix build for MacOSX

commit 18f9a70a8f5dca87e0e60d27145e9a9fb0a10dfd
Merge: bbde42384d c782435b7b
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Feb 28 16:22:32 2020 +0800

    Merge branch 'origin/ExprCompleter'

commit 73be870c5c2a009aaf97c8fa2970de34e57216af
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Feb 27 09:59:58 2020 +0800

    Gui: improve support of named sketch constraint in Expression completer

commit 9f2b113fabb47eedf31a30c75538361b1b97aecc
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Feb 2 15:37:17 2020 +0800

    App: change Property::getPaths() and getPyPathValue()

    Since ExpressionCompleter will now look into Python attributes, there
    is no need to expose duplicated information through getPaths(), except
    those that provides extra information (e.g. Rotation.Angle with extra
    unit information)

    getPyPathValue() is modified to return Python attributes after the
    given path.

commit 21e6e898962681f3cc8c757633ad4d6a00328b5c
Merge: e24bc133c3 01e8e7f777
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Feb 18 12:16:20 2020 +0800

    Merge branch 'upstream/master' into LinkDev2

commit 09e960d7d4f2d91698fc10f03017a4c0eb65631c
Merge: 9ac862bb94 1d9a3ab790
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Jan 15 08:04:40 2020 +0800

    Merge branch 'origin/Screenshot' into LinkDev

commit d72a496d0899c28b37084babab6e8cfb6fd98c38
Merge: dcfb230e9d c075380674
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Jan 10 14:07:47 2020 +0800

    Merge 'upstream/master' into LinkMerge

commit bd119f7e0b6d67ab30fdd17d4a9198399ef160ea
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Aug 30 11:02:16 2019 +0800

    Sketcher: fix missing update on expression change

commit 51db4153731ac112f2b8c72a30f38565a0db6e76
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Aug 23 18:58:14 2019 +0800

    Sketcher: fix constraint update in undo/redo

commit bf3031326ef4fa8c14e83bca3b0470680e58f8f4
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Aug 7 08:41:53 2019 +0800

    Sketcher: auto undo when canceling constraint editing

    Related realthunder/FreeCAD_Assembly3#207

commit 1dfd9b93c9f9a1bfe56e1aa940b118c933b2de1c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Aug 5 12:32:27 2019 +0800

    Support save/restore document into/from directory

    This is supposed to be the first step towards a proper support of
    version control in FreeCAD. The commit enables user to save a document
    into a directory without compression. Here is a summary of the changes,

    * Add proper support of XML characters content in Base::XMLReader and
      Base::Writer. Use XMLReader::beginCharStream() to stream either binary
      or text data. Binary data will be base64 encoded on the fly with user
      defined line size. The base64 string is stored as plain character
      content inside the current XML element.  Text data is stored inside
      CDATA section with automatic detection and handling of CDATA end tag
      inside the text. Be WARNED though, there is no validation of text
      character yet. The user must ensure the input text is valid UTF8.

    * Add three properties to App::Document to let user fine tune directory
      saving options for better version control. They are,

      * ForceXML, a integer specifying the level of preference to store
        property content inside the XML file. 0 being the least prefernce,
        i.e. store everything into separate files if possible.

      * SplitXML, a boolean value to decide whether to split each object's
        content into separate XML files.

      * PreferBinary, a boolean value indicating the preference of stroing
        property content as binary or text.

      These options only works when saving document into a directory. The
      default setting, which is also the recommended setting for version
      control is, ForceXML=3, SplitXML=true, PreferBinary=false. The default
      options can be changed using parameter editor with path
      BaseApp/Preferences/Document

    * Modification in various properties to support the above options.

    * Various GUI modifications to allow chooseing directory (and special
      file name Document.xml) for saving.

    * New signalDocumentFilesSaved to signal any files addition and removal.
      This is intended to be used by version control modules to automate
      file adding and removing.


commit 6205d58e5733cf63054d093d8e727a4239329388
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Aug 5 08:04:03 2019 +0800

    Sketcher: fix external editing

commit 9c6d77d0efd3534a85497ea0489631129f0d7831
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Jul 16 13:45:10 2019 +0800

    Sketch: fix in-place editing

commit 5c89db55b80370e85f35afa26b95cb2eb7e8efab
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Jun 27 12:49:55 2019 +0800

    Sketcher: fix missing external geometry during export/import

commit 53093e9cccdfe7eecf656878cb92830e00e40387
Merge: cecb599b00 d7b6d4dba9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Jun 21 12:32:25 2019 +0800

    Merge 'upstream/master' into LinkDev

commit 9a566c06b5e5c7ac68ecf9430061a226dd800256
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri May 31 06:28:00 2019 +0800

    Gui: add coinRemoveAllChildren to work around Coin3D bug

    See bug description:
    https://bitbucket.org/Coin3D/coin/pull-requests/119/fix-sochildlist-auditing/diff

    Because of path based rendering (SoFCPathAnnotation) in mouse over
    highlight, this bug causes crash more frequently here comparing to
    upstream.

    All C++ calling of SoGroup::removeAllChildren() is replaced by
    Gui::coinRemoveAllChildren(), and python code is fixed by monkey
    patching SoGroup.removeAllChildren() in FreeCADGuiInit.py.

commit f8ebbcddb5acd312609f82ededbffa671e54169b
Merge: 647f993148 2a9bd6d0e7
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed May 22 14:47:08 2019 +0800

    Merge 'upstream/master' into LinkMerge

commit 3147600a2d5ce3e9b5c3b755503e55d774d13d99
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun May 19 10:57:39 2019 +0800

    App: improve AutoTransaction for better undo/redo

    Merge Gui::Command::AutoCommit function into App::AutoTransaction for
    auto transaction management. See document in AutoTransaction for more
    details.

    This patch also makes sure any modification done by any Gui::Command is
    undoable/redoable without requiring to modify user code.

commit 77aa17c97cca43de93f8865bb2fa22339165f726
Merge: 262db980be df38102017
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun May 5 14:35:09 2019 +0800

    Merge upstream/master into LinkMerge

commit 58a3b25b0dc584e7631da3e64eb7dba54bc7f1b3
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Apr 25 11:38:42 2019 +0800

    Sketcher: fix constraint expression

    Fixes #11

commit 917a881efdb35ed6ad3593a9ed4ec4dea7347b19
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Apr 25 11:38:20 2019 +0800

    Sketcher: selection sync of constraint task panel



commit 556821135d4c3d13dd8488cefce37a0178768716
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Apr 22 08:46:35 2019 +0800

    Property: add new API getPyPathValue()

    To prepare for switching Expression internal calculation to Py::Object.

commit 1204593d8070ef07b2a61428311cbc674d0b80f9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Apr 14 20:54:32 2019 +0800

    Expression: move comment into Expression

    The comment is previous available through
    PropertyExpressionEngine::ExpressionInfo. However, this field is not
    really exposed to end-user at the moment. This patch moves the comment
    field into Expression. The plan is to use this field to store meta
    information for various PropertyExpressionContainer, through its API
    get/setExpressions().

    One use case will be implemented in the following commit which
    store/restore spreadsheet cell meta information, such as style, alias,
    etc.

commit 99da55178cc846f82c952ca499bc785ed74b243f
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Mar 29 17:02:15 2019 +0800

    Sketcher: fix undo/redo when editing constraint expression

    Fixes realthunder/FreeCAD_Assembly3#207

commit f670d46f2d1255867b3330052496894cf9ff7a23
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Mar 9 10:18:52 2019 +0800

    Sketcher: fix crash on deleting element in task panel

    Fixes realthunder/FreeCAD_Assembly3#199


commit 5122d611b055099b99506c6ac756111eddc3397f
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Feb 1 21:30:47 2019 +0800

    Sketcher: disable signal on slotElementsChanged()

commit 23826fe06bf1ab95054ddf7a8a4faa93160da4bf
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Feb 1 21:30:30 2019 +0800

    Sketcher: fix potential crash on updateColor()

commit e782f5e79e9cc3e6deba4579aec85d31aafd634b
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Jan 28 11:42:10 2019 +0800

    Sketcher: allow undo sketch recompute after editing

commit 001dc41a6cdfde0c71fe239df22c4853f8d85258
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Jan 26 17:17:37 2019 +0800

    Sketcher: fix update driving constraint when not auto update

    Fixes realthunder/FreeCAD_Assembly3#175

commit 9ad61c89ed87cdd283b746d661c49e11f9be79a9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Jan 26 16:46:40 2019 +0800

    Sketcher: fix ViewProviderSketch double click handling

    Fixes realthunder/FreeCAD_Assembly3#176

commit 7955d7ca5ebb1bc381979d322fe04879efb57af3
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Jan 18 20:44:27 2019 +0800

    Sketcher: fix missing signalElementsChanged on external geometry change

commit 362441873b984a4f169fd430f989aee1016a337e
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Jan 9 07:30:24 2019 +0800

    Sketcher: fix SketchExport command

commit 0cb1f4bacdda9df7488f160ad5a8a2809bf90ed4
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Jan 8 14:18:12 2019 +0800

    Command: add TriggerSource to handle checkable action

    Current implementation of Command has trouble syncrhonize the checked
    status with checkable action, becuase the command can either to
    triggered from Python code or Qt UI. And for ActionGroup it can also be
    triggered from a child action. This commit introduces the TriggerSource
    as an additional argument to Command::invoke() to help the command
    logic syncrhonize checked state, in the form of a helper function
    Command::setupCheckable(). See PythonGroupCommand::activated() for an
    example usage.

commit ca371d11271a4e7415adcce2a26f1b3aa823cb0f
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Jan 3 12:28:57 2019 +0800

    Sketcher: fix non-undoable change when editing

commit eaa2d93c0dd0db2e7e2f650e2df8071621f5e5e6
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Jan 2 19:33:45 2019 +0800

    Sketcher: fix toggleConstructions()

commit 0a41512e62ae42b37876093df0ef158753f1cd76
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Jan 2 19:33:22 2019 +0800

    Sketcher: sync element selection in TaskSketcherElements

commit 66f482ef6bdc245ac8b92e568eacd46fb2467eae
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Dec 28 14:58:49 2018 +0800

    Sketcher: add commend for auto fixing geometry reference

commit 911597cb6d5122a0dadcc0d1bf947b529ae47783
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Dec 28 14:56:36 2018 +0800

    Sketcher: fix preselection highlight for external geometry

commit b6711bc1cc4eb426df680cfb9a0d512e660a92a4
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Dec 25 21:01:45 2018 +0800

    ViewProviderSketch: fix missing commitCommand on drag constraint

commit 1de7f47a5a355242573d2135a5ec3c8ac8b4cafa
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Dec 25 21:03:31 2018 +0800

    Sketcher: fix migrating of older skech

commit 7e58e9cffec2b6575c084805d9342383e2d1688d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Dec 25 21:02:22 2018 +0800

    Sketcher: fix TaskSketcherElements highlight of external element

commit b2bba4cef9512df649ad8141988606f1cf741245
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Dec 25 12:17:05 2018 +0800

    Sketcher: change axis geometry ID to avoid conflict

commit d298939986c2f6cc5af5f80655f0182f3d97125b
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Dec 24 18:27:28 2018 +0800

    Sketcher: fix external geo ref update

commit 6fb24f438c831f09201ad6d55477be8b1ebe671c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Dec 19 18:17:26 2018 +0800

    Sketcher: refactor external geometry handling

    External geometries are now persisted into files, and can be

    * defining, for building shape,
    * frozen, do not update while external object
    * detached, do not associated to any external object
    * re-attached, change the associated external geometry element

    Missing of any external reference no longer silently delete all
    associated constraints. Instead, the error will be explicitly shown to
    user, and all geometry and constraints remain intact. The user can
    decide whtether to detach or re-attach the external geometry.

    Fixes realthunder/FreeCAD_Assembly3#76

commit 5c6272dd25e41bfb7585344bd9ea9e5c555f1205
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Dec 19 18:17:12 2018 +0800

    Sketcher: add some icons

commit 52718c851c7c3d3ddfa1bebe6595c6a0fa85b394
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Dec 21 07:21:27 2018 +0800

    Sketcher: fix PropertyConstraintList undo

    Fixes realthunder/FreeCAD_Assembly3#166

commit 5c2f01826312a00bfaff2edbfaaca7a290fcb2f0
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Dec 13 17:04:21 2018 +0800

    Improve error message

commit 11931cf870e2ebdab4e16a8201763f524aab6292
Merge: 45d9defdbf 73df4e6fc0
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Nov 25 17:44:36 2018 +0800

    Merge upstream master into LinkMerge

commit 9d0c3339a9a0f00cd9bc07b8f6cee315906e2acf
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Nov 5 17:15:06 2018 +0800

    App: replace boost::any with stx::any for performance

    Mostly for small body optimization, as boost::any uses dynamic
    allocation to hold any data inside. See src/App/stx/any.hpp

    Original implementation from

    https://github.com/thelink2012/any

    With modification from

    https://github.com/tcbrindle/cpp17_headers

commit 3008b30596ad1ed9a63db199f0c377bd949f3968
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Nov 5 16:27:45 2018 +0800

    Expression: refactor for better performance

    * Remove raw pointer usage in Expression, replace with std::unique_ptr,
      and enforce C++ move symantics on expression construction.

    * Use boost::pool_allocator to improvate memory allocation speed.

    * Use (a patched) bison C++ parser for performance and exception safty

    * Remove all Python interpreter useage, use Python API directly instead.

commit 6af8bcec1e55d0e5829bb239e4f8b593cbb6efce
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Oct 8 07:36:47 2018 +0800

    Expression: refactor

    Details will be documented at
    https://github.com/realthunder/FreeCAD_assembly3/wiki/Expression

commit 04ccbe4b5bf30a63d03c81811100d46f3034e584
Merge: 63dd5552a5 052d8a70e9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Sep 12 14:24:02 2018 +0800

    Merge branch 'upstream/master' into LinkMerge

commit 2c0a8deea1b96c649070c7bc8b680f45c82017fa
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Sep 7 16:33:54 2018 +0800

    SketchExport: fix update problem

commit 6921476dfed8de7c4e4192a902924e58c5ac853f
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Sep 1 13:30:04 2018 +0800

    ViewProviderSketch: filter external object in onSelectionChanges()

commit f9b8048d863ea79e51d5bdc4f8fdd8f4c21e22c8
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Aug 20 10:59:16 2018 +0800

    Use the new ObjectIdentifier constructor to reduce ambiguity

    All modifications uses ObjectIdentifier to capture the path of its own
    property. The new ObjectIdentifier explicitly sets the document object
    name as the owner name to avoid runtime resolving ambiguity where there
    is an object with same name as the property.

commit d5685b8f19aa97c25f37ee2d72cf032712424ad3
Merge: 9374feae10 123e9d210d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Aug 6 16:08:07 2018 +0800

    Merge upstream/master into LinkMerge

commit 960e63fa0518362c51823214ee842ed522a35b1e
Merge: 05453c6c04 8ef330d19a
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Jul 30 16:30:38 2018 +0800

    Merge upstream/master into LinkMerge

commit dc940ca3b3dc839045f4fc0a2665655d9875355c
Merge: c14743cf07 ab1520b872
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Jun 30 17:17:01 2018 +0800

    Merge 'upstream/master' into LinkMerge

commit 7b636f5c8ab8dc922f5959fe9605113aad6d6fd7
Merge: 99ae7ede21 e49a5af3ae
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Jun 2 11:33:34 2018 +0800

    Merge 'upstream/master' into LinkMerge

commit 6599d8e6ef215eb76fef29513a961bfefbae2faa
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Apr 25 17:43:29 2018 +0800

    Sketcher: add command to swap geometry ID

commit ad52d35284472ac36d1254c672ccf6b166fe1792
Author: Abdullah Tahiri <abdullah.tahiri.yo@gmail.com>
Date:   Tue Mar 27 07:06:09 2018 +0200

    Sketcher: Bug fix - prevent inter part/body links

    =================================================

    With the support for external geometry during carbon copy, it was introduced a way for creating inter-part/inter-body links. See:
    https://forum.freecadweb.org/viewtopic.php?f=10&t=27700&p=223736#p223736

    This commit closes this door, while still allows carbon copy with external geometry support within the same body.

commit 075b1104f35e4edd913444f4fd9c12262109c8ca
Author: Yorik van Havre <yorik@uncreated.net>
Date:   Mon Mar 26 18:52:02 2018 -0300

    Merged translations from crowdin

commit c1f773491ea357a63832d923ceda6c345bf94f01
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Apr 15 17:43:13 2018 +0800

    SketchObject: fix for the new TopoShape naming scheme

ommit 2996a076422fb9b2312ee2660e6d20b1bf12ac9f
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Apr 12 19:26:24 2018 +0800

    TopoShape: remove appendTag argument in all maker API

    Auto decide when to insert tag in TopoShape::processName.

    Fixed getRelatedElements() for the new naming scheme.

commit 18bd442c38df803b154f63f7ab3c70bd3f0cc432
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Apr 7 08:09:04 2018 +0800

    Sketcher: explicitly use shadow subname in external geometry

commit 833a0d4b56feab6d3269d1548cd55e42cca7aa11
Merge: 76593a0e44 f5bd719a6d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Mar 26 18:03:47 2018 +0800

    Merge 'upstream/master' into LinkMerge

commit 752d0dee28e67623529cbd5472cdf010156f7948
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Mar 13 06:45:20 2018 +0800

    Sketcher: fix vertex export

commit efbea6fe92847c1a86a9341b02008f7ebe4fffa6
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Mar 11 09:17:47 2018 +0800

    Sketcher: change element mapping op code

commit b7b9031fda1a383671c69ae1adbedb2dddc11711 (tag: asm3-0.3)
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Mar 6 07:48:48 2018 +0800

    Sketcher: fix SketchObject getSubObject

commit 7cf0beb6c024fd0cb3739de843d7ccafc6de8035 (realthunder/LinkDevMerge)
Merge: 0a95adbd72 f189c95984
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Mar 5 16:25:45 2018 +0800

    Merge remote-tracking branch 'upstream/master' into LinkDev

commit c7f9858f9e9ed3109f8ce6b976401c028be3551f
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Feb 28 20:19:31 2018 +0800

    Sketcher: migrate sketch to new element map framework

commit 2ddd4b44b900ff718b856ac54d9f99f31e091087
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Feb 24 10:29:24 2018 +0800

    Sketcher: add geo history to resuse deleted geo id

    The behavior is controlled by the following parameter:

        BaseApp/Preferences/Mod/Sketcher/GeoHistoryLevel

    0           : disable,
    1(default)  : reuse id only if the new geometries's both start and end
                  points matches some deleted geometry,
    2           : resuse id when either start or end points matches some
                  deleted geometry.

    It works like this. The sketch object will keep an adjacency list of
    each existing geometries. Every time the geometry changes, a new
    boost::geometry::retree is built based on existing vertex positions. The
    adjacency list is updated while the rtree is built. The list will
    sychronize the adjacency id list of all existing geometry, but keep any
    delete geometry id unchanged.

    When new geometry is created, sketch object will query the rtree based
    on the start and end points of the new geometry, and try to find any
    deleted geometries id's that are connected to the same existing
    geometries that owns the hitting points, and reuse its id.

    The net effect is that, say, when you delete an edge in a square, and
    replace it with a bspline, the bspline will have the exact same id as
    the deleted line. So, if you only delete and replace one edge at a
    time, you can change the type of all edges while keeping the same id's.
    Moving points during the process will not have any negative effect.

commit 5fc101b3290555faeec8adb5081e92e38cc9c387
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Feb 17 09:52:25 2018 +0800

    Sketcher: add support for sketch exports

    SketchObject now use the new TopoShape element name mapping to expose
    mapped geometry topological names during editing. SketchExport exports
    the editing geometry using those mapped names.

    The SketchExport acts just like SketchObject to the end-user, except
    that you can't directly edit its content. Instead, when you double
    click a SketchExport, it forwards the editing request to its parent
    SketchObject, and then pre-selects the exported geometries. You can
    then changed the export by selecting/removing geometries.

    The SketchExport is a Part2DObject, meaning that it can be mapped onto
    other faces, independent of its parent SketchObject. If no mapping,
    then it follows the parent SketchObject placement. If mapped, when you
    double click to edit the SketchExport, it will start editing the parent
    SketchObject in the placement of the SketchExport.

commit ca9ae3c897ffa5260009fe40b88318a087a0c8a0
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Fri Feb 9 13:00:01 2018 +0800

    Sketcher: getSubObject support for internal geometries

    When editing, the selected sub-element name will be appended with '_' to
    tell SketchObject::getSubObject() that the element is referring to
    internal geometry. This makes it possible to retrieve the shape of
    internal geometries, such as constructions or external geometries.

    This patch also explicitly set the editing document, object and subname
    when doing selection in 3D view. Because of the support of in-place
    external editing, the editing may originated from various child/link
    inside some container. This patch makes sure that during editing,
    selection in 3D view will cause TreeView to select the correct item.

    Note, there is no support for enhanced selection from other source
    during editing yet, e.g. from various commands or task panels.

commit f405cbcfb65f681366cf4ff09518cc4b95d19c99
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sat Feb 3 17:02:51 2018 +0800

    Remove .orig files added during merge

commit 0daa959650c4bd7454207bd561441dcaa47b58ef
Merge: a5ce0b7d71 bb39cc783a
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Thu Feb 1 16:27:30 2018 +0800

    Merge upstream/bb39cc78 into LinkDev

commit 0ae792076681c53e4e6038499fd022bc0d066612
Merge: 88e940143c 2c69b79c15
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Tue Jan 9 16:23:24 2018 +0800

    Merge upstream 2c69b79c15623a2dd1314f3830921822c8dc07c1

commit 83ecc94604b8ca183f44407188a01a03311a95b0
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Dec 27 18:20:30 2017 +0800

    Sketch: fix external editing

commit 8c10ed790417a7f769d6718fd554a821eda7a24e
Merge: e08f252c4c c54c08cca4
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Dec 13 16:17:23 2017 +0800

    Merge remote-tracking branch 'upstream/master' into LinkDev

    Changes against Link branch to conform with upstream master:

    * Document::recompute() no longer throw exception on cyclic dependency,
      but callss DocumentP::partialTopologicalSort() to do the sort.
    * PropertyContainer no longer treat DynamicProperty with Prop_ReadOnly
      as Python read only attribute. Static property still does.

commit 478e64348ba845f4c1a9f54cff3b6ac7e95a86a0
Merge: 47eef5c3a4 b01c503c2c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Sun Nov 12 01:33:58 2017 +0800

    Merge upstream/master b01c503c into Link branch

commit ea882f5d9c427f409859a90a627c0cabb297fa91
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Mon Oct 16 11:27:57 2017 +0800

    ViewProviderSketch: multi-placement editing support

    Demonstrate how to support editing the same object in difference
    placements, and possibly, different documents.

    Thing to watch for:

    * Do not use ActiveDocument, instead, use the object's owner document in
    case the editing happens in a different document. Or, use
    Gui::Application::editDocument() to access the editing document's
    viewer.

    * Make sure undo/redo is done through the object's owner document,
    which may be different from the editing document.

    TODO: current UI cannot show foreign document undo/redo when editing
    externally linked object.

commit 05631137f44ab46c5d9f2c9041330f7fa37f848a
Merge: f6fc75addb 2751a3e847
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date:   Wed Aug 30 13:33:22 2017 +0800

    Merge remote-tracking branch 'upstream/master' into LinkStage1

I have fast read most of the first 20 commits and several of the rest.

There are several aspects that make integrating changes a daunting effort:

  1. The code of certain functionalities depends on changes outside the Sketcher module (e.g. App/DocumentObject, Part::TopoShape), not currently present in master. Some of these decisions were reverted several times over the LinkStage3 development and are no longer part of LinkStage3. Some others yet, most importantly on Part::TopoShape for the functionality we trying to integrate, are simply not in master yet (examples below).
  2. The code includes some design decisions that may require a wider discussion because of its impact, for example, the code relating to in place editing from another document (e.g. LinkStage3 commits ea882f5d9c427f409859a90a627c0cabb297fa91 (multiplacement editing support), 83ecc94604b8ca183f44407188a01a03311a95b0 ( Sketch: fix external editing)).

In order to grasp the most important hinderings, I have done the exercise of cherry-picking some basic important commits from the begining that would impact the functionalities of Export, Toponaming and Defining External geometry, so I have basically cherry-picked those on top of the geometry extensions development, for the interested this branch.

In that branch every commit compiles (but it is not functional, some parts do work).

Each cherry-pick involved adapting the extensions part and substantially looking for changes in LinkStage3 development, as several decisions affecting src/App evolved over time and were later reverted, I commented out the code involved in these cases.

Summary of operations:
→ Cherry-picked: ca9ae3c897ffa5260009fe40b88318a087a0c8a0 (Sketcher Subobject support)
→ Cherry-picked: 5fc101b3290555faeec8adb5081e92e38cc9c387 (Sketcher Export)

  • Several functions that were afterwards removed commented out compiles but it is not functional.
    → Cherry-picked: 2ddd4b44b900ff718b856ac54d9f99f31e091087 (geohistory)
  • Commented out code for the same reason above.
    → Cherry-picked: c7f9858f9e9ed3109f8ce6b976401c028be3551f ( Sketcher: migrate sketch to new element map framework)
  • Commented out code for the same reason above.
    → Cherry-picked: b7b9031fda1a383671c69ae1adbedb2dddc11711 ( Sketcher: fix SketchObject getSubObject)
  • Commented out code for the same reason above.
    → Cherry-picked: efbea6fe92847c1a86a9341b02008f7ebe4fffa6 ( Sketcher: change element mapping op code)
  • Commented out code for the same reason above, and specially because of non-existing functions in Part::TopoShape()
  • Missing TopoShapeOpCode.h
    → Cherry-picked: 752d0dee28e67623529cbd5472cdf010156f7948 ( Sketcher: fix vertex export)
  • Commented out code for the same reason above
  • Missing TopoShapeOpCode.h
  • Missing Part::TopoShape().makEWires(shapes,TOPOP_SKETCH)

Aside from the fact that replaying commits on top of master may not be the best strategy due to all changes involved, the lack of convergence of TopoShape between LinkStage3 and master might be a major obstacle at the present. To be honest I do not have an idea of how far away from master TopoShape currently is. I have looked into changes in Part using git as above, and I got again a long list of commits.

Way forward

This massive integration requires not only wide-knowledge of FreeCAD code base, but also knowledge about LinkStage3 and its design decisions. Looking into all the TopoShape code for differences is going to take me a considerable amount of time, as I am not familiar with this part of FreeCAD.

5

Just want to say, thank you for all your work. Its apparent in this thread that you took painstaking time to log and report your activities and justifications. Thank you for applying your precious time to help improve FreeCAD. :smiley:

6

+1

Incredible write up and detail. Really interesting to read.

You’re doing really valuble work here and I am excited to see where that leads.

Thanks,

Dan

7

Hi, abdullah, nice write up. I only got to know your post today because of other people’s referring. Next time, please do not hesitate to ping me. I’ve known your introduction of the geometry extension when I am doing merge on my side. But you seem to have stopped developing for some reason, so I didn’t follow up. I am fine with your ideas about the extension. It’s just that for backward compatibility reason, some of the extensions must be included in Part, so that the old valina Geometry can know how to port the those fields (id, flags, ref, etc) saved in legacy file into new extension. In other words, it would be better to put SketchGeometryExtension and ExtenalGeometryExtension in Part WB. Maybe drop the ‘Sketch’ prefix. We can name the common ancestor of all geometry extension as GeometryExtensionBase.

I am not sure I understand correctly about your Facade idea, though. Is this a helper class that can query and install extensions on demand? More specifically, I am kinda confused when you say that the Facade must inherit from extension(s) that it supports. Is that really necessary? I thought the original purpose of introducing ‘extension’ is to work around problems with multi-inheritance. As a helper class, the Facade does not really need to inherit from any thing IMO. It can have its own class hierarchy, yes, but I don’t think it needs to mangle with the geometry extension class hierarchy.

I don’t think it is possible to do cherry pick now for the most parts. Because the underlying toponaming framework is not there yet. I’ll start merging that part when 0.20 cycle starts. The constraint property list part should be doable. I’ll try make a PR about this in next few days.

8

I was planning to ping you this weekend. The reason is that I wanted to have a better insight into your modifications to SketchObject. My time availability is very limited, so it takes a lot of time to me to do tests. Because the high amount of cross-dependencies between the different sketchobject enhancements, my initial idea of divide and conquer (understanding each enhancement one by one) was not successful. I now realise that it is rather to understand it as a whole or fail in any attempt. I did not want to waste your time before doing my homework. I tend to document my homework. I am sorry if this caused some confusion.

The main idea behind geometry extensions was to decouple Part WB from workbenches that use Part::Geometry but need to extend it. I was not comfortable with the idea of filling Part::Geometry with functionality that are workbench specific. This applies to for example layer information in future sketcher enhancements. My original understanding of the data members that you added to Part::Geometry to support the enhancements of SketchObject subject of this thread is that they were sketcher specific (and even some applying only to sketcher external geometry). For this reason, I was never comfortable with adding them to Part::Geometry. If I misunderstood this (i.e., if today Ref, Flags, Id, … serve a general purpose for a geometry for any workbench and they are not sketcher WB specific), then, of course they should be put in Part::Geometry. If they are Sketcher WB specific, then my opinion is that they should be in the Sketcher WB. Geometry Extensions is a way of achieving this.

A secondary goal was to do allow extension while keeping a common Part::Geometry class common to all WBs, so that the fact that extensions exist is transparent for common operations using Part::Geometry. This could also mean WB interoperability, so that one WB can exchange Part::Geometry with another one, where the Part::Geometry ends up having information from several WB and each WB treats its own information while not being affected by the presence of information specific to other WBs.

A couple of years ago I had much more time available and I really wanted to offer a framework to integrate your Sketcher enhancements without coupling Part and Sketcher WBs. Then my time availability changed rather dramatically. It is not that I gave up on the idea (otherwise I would have removed the dead code, SketchGeometryExtension and ExternalGeometryExtension). These were intended mainly at that time for your sketcher enhancement. I did not arrive in time with this solution, so you never got to consider it. It is fair.

Today those fields are only present in LinkStage3 branch. So my guess is that you are referring to backward compatibility within LinkStage3. Please, correct me if I am wrong.

All depends on the generality of that information. If it is Sketcher specific information, moving the extensions to Part::Geometry is purpose-defeating as the aim of the extensions is to decouple workbenches. In fact, if this information were generic, I would not have any problem with having it where it is in your branch (as data members of Part::Geometry). Because I tend to think it is Sketcher specific (and even within the sketcher different if it is external geometry or normal geometry), this is why I think it should be in the Sketcher module.

Now, I do understand the need for backward compatibility within LinkStage3 and I was wondering if a custom solution to address this issue in LinkStage3 would be possible (some kind of migration tool).

If you do not favour a custom migration tool, in a very simplistic form, LinkStage3 could keep the data members of Geometry (for a while), so it would be possible to read old LinkStage3 files and LinkStage3 SketchObject might be provided with a mechanism to port this information into the corresponding extension. The saving mechanism could then save just the extension version, thereby enabling progressive migration of files. I have come to appreciate how resourceful you are, so I am sure that it will be possible to find a solution to this problem, should the problem exist (if sketcher specific information). Of course, if you want my input or help, within my limitations, you will have it. :wink:

The idea of Facades of geometries is indeed that of a helper class that, given a Part:Geometry *, helps manage and query some specific extensions (not necessarily all). This is, Sketcher::GeometryFacade will handle those extensions necessary for normal sketcher geometry, it will ignore any other extensions (today your GeoId, in the future for example a LayerId). Sketcher::ExternalGeometryFacade will handle those extensions relevant for an external sketcher geometry, it will ignore any other extension (thereby providing an interface to GeoId, LayerId, Ref, Flags,… and in the future also LayerId). So a geometry facade, allows to get a suitable interface for a class of geometry for a specific workbench. I can imagine that somebody using default extensions (boolean, strings, …) in a python WB or a macro can create its own python facade to manage these default extensions in the sense intended by his/her python WB or macro.

Well, I realise that part of my presentation above may be misleading when referring to inheritance, specially if wrote that “the Facade must inherit from extensions that it supports”, that is wrong. There is private inheritance involved but it is not between a facade and an extension. Let me correct any previous inaccurate assertions.

As you say and it is my understanding too, a facade can have an interface that is not defined by any class hierarchy. More importantly, a facade IS not a extension (test for public inheritance). A facade is deliberately not implemented as a extension (has-a test for private inheritance). This is only logical, as the geometry already includes the extension by composition (geometry has an extension) and inheritance (of any type) would only lead to duplicating the functionality.

Because extensions are workbench specific, it is reasonable to think that a facade intended for a workbench will implement the whole interface of the extension (and here I am not talking inheritance, just having the same “functions”). So, while it is indeed not a requirement of a facade pattern to offer the same interface of any of its subsystems (I am following the definitions in “Design Patterns” by the gang of four for this), it is reasonable and convenient to provide a same interface in the present case as follows.

In the case of classes of geometries, especially when different classes of geometries may have common extensions, it is convenient that the geometry facades of these different classes grow in functionality at the same rate.

To keep all this grow under control, the solution I came to is to implement GeometryFacade and SketchGeometryExtension in terms of an interface (private inheritance here of class ISketchGeometryExtension). This is, it is not that GeometryFacade inherits from SketcherGeometryExtension, rather a common interface of the extension is defined in a separate abstract class, then Extension and Facade are implemented in terms of that common interface. Code may speak louder than words:

class ISketchGeometryExtension
{

public:
    // Identification information
    virtual long getId() const = 0;
    virtual void setId(long id) = 0;
};

class SketcherExport SketchGeometryExtension : public Part::GeometryExtension, ISketchGeometryExtension
{...}

class SketcherExport GeometryFacade : public Base::BaseClass, ISketchGeometryExtension

Here in both the interface ISketchGeometryExtension is privately inherited (I think I will add private in the code to mark the intent).

Here, SketcherGeometryExtension IS NOT a ISketchGeometryExtension. GeometryFacade IS NOT a ISketchGeometryExtension. GeometryFacade IS NOT a SketchGeometryExtension (no public inheritance, no chance of casting any of this classes to the “common interface” class).

Private inheritance forces the developer of GeometryFacade and SketcherGeometryExtension to implement the interface of ISketchGeometryExtension. I clarify that parts of this interface may be public and others private should such need arise (note that the override can be made public or private as needed, for example in the rare case that a facade should only partially implement an extension interface). There is currently no such need. This forcing the implementation of an interface forces the developer to keep Facade and Extension development synchronised, without coupling them.

The ExternalGeometryFacade counterpart should be as follows:

class ISketchExternalGeometryExtension
{
public:
    // Identification information
    // START_CREDIT_BLOCK: Credit under LGPL for this block to Zheng, Lei (realthunder) <realthunder.dev@gmail.com>
    virtual bool testFlag(int flag) const = 0;
    virtual void setFlag(int flag, bool v=true) = 0;
    // END_CREDIT_BLOCK: Credit under LGPL for this block to Zheng, Lei (realthunder) <realthunder.dev@gmail.com>

    virtual bool isClear() const = 0;
    virtual size_t flagSize() const = 0;

    virtual const std::string& getRef() const = 0;
    virtual void setRef(const std::string & ref) = 0;
};

class SketcherExport ExternalGeometryExtension : public Part::GeometryExtension, private ISketchExternalGeometryExtension
{...}

class SketcherExport ExternalGeometryFacade : public Base::BaseClass, private ISketchGeometryExtension, private ISketchExternalGeometryExtension
{...}

  • Note in master ExternalGeometryFacade is inheriting public on both interfaces. This is a copy/paste mistake and was not intended. I have modified it in the code above for the sake of the argument.

  • In master ExternalGeometryExtension is defined differently than from above. Here, I iterated several solutions and somehow what I merged does not correspond to what I initially intended. It is code that works, but is not what it should be. Your comment prompted me to review this and I thank you for that.

If you think that this approach is somehow wrong, let me know. I am not a proud great professor with a good reputation to maintain. I am just one hobbyist trying to learn and making mistakes. When I make mistakes and I am told, I admit them and try to correct them, learn from them and move forward. No hard feelings. Any gain is a gain for us all.

Indeed. This is something I realised too. I think that the most sensible approach is to first merge all the dependencies (e.g. toponaming and propertylist parts), so that the interfaces used are as closest as possible between LinkStage3 and master. Even them, cherry-picking would require heavy changes because of the development path. This week I have been experimenting with git checkout --patch realthunder/LinkStage3. This enables dynamic file by file hunk by hunk selection. In my tests the worst part was coming from the toponaming part and the different approach towards setValues moves in properties. I think it could work much better if we bring the code as closest together as possible. I do not mean this is a panacea, but cherry-picking was very painful in my experience.

BTW, do we have an estimated date for 0.19 release 0.20 cycle start?

I think that the first think we need to agree is whether Ref, Id and flags are sketcher specific or general for any geometry and where the extensions should go. I wait for your input.

In parallel, ping me if you PR some of the dependencies. I will stop what I am doing to help review it. That is a commitment.

Thanks for your proactive approach.

Regards,
abdullah

9

Don’t know either. I am waiting too.


I think that the first think we need to agree is whether Ref, Id and flags are sketcher specific or general for any geometry and where the extensions should go. I wait for your input.

Yes, by ‘backward compatibility’, I mean the files saved by my branch. Extension is a good way to decouple data needed by different logic, i.e. code in various workbenches. But then, since it is just data in those files, I don’t think it is unreasonable to have some basic data only extensions provided by Part module itself. The logic of operating on those data still resides in the corresponding workbenches. For future more specific functionalities, each workbench can implement their own extensions.


In parallel, ping me if you PR some of the dependencies. I will stop what I am doing to help review it. That is a commitment.

Sure. Let me first finish my work at hand. I am doing some major changes in topo naming right now. The basic framework stays the same, but lots of tweaks here and there to improve efficiency, as I get a few reports recently about memory hogging.

10

Looks like the end of this 2020: https://forum.freecadweb.org/viewtopic.php?f=25&t=50313&p=446415#p446415

11

When I click your link I get: “You are not authorised to read this forum.”

It is apparently over my paycheck :laughing:

12

I am not convinced that such extensions would be generic. Part already has some very generic extensions (Default extensions). These are more convoluted constructions. If ever used by other workbench, it would rarely be because it is exactly what that other workbench needs, but rather because it is available.

Maybe it makes sense to evaluate a mechanism to migrate to extensions living in the Sketcher first, before discussing further on where to put the extensions.

What about this mechanism, that would be used in SketchObject like this in order to restore LinkStage3 legacy files into Geometry extensions?

With this, I have managed to read the GeoIds of a file saved with LinkStage3.

In principle FC master does not require this mechanism in PropertyContainer, but your branch could leverage it to import your legacy files directly into geometry extensions.

I have not implemented the same for your property ExternalGeo, because FC master does not have this property, but the same mechanism could be applied.

This should enable your branch and master to have a same Part::PropertyGeometryList and Part::Geometry without the extra information, and have the sketcher extensions in the Sketcher module. :smiley:

Does this mechanism address all your concerns and enables you to consider having the extensions in the Sketcher module, or do you see further drawbacks that should be addressed?

13

Well, it can certainly work, but I don’t quite like it. Because, the fact that any object has to use this way to override property restoring indicates that the property does not belong to the same module of the object, which in turn means it would create an unnecessary source code level dependency between the two modules. Any changes to the property saving code must be manually synchronized in the object’s module. The core already has mechanism of migration for missing property and property type change. If the object really wants a customized property, the recommended way is to create a new type of property in its own module, maybe derived from the original one, and migrate the property of existing object to the new type.

The reason to include the extension in Part module is simply because PropertyGeometryList lives in Part, and it is Part::Geometry that gains the new ability to install/save/restore extensions, so it should be PropertyGeometryList’s responsibility to handle migration. Strictly speaking, it should be Part::Geometry’s responsibility, but those extra attributes are currently saved/restored by PropertyGeometryList. Since they live in the same module, it is not such a big problem. What I am proposing, is for PropertyGeometryList to check for those attributes. If not empty, install some extension(s) to hold those attributes. And that’s it. Other workbenches that are not using those attributes won’t care, and are certainly not obligated to derive any future extension from these.

14

I only partly agree:

  • Certainly Part::Geometry and Part::PropertyGeometryList do not belong to the same module as Sketcher::SketchObject. It is common to have properties within a container coming from other modules. This should not be a problem of its own.

  • Strictly speaking there is no source code level dependency between the two modules. There is rather a “dependency” between a module (Sketcher) and a legacy implementation (Sketcher related data members in Part::Geometry inserted by Part::PropertyGeometryList). Unnecessary coupling between Sketcher and Part is what we have at this time in LinkStage3. I do not think such coupling is sound. The aim of the present change of structure is precisely to decouple these two modules. In the end result, the new Part::Geometry and the new Part::PropertyGeometryList are able to save and restore their own data members. These new classes are ready to be used in a third module (for example Path) without any need for the specialrestore mechanism. In fact, it is possible to have a non-backward compatible Sketcher with these new properties without any need for the container performing a restore, which is only done for backward compatibility. Of course, legacy backward compatibility is a must. No discussion there. Just, I think it demonstrates that there is no such dependency between modules.

  • The “migration” in the container (SketchObject) is the price to be paid for having backward compatibility in cases where architecture mistakes were committed. For transparency, I do not only think that those mistakes are the Sketcher data members in Part::Geometry saved and restored by Part::PropertyGeometryList in LinkStage3. For example, I also made a mistake when I decided to reserve construction points for special geometry which the solver ignores during solving (mostly BSpline knots), which is a Sketcher-specific feature. Probably making construction, which only makes sense for the Sketcher, as a data member of Part::Geometry was also a mistake. One that we may even discuss if it makes sense to undo. Bottom line is that mistakes happen. We should have the tools to handle them and in order to handle them we might have to pay a toll (in connection with the next argument). Of course, we should be willing to pay as smallest a toll as possible (or no toll at all if we can really get away with it). I do not currently see a way out without paying any toll.

Indeed. This is the toll we pay in the implementation I showed, in order to have new Part::Geometry and Part::PropertyGeometryList that are generic and decoupled from Sketcher interference. I think we should tacklet this issue, one way or another, because delaying the decoupling may only exacerbate the problem and increase the toll to be paid.

In any case, I note that Part::Geometry restoring mechanism is reused in full. There may only be a need for synchronisation if saving/restoring when Part::PropertyGeometryList is modified. This should be very unlikely to happen (not to say it should never happen, which is what I think). In fact, to delegate the saving of Geometry data members to PropertyGeometryList is IMO not a sound decision either.

Which are just other types of “mistakes” where we already pay a toll. The toll is certainly lower in the case of a mere name change. In the case of a type change, the toll depends on the ability for the new type to handle the old type. If it would require a dependency of a different module, then the toll would be equivalent to the one we are facing here (for example, if we had the same case as we have now and instead of applying the new mechanism, we would decide to rename the property “Geometry” as “Geometry1”, this would have many other problems, but would also have those we are facing now in addition).

In any case, I note that we will only have to handle the restore in the container insofar as we require backward compatibility. We might re-evaluate this in the future if the synchronisation becomes cumbersome to maintain (which in any case I do not expect), under the scope that support for migration from versions 10 years old might not be a strong requirement. In any case, I do not think this synchronisation would likely become a problem.

That is indeed an alternative. Here I think it depends if you want to keep the same type for exchange between modules or not. It might also impact current python modules and WB.

In any case, this route would reasonably require to stop relying on Part::Geometry on the Sketcher module, and create a Sketcher::Geometry. But Part::Geometry is not even a property. Then it would be necessary to create Sketcher:PropertyGeometryList as a list of Sketcher::Geometry objects. That would be the property to substitute. For backwards compatibility you would still need to have Sketcher::PropertyGeometryList handle the import from Part::PropertyGeometryList. And either lose direct interoperability at Part::PropertyGeometryList level or provide adaptors, which would require additional code and may introduce additional dependencies. Even at Geometry level, because Geometry uses a factory method to create geometry, even with inheritance, there would be a substantial reimplementation effort, which would likely result in an explosion of new classes… Sketcher::GeometryPoint, Sketcher::GeometryPointPy, … (mileage varies with implementation, as it is the case for advantages and drawbacks).

A solution with Geometry Extensions and Geometry Facade has a substantially lesser impact, even in a case where only the Sketcher would replicate such a hierarchy.

If Part::Geometry would be used by a third module and this module would require its own data members, then replicating all that class hierarchy potentially including Python twins would be necessary for the third module. This looks like a solution showing no scalability at all. A scalability that is readily achievable by using the Geometry Extensions solution.

In view of the above, I do not think the proposal of a new property is a realistic sound alternative to the specific problem we are facing at the present.

The solution I showed is just one solution that satisfies the goal of decoupling modules and stopping the use of Part WB for Sketcher WB specific data members. It additionally serves the purpose of fixing the mistaken use of construction points for other goals. It could also serve a migration of the construction status to the Sketcher WB (this is a separate topic because there is a need to evaluate the impact to Python macros and WBs).

I fear it is not possible to have backward compatibility without relying on the container or giving away decoupling between WBs. I feel strong about giving away decoupling as I hold this decoupling as a design maxima (but I admit that I am not a professional coder, let alone a software architect, just an ok pretender :wink: ).

Feel free to propose a counter-proposal. I am happy to look into it.

15

When I said ‘source code level’ dependency, I am referring to this solution you proposed, where you effectively try to restore the SketchObject.Geometry property inside Sketcher module. What if PropertyGeometryList changes its storing format, which means that it will have its own way to handle some other migration? The sketch migration code will have to duplicate the changes. I actually did made many changes to property saving/restoring mechanism in my branch, when I made it possible for FC to to save/restore directly to/from uncompressed directory. For example, the OCC shape property can now save its ascii or binary data directly in xml as character stream, other properties too, such as PropertyColorList, etc. I didn’t touch PropertyGeometryList, though, because of sketch usage, which may cause some potential complication.


[quote=“, post:13, topic:45512”]
The core already has mechanism of migration for missing property and property type change.
[/quote]

Which are just other types of “mistakes” where we already pay a toll. The toll is certainly lower in the case of a mere name change. In the case of a type change, the toll depends on the ability for the new type to handle the old type. If it would require a dependency of a different module, then the toll would be equivalent to the one we are facing here (for example, if we had the same case as we have now and instead of applying the new mechanism, we would decide to rename the property “Geometry” as “Geometry1”, this would have many other problems, but would also have those we are facing now in addition).

I don’t really think it that way. Many of the changes required are not ‘mistakes’. They are often just normal migration anticipated when new features develops. And what the core provides now is enough to handle them.


[quote=“, post:13, topic:45512”]
If the object really wants a customized property, the recommended way is to create a new type of property in its own module, maybe derived from the original one, and migrate the property of existing object to the new type.
[/quote]

That is indeed an alternative. Here I think it depends if you want to keep the same type for exchange between modules or not. It might also impact current python modules and WB.

I think you misunderstood me here. I didn’t mean to have a new property in sketch to handle this. Your extension idea is good. And it does the decoupling well. What I propose is to create some data only extension in Part just for migration purpose. If you really want a clean separation from now on, you can, for example, uninstall those extensions from Part after restore, and replace them with some sketch specific extension. In other words, those Part extensions can be transient, and only serve the purpose to hand over information restored from legacy files.

16

Let’s clarify the terminology. The idea I proposed there is not a dependency and it is consistent with the fact that the container is responsible for important changes in properties (such as renames or changes of type). Now, this does not mean that we have to use that idea, specially if it is possible to have a less intrusive way of handling the situation (see below).

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.

Which are just other types of “mistakes” where we already pay a toll. The toll is certainly lower in the case of a mere name change. In the case of a type change, the toll depends on the ability for the new type to handle the old type. If it would require a dependency of a different module, then the toll would be equivalent to the one we are facing here (for example, if we had the same case as we have now and instead of applying the new mechanism, we would decide to rename the property “Geometry” as “Geometry1”, this would have many other problems, but would also have those we are facing now in addition).

I don’t really think it that way. Many of the changes required are not ‘mistakes’. They are often just normal migration anticipated when new features develops. And what the core provides now is enough to handle them.
[/quote]

I can only agree to disagree.

Development of features needs to have the impact on the architecture and specially the coupling in mind. Reliance on Part::Geometry to store sketcher data members to enable extended Sketcher functionality is a good example of it. I have kept several ideas to develop the Sketcher further aside to avoid this coupling. Then I tried to find a way to avoid it (Geometry Extensions). Eventually I will implement them.

In fact, the core does not provide to decouple this inter-module coupling. That is why we are discussing here how to do it best, which requires a new solution to maintain the decoupling.

That is indeed an alternative. Here I think it depends if you want to keep the same type for exchange between modules or not. It might also impact current python modules and WB.

I think you misunderstood me here. I didn’t mean to have a new property in sketch to handle this. Your extension idea is good. And it does the decoupling well. What I propose is to create some data only extension in Part just for migration purpose. If you really want a clean separation from now on, you can, for example, uninstall those extensions from Part after restore, and replace them with some sketch specific extension. In other words, those Part extensions can be transient, and only serve the purpose to hand over information restored from legacy files.
[/quote]

Actually, I think it makes sense the idea of having an intermediate extension for migration purposes. I do not appreciate that it is just another type to be registered in the type system for occasional use, but overall it may lead to a lower-impact solution on the project, which I appreciate.

Thanks for the input. Now I am reviewing code. I will eventually post back here.

17

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.

18

Hi!

I have dedicated part of the last month to Sketcher coding. Most of the code is directed to fix legacy sketcher decisions (most taken by myself) so as to enable unconstrained future development. Even though there are not many user appreciable changes, I thought it made sense to make a short post to show them.

One of the abilities gained is that now each geometry carries information about the Internal Alignment Type they are (or none, if they are not), and can carry more information, including geometry specific visualisation information. This enables for the user some longed for cosmetic value and functional value:

  1. First, the less important of all, Internal Alignment geometry is now drawn in a different color indicating this condition. In the future it may be shown with different draw style (different thickness lines, or dashed lines,… this will need discussion to see what the community wants).

  2. More function-wise is the specific case of B-Spline poles. There was the historical user complaint that poles should not have a size defined by a length unit. This is not just about hiding the unit (which can be done in master), but as to have the magnitude of the constraint applied to a pole meaningful (weight value irrespective of a “physical size” necessary for visualisation). Moreover, it is nice that we can depict and control a pole using a circle, but a pole is not a circle and many constraints make no sense on a pole. To start with a radius constraint. This version still reuses the command toolbar radius constraint (albeit with updated tip) and the art icons (until somebody wants to provide new art for the weights), but it is run using a new sketcher constraint of type “Weight”. Unlike “Radius”, “Weight” is dimensionless. In case you were wondering, the physical size of the circles is handled directly by the 3D view (ViewProvider), which represents them proportional to the weight value, where the proportion factor depends on the size of the B-Spline. Therefore, if you have a B-Spline measuring kilometers, your poles will be proportional, if your B-Spline measures millimeters, then your poles will be proportional too. Let’s take a peek:
    InternalAlignmentPeek.gif
    Even though the increase in functionality is very limited, the internal changes are substantial. In fact, it handles the migration from a format that reused the construction status of points to represent B-Splines knots and which prevented enabling defining points in the sketcher (points that define the Shape of the sketch). Yes, it paves the way for defining construction points. In its current form it also migrates the construction status of geometry from Part WB to Sketcher WB (removing any coupling of Part on Sketcher). But this is something I personally was longing for, but it has implications that I have to discuss with fellow developers. The lack of substantial functionality increase in fact is somehow on purpose, as I wanted to keep this PR as small as possible (and it is not that small anymore). Just comprising those features that are affected by the construction status in the Sketcher, because I take leverage of the change of format for a seamless migration.

Because the changes are substantial and I would ask to those who can build FreeCAD from source, to test this PR if possible:
https://github.com/FreeCAD/FreeCAD/pull/4112

I am very very very interested in crashes. This PR is the first PR that makes heavy use of Geometry Extensions, which are not well tested.

There is one caveat: A file saved with this PR will not (properly) load in FreeCAD master (it is possible to provide forward compatibility (e.g. for v0.18), but it needs some commits in the version requiring forward compatibility (e.g. 0.18)). Do not use it for anything important.

19

This post is to discuss a design decision in PR:
https://github.com/FreeCAD/FreeCAD/pull/4112

Let me know your impressions.

Introduction

I have said several times that I do not appreciate having coupling between Part and Sketcher WB.

One of the main limitations to Sketcher development in the last years has been depending on the data members of Part::Geometry to store Sketcher information. Because Sketcher specific information should not be stored by Part::Geometry (Part should not know anything specific about the Sketcher), the Sketcher development has made use of (sometimes rather) ingenuous but often constrained solutions to achieve its goals. This has sometimes led to over-stretched solutions that “did the trick” but are not future proof. To the top of my head it comes reusing the construction status of points to code B-Spline knots, which now interferes with having points that define the shape of the Sketch outside edit mode (that was my wrong if anybody is wondering).

The origin of this comes from “Construction state” being stored by Part WB. In fact, I do not think Construction state even belongs there and only Sketcher uses it. But this also means that Sketcher is relying on code outside its control for some rather important functionality.

The solution to enable decoupling of storage between workbenches using Part::Geometry and at the same time enabling the Sketcher to have unlimited new per geometry storage is Geometry Extensions. At the moment of starting to use Geometry Extensions to store Geometry information there is a window of opportunity to remove all coupling, including the “Construction State”.

The PR has many more decisions. All have in common that they depend on the introduction of GeometryExtensions. So I have grouped them while trying to keep the PR as small as possible. This is the reason why there are “unrelated features” in a “not so small PR”.

The removal of construction from Part::Goemetry has an impact on forward compatibility (backwards compatibility is guaranteed). However, this does not only originate from moving the construction state, as for example, the coding of b-spline knots would be affected even if the “construction state” would stay in Geometry. Still, it is possible to add forward compatibility is so desired to older versions, if we take advantage of the change of format.

Below I discuss just the removal of the storage of the “construction state” from Part::Geometry, because I believe the other design decisions to be ok.

The decision under discussion

  1. Remove “bool Construction” data member from Part::Geometry:
class PartExport Geometry: public Base::Persistence
{
    ...

- protected:
-    bool Construction;
};
  1. Bring the construction functionality to the Sketcher module, to SketchGeometryExtension.

So, the decision is: moving the storage of this data member from Part module to Sketcher module.

Additionally, removing Python access to the Construction attribute that is no longer in Part WB. But I note this is currently broken in master (as I will explain below).

Impact

  1. Storage: while backward compatibility is guaranteed, forward compatilibity is not (unless specific code is provided, which is doable).

Currently, storage of geometry looks like this:

<Geometry  type="Part::GeomArcOfCircle">
    <Construction value="0"/>
    <ArcOfCircle CenterX="-0.6972459419812660"  ... />
</Geometry>

After the change it would look something like this (geometrymode is a std::bitset that will exist nevertheless and contains or is intended to contain other geometry mode flags in addition to construction status, such as blocked geometry state):

<Geometry  type="Part::GeomArcOfCircle">
    <GeoExtensions count="1">
        <GeoExtension type="Sketcher::SketchGeometryExtension" id="15" geometrymode="0"/>
    </GeoExtensions>
    <ArcOfCircle CenterX="-0.6972459419812660"  ... />
</Geometry>

For backward compatibility, the presence of “GeoExtensions” can be leveraged, so when reading element in Geometry::restore, the element is a “GeoExtension” (which will be mandatory, it is currently not), then it is the new format and no Construction will be read (because it does not exist and would only mess the restore process if read is attempted). If the element is “Construction”, then it
is legacy format, and this will trigger special code to create a Part::GeometryMigrationExtension (a new GeometryExtension in Part WB used exclusively for migration purposes) to convey this information to the Sketcher, which will set the new format.

For forward compatibility (if it is to be offered), without further code, old FreeCAD would read a new file format by reading and skipping parts of the file because it will not find the construction element. New code would need to be provided leveraging extensions presence again. If geometry does not read construction, then the Sketcher would set the Part::Geometry construction with the right value from the SketchGeometryExtension. There are other changes which will affect forward compatibility (e.g. new way of marking bspline knots without using “construction points”), so I am not sure how much of a problem it is not to provide forward compatibility. It is in any case a solvable problem if we want to (for this change and also for BSplineControlPoints).

  1. Any Python macro that may directly query Construction on a Part::Geometry construction (if such macro exists, I know none), would need to be adapted:
>>> geos = ActiveSketch.Geometry
>>> geo = geos[0]
>>> geo.Construction
True

If construction status is to be queried, this could be used (this works also today in master because of the facade):

>>> gfs = ActiveSketch.GeometryFacadeList
>>> gf = gfs[0]
>>> gf.Construction
True

Of course, this does not affect at all toggling or setting construction via SketchObject, for example:

ActiveSketch.toggleConstruction(0)
ActiveSketch.setConstruction(0,True)

worked and will work transparently.

Rationale for the changes “now” (together with the introduction of actual use of GeometryExtensions)

  1. Coupling between Part WB and Sketcher WB

1a) Construction data member of Part::Geometry is only used by the Sketcher.
1b) Construction status setting is currently broken because Sketcher used it for B-Splines (you cannot use Part.Construction to set construction status, it won’t work, check the result afterwards).
1c) It should probably not be possible to set the Geometry status outside the Sketcher, as it affects the Sketcher and should be under its control.
1d) It invites a developer of a new WB to use this for any other purpose instead of creating a geometryextension, which may lead to interoperability issues and unexpected results if geometries are assigned between WBs (currently not a problem, because there is no such WB).

  1. There will never be another tonight ™:

2.1 The way Geometries are currently saved is not flexible/future proof

While new data members may be added to Part::Geometry and save them, the way this saving mechanism is implemented is rather inflexible, because Part::Geometry does not know how many xml elements it needs to read (there is not any counter indicating how many should be read as anywhere else, they will not be homogeneous elements either). Additions and removals become cumbersome to code, certainly removals and it is not a flexible solution. The alternatives are to store Geometry data members via PropertyGeometryList using attributes instead of new elements (LinkStage3 does this), but this creates two additional problems. First, it is not necessary from a conceptual point of view that a Geometry forms part of a PropertyGeometryList. Second, it is not sound that PropertyGeometryList should know about the internals of a Geometry and do the restore for them.

2.2 Backward and forward compatibility (the latter with code), are only possible because GeometryExtensions are not currently in use, so it can be leveraged in Part::Geometry::restore.

Although GeometryExtensions were added almost one year ago, the Sketcher is not yet using them (the PR above is the first to leverage them). Because of the limitation of the geometry saving mechanism explained above, the introduction of extensions is key to enable moving this construction information to the Sketcher. Any other moment will need dirty hacks or considerable unnecessary changes to the restore mechanism (or I do not know better).

Alternative: There are levels of survival we are prepared to accept ™

The obvious alternative is: not to move the construction state from Part::Geometry.

We would have to assume the coupling, assume that most likely it will be there forever and be careful with future development and warn future developers.

The sky will not turn red because of this. But it could be avoided with a rather limited impact today.

20

I have a couple of questions:

  1. Is it your plan to try to have this ready for the 0.19 release, or to wait until after that?
  2. Besides testing B-splines, are there other particular areas of Sketcher we should focus our testing on?