[Core] Enable exposing VarSets in App::Part #12532

pieterhijma · 2024-02-21T11:41:10Z

pieterhijma · 2024-02-21T11:55:47Z

Dear all, In issue #12120 I've added the video Custom Data Elements, Variants, Modularity - Part 1 to discuss the problem with modularity. Part 2 of the video is here:

Custom Data Elements, Variants, Modularity - Part 2

This video was created with the code that is currently in this PR.

The initial implementation automatically redirected VarSet references. To make the implementation less intrusive and similar to other approaches such as Spreadsheet configuration tables, we switched to an implementation that makes use of hiddenrefs.

Before this commit, users had to make references to the parent part hidden manually. This commit extends expression visitors and expression modifiers with an expression rewriter. An expression visitor visits the expression tree to gather information. An expression modifier can modify individual nodes within the tree. An expression rewriter can rewrite the tree itself, that is, add to and remove nodes from the tree.

pieterhijma · 2024-03-07T11:52:07Z

The problem sketched in #12531 can be solved in several ways. The cleanest solution would be that a part can remain absolutely unmodified as soon as the VarSet is exposed. The first commit attempts that by redirecting references to VarSets automatically. However, in that case it is challenging to maintain dependencies between VarSets that are replacing the geometry. To make that happen, we would need to maintain a mapping between replacing VarSets and the Parts they are used in to create the variant geometry. This is especially challenging with subshapebinders that create copies of the object in a temporary document.

Another solution is to change the references inside a Part that refer to the exposed VarSet to point to the propery within the parent Part that references the replacing VarSet. However, this creates cyclic dependencies and this makes it challenging to maintain proper dependencies as well.

In the end, I chose for a solution that makes as much as possible use of existing logic. This has several benefits:

Problems that occur with this logic can be solved in general improving traditional configuration tables, for example.
This is already an approved development path in FreeCAD.
It allows the above implementation to be fairly separated from the rest of the core functionality; it is not necessary to have deep changes in the core logic.
The current solution can make use of the dependency checking logic already available for hidden references.

yorikvanhavre · 2024-03-12T09:39:38Z

My opinion here is more or less the same as last time, that there is a lot of core change, with new functionality added to the expression engine, all this for a very specific use case (basically the "exposed / not exposed" system). It seems to me the general tendency should be to avoid that, and, if a specific use case needs to be implemented, to confine as much as possible and do with what FreeCAD has instead of trying to adapt large parts of FreeCAD, specially core stuff, to such case.

I feel this adds a lot of complexity for little use outside that specific use case. I'd prefer to have that use case solved without so much core change. IMHO all that happens here could and should somehow be contained inside the VarSet object itself

But I might be wrong on this, I would like others to have a look (@chennes or @wwmayer maybe?)

NomAnor · 2024-03-12T13:48:14Z

I just tried your PR and created a file as shown in your "Part 2" video. Maybe I missed something, but when I change the VarSet outside of Part, no automatic recompute happens. The outside VarSet is set in the Part property created from the exposed VarSet inside of Part. This is obvious because the expression now contains a hiddenref(<<Part>>.Params.Test) call instead of <<Params>>.Test and this breaks the dependency cycle.
I understand that this stems from the limitations of the current code. But from a user perspective this is not a usable solution, nobody wants to manually recompute all features that reference the parameters.

I'm not familiar with the depency code in FreeCAD, but my understanding is, that the core problem is, that the dependency resolution (for a recompute?) has a granularity of a DocumentObject.
When I create a Part with property X and then create a Body inside of the Part that references that property in a FeatureY, I create a cycle because the logic is as follow:

X is thouched -> Part is touched -> FeatureY must be recomputed and is then touched -> Body must be recomputed and is then touched -> Part must be recomputed and is then touched -> !! Feature Y must be recomputed.

Would it be feasable to change the dependency resolution to a granularity of a property, such that the logic becomes:

Part.X is touched -> FeatureY.Height is touched -> FeatureY.Shape must be recomputed and is then touched -> Body.Shape must be recomputed and is then touched -> Part.Shape must be recomputed and is then touched

As long as Part.X is not dependent on Part.Shape there is no cycle. This fits with the separation of computing the shape and the execute() function as mentioned in #12868. (Just a thought I had, even with a cycle, some kind of fixed-point iteration could also be useful in some constelations)

This is obviously a bigger change to the core code, but one that would allow a more precise dependency tracking across all workbenches. Implementing variant links might then become much more straight forward.

pieterhijma · 2024-03-13T12:03:31Z

My opinion here is more or less the same as last time, that there is a lot of core change, with new functionality added to the expression engine, all this for a very specific use case (basically the "exposed / not exposed" system). It seems to me the general tendency should be to avoid that, and, if a specific use case needs to be implemented, to confine as much as possible and do with what FreeCAD has instead of trying to adapt large parts of FreeCAD, specially core stuff, to such case.

I feel this adds a lot of complexity for little use outside that specific use case. I'd prefer to have that use case solved without so much core change. IMHO all that happens here could and should somehow be contained inside the VarSet object itself

But I might be wrong on this, I would like others to have a look (@chennes or @wwmayer maybe?)

I agree you would want to avoid many core changes and that you want to confine the functionality as much as possible. Indeed, it is a specific use-case, but a very powerful one and not one that I believe other CAD programs have. In that light, I believe the changes are warranted.

I think it may be useful to make clear what exactly is added to the core. We can make a distinction between things that are confined and things that are not confined.

Confined changes to the core:

VarSet.*
PropertyVarSet.*

Not confined changes to the core:

Part.*: overriding addObjects / removeObjects to take into account variable sets. In my opinion this is a small change.
Expression rewriter: I would have preferred to make a separate issue and PR for this functionality, but because of FreeCAD's design with regards to expressions, this is very hard to do. The expression functionality is in essence an AST, an abstract syntax tree and it is very logical to have visitors for this kind of logic that walk the expression tree and gather information. FreeCAD has visitors for precisely this reasons. There is also an extension to visitors that allows one to modify single nodes within the tree. A logical next (and final) stage for ASTs is to have a rewriter that allows you to manipulate the structure of the expression tree. This functionality is not available in FreeCAD's expression framework, but it is very useful to have this functionality. All the generic rewrite() and _rewrite() replaceExpression() in Expression and ExpressionVisitor are part of this functionality. I made it as similar and close as possible to the visit() and _visit() so it is easy to verify what the behavior is.
rewrite VarSet Expression: Given the generic rewriter functionality of above, this is one particular instance of the rewriter. Because of FreeCAD's design and to an extent C++ limitations (only single dispatch instead of multiple dispatch) one such visitor/modifier/rewriter touches much code, typically Expression.*, ExpressionParser.h, ObjectIdentifier.*, PropertyExpressionEngine.*, and ExpressionVisitors.h. For example, the visitor renameObjectIdentifierExpression (the first one in ExpressionVisitors.h) has presence in obviously ExpressionVisitors.h, in Expression.* (renameObjectIdentifier, _renameObjectIdentifier) ExpressionParser.h (_renameObjectIdentifier), and PropertyExpressionEngine.* (renameObjectIdentifiers). Same is true for the second one updateElementReference but this also has presence in ObjectIdentifier.*. So, in fact, none of the visitors are very confined, thus the fact that the VarSet rewriter touches much code is simply an artifact of this design. It is a pity that this is the case, because it makes it difficult to test visitors/modifiers/rewriters in isolation, because you would have to add a specific test to all those source files. This is why I didn't make a separate issue/PR for the rewriter. If confinement / modularity of source code is an issue, I would advise to investigate how to define visitors/modifiers/rewriters such that they are independent from Expression.*, ExpressionParser.h, and ObjectIdentifier. I think what would be needed is relax protected/private restrictions in these source files to allow visitors to manipulate the data structures. In addition, it is probably needed to manually dispatch on type of expression, but I'm not so sure this would be a large drawback compared to the current approach. By the way, if the generic rewrite functions from the point above are deemed as code duplication (you could argue that it is because it closely follows the visit functions), I think it would be possible to express the more simple visitors in terms of the rewrite functions.
findReferencedVarSet: This is in Expression.* and ObjectIdentifier.*. I may be able to factor this out, possibly by making App::VarSet a friend class but given the design above, this is in my humble opinion the most logical decision.
dependencies for VarSets: this adds a signal/condition to the already existing dependency mechanism for hidden references. Since this implementation uses hidden references (as created by the rewriter), this is a small modification to existing code to acquire the requested behavior.

So, from my point of view the following has happened here:

FreeCAD's expression visitors have become more powerful by allowing rewriting expression trees.
One visitor (a rewriter) has been added to the already existing visitors touching the same source files that all visitors are touching.
A very small change has been added to Part to make the combination Part/VarSet powerful.
I added logic to find variable sets in expressions (may be possible to factor this out)
Added VarSet dependency tracking to existing hiddenref dependency tracking.

So, I can see that the proposed changes feel large, but in fact, I think it is as small as can be to acquire the functionality. For convenience, I will comment on each diff to which category it belongs.

pieterhijma · 2024-03-13T12:04:38Z

src/App/Expression.cpp

@@ -282,6 +282,13 @@ void ExpressionVisitor::offsetCells(Expression &e, int rowOffset, int colOffset)
    e._offsetCells(rowOffset,colOffset,*this);
 }

+Expression* ExpressionVisitor::rewriteVarSetExpression(Expression &e, const DocumentObject* parent,


VarSet rewriter

pieterhijma · 2024-03-13T12:05:05Z

src/App/Expression.cpp

@@ -822,6 +829,12 @@ void Expression::Component::visit(ExpressionVisitor &v) {
    if(e3) e3->visit(v);
 }

+void Expression::Component::rewrite(ExpressionVisitor &v) {


Generic rewriter

pieterhijma · 2024-03-13T12:05:22Z

src/App/Expression.cpp

@@ -1106,6 +1119,19 @@ ExpressionPtr Expression::replaceObject(const DocumentObject *parent,
    return ExpressionPtr(expr);
 }

+VarSet* Expression::findReferencedVarSet() const {


find referenced VarSet

pieterhijma · 2024-03-13T12:05:42Z

src/App/Expression.cpp

@@ -1137,6 +1163,25 @@ void Expression::visit(ExpressionVisitor &v) {
    v.visit(*this);
 }

+Expression* Expression::rewrite(ExpressionVisitor &v) {


Generic rewriter

pieterhijma · 2024-03-13T12:05:54Z

src/App/Expression.cpp

@@ -1184,6 +1229,13 @@ Expression* Expression::copy() const {
    return expr;
 }

+void Expression::replaceExpression(Expression** expression, Expression* newExpression)


Generic rewriter

pieterhijma · 2024-03-13T12:06:37Z

src/App/Expression.cpp

@@ -1663,6 +1715,17 @@ void OperatorExpression::_visit(ExpressionVisitor &v)
        right->visit(v);
 }

+void OperatorExpression::_rewrite(ExpressionVisitor &v)


Generic rewriter

pieterhijma · 2024-03-13T12:07:01Z

src/App/Expression.cpp

@@ -2867,6 +2930,29 @@ void FunctionExpression::_visit(ExpressionVisitor &v)
    }
 }

+void FunctionExpression::_rewrite(ExpressionVisitor &v)


Generic rewriter

pieterhijma · 2024-03-13T12:07:29Z

src/App/Expression.cpp

+    }
+}
+
+Expression* FunctionExpression::_rewriteVarSetExpression(const DocumentObject* parent, const char* nameProperty,


VarSet rewriter

pieterhijma · 2024-03-13T12:07:41Z

src/App/Expression.cpp

@@ -3062,6 +3148,64 @@ bool VariableExpression::_renameObjectIdentifier(
    return false;
 }

+Expression* VariableExpression::_rewriteVarSetExpression(const DocumentObject* parent, const char* nameProperty,


VarSet rewriter

pieterhijma · 2024-03-13T12:07:57Z

src/App/Expression.cpp

@@ -3301,6 +3445,13 @@ void ConditionalExpression::_visit(ExpressionVisitor &v)
    falseExpr->visit(v);
 }

+void ConditionalExpression::_rewrite(ExpressionVisitor &v)


Generic rewriter

pieterhijma · 2024-03-13T12:08:08Z

src/App/Expression.h

@@ -57,6 +57,22 @@ class AppExport ExpressionVisitor {
 public:
    virtual ~ExpressionVisitor() = default;
    virtual void visit(Expression &e) = 0;
+


Generic rewriter

pieterhijma · 2024-03-13T12:08:22Z

src/App/Expression.h

@@ -76,6 +92,27 @@ class AppExport ExpressionVisitor {
            const std::string &ref, const char *newLabel);
    void moveCells(Expression &e, const CellAddress &address, int rowCount, int colCount);
    void offsetCells(Expression &e, int rowOffset, int colOffset);
+    /** @brief Rewrite an expression that references a variable set.


VarSet rewriter

pieterhijma · 2024-03-13T12:08:46Z

src/App/Expression.h

@@ -161,6 +198,8 @@ class AppExport Expression : public Base::BaseClass {

    void visit(ExpressionVisitor & v);

+    Expression* rewrite(ExpressionVisitor &v);


Generic rewriter

pieterhijma · 2024-03-13T12:15:00Z

src/App/PropertyExpressionEngine.cpp

@@ -858,6 +882,17 @@ void PropertyExpressionEngine::renameObjectIdentifiers(const std::map<ObjectIden
    }
 }

+void PropertyExpressionEngine::rewriteVarSetExpressions(const DocumentObject* parent, const char* nameProperty,


VarSet rewriter

pieterhijma · 2024-03-13T12:15:13Z

src/App/PropertyExpressionEngine.cpp

@@ -1079,3 +1114,8 @@ void PropertyExpressionEngine::onRelabeledDocument(const App::Document &doc)
            e.second.expression->visit(v);
    }
 }
+


VarSet rewriter

pieterhijma · 2024-03-13T12:15:26Z

src/App/PropertyExpressionEngine.h

@@ -150,6 +150,9 @@ class AppExport PropertyExpressionEngine : public App::PropertyExpressionContain

    void renameObjectIdentifiers(const std::map<App::ObjectIdentifier, App::ObjectIdentifier> & paths);

+    void rewriteVarSetExpressions(const DocumentObject * parent, const char * nameProperty,


VarSet rewriter

pieterhijma · 2024-03-13T12:15:40Z

src/App/PropertyExpressionEngine.h

@@ -189,6 +192,7 @@ class AppExport PropertyExpressionEngine : public App::PropertyExpressionContain
                DiGraph &g, ExecuteOption option=ExecuteAll) const;

    void slotChangedObject(const App::DocumentObject &obj, const App::Property &prop);
+    void varSetChanged(const App::DocumentObject &obj, const App::Property &prop);


Dependencies hiddenrefs VarSets

pieterhijma · 2024-03-13T13:13:21Z

I just tried your PR and created a file as shown in your "Part 2" video. Maybe I missed something, but when I change the VarSet outside of Part, no automatic recompute happens. The outside VarSet is set in the Part property created from the exposed VarSet inside of Part. This is obvious because the expression now contains a hiddenref(<<Part>>.Params.Test) call instead of <<Params>>.Test and this breaks the dependency cycle. I understand that this stems from the limitations of the current code. But from a user perspective this is not a usable solution, nobody wants to manually recompute all features that reference the parameters.

Hm, that is not good and not as intended. Thanks for reporting, I will investigate.

I'm not familiar with the depency code in FreeCAD, but my understanding is, that the core problem is, that the dependency resolution (for a recompute?) has a granularity of a DocumentObject. When I create a Part with property X and then create a Body inside of the Part that references that property in a FeatureY, I create a cycle because the logic is as follow:

X is thouched -> Part is touched -> FeatureY must be recomputed and is then touched -> Body must be recomputed and is then touched -> Part must be recomputed and is then touched -> !! Feature Y must be recomputed.

Would it be feasable to change the dependency resolution to a granularity of a property, such that the logic becomes:

Part.X is touched -> FeatureY.Height is touched -> FeatureY.Shape must be recomputed and is then touched -> Body.Shape must be recomputed and is then touched -> Part.Shape must be recomputed and is then touched

As long as Part.X is not dependent on Part.Shape there is no cycle. This fits with the separation of computing the shape and the execute() function as mentioned in #12868. (Just a thought I had, even with a cycle, some kind of fixed-point iteration could also be useful in some constelations)

This is obviously a bigger change to the core code, but one that would allow a more precise dependency tracking across all workbenches. Implementing variant links might then become much more straight forward.

I don't think this is the issue here. I've added dependency tracking to the dependency tracking for hiddenrefs, so I would expect the above to work. I believe I have also a test for exactly this.

The property-based dependency tracking you sketch above is something I have in mind as well and I could envision it to be a conclusion of #12868 that we need that kind of functionality. But indeed, it would be a big change from what we have now.

luzpaz · 2024-03-13T16:01:50Z

Linter report:

Warning: src/App/Part.cpp:113:    <-- trailing whitespace
Warning: src/App/Part.h:89:    <-- trailing whitespace
Warning: src/App/PropertyVarSet.h:40:    <-- trailing whitespace
Warning: src/App/VarSet.cpp:241:std::string VarSet::getNameOriginalVarSetProperty(const char* name) <-- trailing whitespace
Warning: src/App/VarSet.cpp:299:        <-- trailing whitespace
Warning: src/App/VarSet.h:58:    <-- trailing whitespace
Warning: src/App/VarSet.h:69:    <-- trailing whitespace
Warning: src/App/VarSet.h:121:    <-- trailing whitespace
Warning: src/App/VarSet.h:147:    <-- trailing whitespace
Warning: src/App/VarSet.h:148:    }; <-- trailing whitespace

pieterhijma · 2024-03-13T16:04:30Z

I just tried your PR and created a file as shown in your "Part 2" video. Maybe I missed something, but when I change the VarSet outside of Part, no automatic recompute happens. The outside VarSet is set in the Part property created from the exposed VarSet inside of Part. This is obvious because the expression now contains a hiddenref(<<Part>>.Params.Test) call instead of <<Params>>.Test and this breaks the dependency cycle. I understand that this stems from the limitations of the current code. But from a user perspective this is not a usable solution, nobody wants to manually recompute all features that reference the parameters.

Hm, that is not good and not as intended. Thanks for reporting, I will investigate.

I've just redid the video with the new implementation and for me it works as intended. That is, if I change the global properties of length height and thickness of the wood, the assembly changes accordingly. The only thing I can think of is that somehow property Part.Params is pointing to a different VarSet than you expect?

NomAnor · 2024-03-15T12:11:44Z

Now it somehow works. Not sure what went wrong. I did a clean build of main with your PR merged.

When are the expressions rewritten?
As a test, I added a second property to the original VarSet and then changed my expression to point to it (<<Params>>.Test2). Then I added the same property to my custom VarSet. Changing the value in the custom VarSet did not influence the expression value, because it was not rewritten to the hiddenref.
As a user I would expect, that I can change the expression without having to worry about adding the hiddenref call myself.

After toggling the Exposed property the expression was rewritten, but I had to relink my custom VarSet.

pieterhijma · 2024-03-18T15:32:50Z

Now it somehow works. Not sure what went wrong. I did a clean build of main with your PR merged.

I can imagine that you can end up in a situation where it doesn't work properly. The dependencies are quite complicated because temporary hidden files are in the loop as well. But I think this is true for all subshapebinders that do bind copy-on-change.

When are the expressions rewritten? As a test, I added a second property to the original VarSet and then changed my expression to point to it (<<Params>>.Test2). Then I added the same property to my custom VarSet. Changing the value in the custom VarSet did not influence the expression value, because it was not rewritten to the hiddenref. As a user I would expect, that I can change the expression without having to worry about adding the hiddenref call myself.

After toggling the Exposed property the expression was rewritten, but I had to relink my custom VarSet.

Yes, good point. Indeed, you figured out yourself that this is the moment that the expressions are rewritten. Adding a property to the original VarSet breaks the model because the VarSet as argument is not equivalent with the original anymore (in the same way where a computer program is broken if you change the argument list of a function). You solved it by adding the property to the argument VarSet as well.

I'm thinking on how to best approach this problem. It will probably be common that you want to add a variable to an orginal VarSet. One solution would be to only allow adding properties if the VarSet is not exposed. However, although it doesn't necessarily break existing binders immediately, it is true that you have to relink all the argument VarSets.

A different approach is to allow adding properties to the original VarSets, and add validation to the variants at strategic places that tell you that a PropertyVarSet is invalid and allow you to make the current VarSet equivalent to the original one. It can then redo the expression rewrite.

@NomAnor, do you have any preference or perhaps other suggestions?

sliptonic · 2024-03-18T16:47:38Z

This issue touches the core so it needs more scrutiny. The expression engine is already complex and this adds additional complexity to achieve new functionality.
I'm adding @wwmayer and @chennes for review and comment

NomAnor · 2024-03-18T17:29:31Z

The dependency management could get hairy, I don't know enough about it to give a good suggestion.

If I put myself in the shoes of a user, I would want the following features:

Edit (add, remove, rename) the properties of the original VarSet.
When tinkering with a design, changes always happen. This can break expressions in the part, which should be visible because eg. a sketch shows a broken icon. This is expected. It can also break "users" of the part and they have to change their VarSets, this is also expected. A hint for the missing properties in the "users" VarSet would be helpful.
Edit expressions that use the exposed properties.
When editing the expression a user should not have to think about wether the value comes from the original VarSet or some hiddenref of the Part container. After editing, the expression must automatically work with "user" provided VarSets (as long as they contain the property).
I don't know what a good implementation for this would look like. A user would be familiar with just referencing the original VarSet, as I tried. But after editing it would have to automatically be rewritten to contain the hiddenref, without user action.

A someone using a parametric Part, it would be nice if the interface would not be strict. That is, if the VarSet I use does not contain all properties of the original VarSet, the values from the original VarSet are used. This way I can only overwrite the values I want to change and I'm not that suceptible to changes in the original VarSet.

src/App/Part.cpp

src/App/PropertyExpressionEngine.cpp

src/App/PropertyVarSet.cpp

pieterhijma · 2024-03-29T09:43:22Z

Linter report:

Warning: src/App/Part.cpp:113:    <-- trailing whitespace
...

Thank you for pointing that out. I was relying on the pre-commit hooks but it turns out src/App (and src/Gui) is not part of the .pre-commit-config.yaml. I guess this is deliberate? I can't find anything about it in for example #9140 and this forum post.

For an App::Part that has exposed variable sets, check if the expressions are rewritten.

pieterhijma · 2024-03-30T20:06:43Z

The dependency management could get hairy, I don't know enough about it to give a good suggestion.

If I put myself in the shoes of a user, I would want the following features:

Edit (add, remove, rename) the properties of the original VarSet.
When tinkering with a design, changes always happen. This can break expressions in the part, which should be visible because eg. a sketch shows a broken icon. This is expected. It can also break "users" of the part and they have to change their VarSets, this is also expected. A hint for the missing properties in the "users" VarSet would be helpful.

Good point. Setting expose to false and then to true again is a solution that doesn't scale, because all the variants loose the VarSets they're parameterized with, so the user has to reset everything. I've added a check in Part that rewrites the expressions in the geometry of the part that is parameterized with new variables in an exposed VarSet. On recompute of a variant, the variant is broken because the updated geometry will reference properties that are not in the VarSets that replace the exposed one. I think this is the right behavior because the error message is quite clear and prompts the user to add the missing property in the replacing VarSet to make it equivalent again.

Edit expressions that use the exposed properties.
When editing the expression a user should not have to think about wether the value comes from the original VarSet or some hiddenref of the Part container. After editing, the expression must automatically work with "user" provided VarSets (as long as they contain the property).
I don't know what a good implementation for this would look like. A user would be familiar with just referencing the original VarSet, as I tried. But after editing it would have to automatically be rewritten to contain the hiddenref, without user action.

Yes, that is what is happening with the latest commit. The user can simply reference the exposed VarSet and then the expression is rewritten immediately.

A someone using a parametric Part, it would be nice if the interface would not be strict. That is, if the VarSet I use does not contain all properties of the original VarSet, the values from the original VarSet are used. This way I can only overwrite the values I want to change and I'm not that suceptible to changes in the original VarSet.

That is a good point. That would indeed be nice. This would be very hard to do with the current implementation, but it may be possible with what I propose in #12868.

yorikvanhavre · 2024-04-17T08:32:46Z

@pieterhijma and me scheduled a video meeting tomorrow (thursday 18 april) at 15h00 CEST (13h00 UTC) to go through the code and explain in detail. Anyone interested is welcome to join/listen. We'll post a jitsi link here at that time tomorrow

pieterhijma · 2024-04-25T19:42:34Z

As Yorik mentioned in the comment above, we had a meeting about this PR. In this comment I will provide a summary of the meeting.

Yorik and Shai were present. Thank you both for your interest! The meeting had two purposes. Firstly, I wanted to explain why this PR touches so many files and why the functionality is necessary. Secondly, Yorik and Shai could give feedback on the FPA grant proposal that was declined. In this comment, I will provide a summary on the former part. In the grant proposal issue, I will summarize the feedback on the grant proposal.

I showed Yorik and Shai the purpose of this PR, enabling exposing variable sets making use as much as possible of the logic already available in FreeCAD, notably subshapebinders and the copy-on-change logic. The current implementation is not fully satisfying because it runs into some limitations of the copy-on-change logic and that's why I submitted the grant proposal.
By means of diffs on the code I tried to show that there is code confined to variable sets and code that interacts with other parts of the core, mainly to rewrite expressions.

While I was explaining the code, the question came up why exactly I needed the functionality to rewrite expressions. To show this, I started FreeCAD in this branch and explained that to expose variables (i.e. making sure properties of a parent Part can be used within the child document objects), I need to get around the dependency cycle problem which I can achieve by making use of hidden references. To make it as transparent as possible for the user, I automatically rewrite expressions to use these hidden references.

This also gave the opportunity to explain that although FreeCAD has logic available for variant parts, the functionality is very difficult to use for regular users and is brittle. We discussed several existing ways to achieve variant parts:

Making copies: This has the drawback that if the design of the original changes, the copies will not be updated.
Copy-on-change links: Users need to manually (and carefully) add hidden references, create properties in the parent object, mark them as copy-on-change, make a link, make the link binding copy-on-change, change the parameters. It will still have the problem that if the original design changes, the copies will not be updated. The copy-on-change link will have a complete copy of the original.
Copy-on-change tracked links: Instead of "Enabled", the user chooses "Tracking" for the copy-on-change binding. It has the same problems as above, but the design is updated when the original changes. However, each time the original design changes, a new copy within the link will be created, creating new document object names. This means that this link is not a "first-class citizen", because it is not possible to build on the geometry of this link because of changing document object names.
Copy-on-change subshapebinders: The document has no copy of the original geometry, but a flattened representation. There is still a copy maintained, but it is in a hidden temporary file. This hidden file is not completely transparently hidden (for example you see it in the dialog when inserting links). The dependency tracking is complicated because the subshapebinder depends on the copy in the hidden file, which depends on the original object.

Since hidden references are not incorporated from the dependency checks (that is what the hidden means), there is separate logic available to manage dependencies. So, in a sense, FreeCAD has two dependency mechanisms. Since this PR makes use of the logic above that has drawbacks, this implementation suffers from the same drawbacks. Hence I created an FPA grant-proposal to address these issues.

Although the idea behind the PR is now more clear, Yorik has reservations for adding more complicated code to the core code base. The core should be understandable by all developers and large changes should receive a high amount of scrutiny. The same reservations are also valid for the grant proposal, so I will continue to elaborate on this in the grant proposal issue.

I will continue the summary of this meeting regarding the FPA grant-proposal there. @yorikvanhavre and @shaise, please comment if I misrepresent anything or if I miss something.

shaise · 2024-04-25T21:11:06Z

I think @yorikvanhavre also mentioned that it will be better if this proposal can be divided to several steps with smaller PRs. This way it will be easier to follow the changes.

pieterhijma · 2024-04-26T20:37:06Z

I think @yorikvanhavre also mentioned that it will be better if this proposal can be divided to several steps with smaller PRs. This way it will be easier to follow the changes.

Yes, thank you. In my opinion that was more about the proposal, so I incorporated that in the summary of the proposal (that wasn't available when you replied to this, sorry).

yorikvanhavre · 2024-05-03T09:23:20Z

I would indeed prefer this to be separated into clearer distinct topics. Basically I would make the expression rewrite system a separate PR, then one that shows more clearly what the VarSet is doing.

maxwxyz · 2024-05-15T18:34:11Z

@pieterhijma also here a heads up for the upcoming feature freeze on Monday, 2024-06-03. Are you planning to split this up as mentioned?

github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Feb 21, 2024

pieterhijma mentioned this pull request Feb 21, 2024

[Problem] Managing Custom Data Elements #12120

Open

2 tasks

pieterhijma force-pushed the expose-varset branch from 9d42214 to 9c9e028 Compare March 4, 2024 15:19

pieterhijma added 4 commits March 7, 2024 11:22

[Core] Enable exposing VarSets in App::Part

18e6bb6

Core: Add VarSet tests

ec9ac44

pieterhijma mentioned this pull request Mar 7, 2024

[Problem] Expression visitor / modifier cannot rewrite expression trees #12799

Open

2 tasks

pieterhijma force-pushed the expose-varset branch from 9c9e028 to ec9ac44 Compare March 7, 2024 11:30

pieterhijma marked this pull request as ready for review March 7, 2024 14:39

pieterhijma mentioned this pull request Mar 11, 2024

Generalize exposing variables / fundamental solution variants #12868

Open

2 tasks

yorikvanhavre self-requested a review March 11, 2024 16:40

pieterhijma commented Mar 13, 2024

View reviewed changes

sliptonic requested review from chennes and wwmayer March 18, 2024 16:43

wwmayer reviewed Mar 19, 2024

View reviewed changes

src/App/Part.cpp Outdated Show resolved Hide resolved

wwmayer reviewed Mar 19, 2024

View reviewed changes

src/App/PropertyExpressionEngine.cpp Outdated Show resolved Hide resolved

wwmayer reviewed Mar 19, 2024

View reviewed changes

src/App/PropertyVarSet.cpp Outdated Show resolved Hide resolved

Core: Remove trailing whitespace

ea12158

pieterhijma added 3 commits March 29, 2024 11:09

Core: Process comments wmayer

d0a8836

Core: Test other ways to add objects to varsets

2caedbb

Core: Check for VarSet expression rewrite in Part

a6d5ca9

For an App::Part that has exposed variable sets, check if the expressions are rewritten.

pieterhijma mentioned this pull request Apr 3, 2024

Generalize Exposing Properties FreeCAD/FPA-grant-proposals#3

Open

chennes marked this pull request as draft April 22, 2024 15:42

		@@ -161,6 +198,8 @@ class AppExport Expression : public Base::BaseClass {

		void visit(ExpressionVisitor & v);

		Expression* rewrite(ExpressionVisitor &v);

		@@ -150,6 +150,9 @@ class AppExport PropertyExpressionEngine : public App::PropertyExpressionContain

		void renameObjectIdentifiers(const std::map<App::ObjectIdentifier, App::ObjectIdentifier> & paths);

		void rewriteVarSetExpressions(const DocumentObject * parent, const char * nameProperty,

[Core] Enable exposing VarSets in App::Part #12532

Are you sure you want to change the base?

[Core] Enable exposing VarSets in App::Part #12532

Conversation

pieterhijma commented Feb 21, 2024 • edited

pieterhijma commented Feb 21, 2024 • edited by maxwxyz

pieterhijma commented Mar 7, 2024

yorikvanhavre commented Mar 12, 2024

NomAnor commented Mar 12, 2024

pieterhijma commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieterhijma Mar 13, 2024 • edited

Choose a reason for hiding this comment

pieterhijma Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieterhijma commented Mar 13, 2024

luzpaz commented Mar 13, 2024

pieterhijma commented Mar 13, 2024

NomAnor commented Mar 15, 2024 • edited

pieterhijma commented Mar 18, 2024

sliptonic commented Mar 18, 2024

NomAnor commented Mar 18, 2024

pieterhijma commented Mar 29, 2024

pieterhijma commented Mar 30, 2024

yorikvanhavre commented Apr 17, 2024

pieterhijma commented Apr 25, 2024

shaise commented Apr 25, 2024

pieterhijma commented Apr 26, 2024

yorikvanhavre commented May 3, 2024

maxwxyz commented May 15, 2024

pieterhijma commented Feb 21, 2024 •

edited

pieterhijma commented Feb 21, 2024 •

edited by maxwxyz

pieterhijma Mar 13, 2024 •

edited

pieterhijma Mar 13, 2024 •

edited

NomAnor commented Mar 15, 2024 •

edited