logari81 wrote: ↑Wed Oct 05, 2022 1:53 pm
Sorry guys, you have hijacked someone else's thread, I think the moderators should split irrelevant discussions, I come to this thread to look for an update about the merging of realthunder's implementation.
Discussions about toponaming aren't off topic. There's probably going to be an announcement soon for the first official beta, stay tuned but not necessarily to this topic.
ok I see now that the discussion has turned into identifying deficiencies of RT's solution, which I agree is relevant, and this is what I had actually asked about before:
logari81 wrote: ↑Thu Aug 11, 2022 9:17 am
Is there at any point a list of issues with RealThunder's implementation that prevent merging it?
I have read RealThunder's description of his solution and looks absolutely convincing to me.
Now my next question is, have you tried/managed to communicate the case(s) that is(are) apparently failing to RT and get his opinion on the issue?
logari81 wrote: ↑Wed Oct 05, 2022 1:53 pm
Sorry guys, you have hijacked someone else's thread, I think the moderators should split irrelevant discussions, I come to this thread to look for an update about the merging of realthunder's implementation.
You are right! Alas, it's a hassle to get this sorted. This topic started as an announcment of Realthunder, and we definitely need a place where he presents his work, and also a place where where to discuss it. There are several topics now and I am unsure how to continue.
A milestone for the next release is the integration of the TNP stuff into master. I have already created a subforum "Toponaming and Link Branch" for discussing this, and maybe we should create another topic in there, where @realthunder can post his progress. I can then pin that topic and I could also try to lock it, but I'm not sure, if the author than can still edit his own first post.
C_h_o_p_i_n wrote: ↑Fri Sep 30, 2022 11:12 am
if too many chain-elements inbetween are deleted
Too many ? We're talking about 3 simple consecutive PartDesign fonctions where 1 is deleted in the middle, and which doesn't pose any problems in FreeCAD 0.20
So i think it's quite normal that a manually reatachment of any orhphaned leaves/twigs might sometime be neccessary.
But then, what's the avantage of all this new code ? FreeCAD v0.20 cannot deal with insertions, LinkDaily cannot deal with deletions.
Missing this post some how. Too busy recently. Anyway, the problem you discovered is due a bug introduced by a recent change in Jun. The problem is not really about topo naming. Because when a feature is deleted, the core behavior (both my branch and upstream master) is to break all link to this feature. So the sketch is effectively detached. The bug is caused by the AttachExtension reseting the placement before calculating the new position. The fix is simple, just restore the placement on abort or error. See here.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
realthunder wrote: ↑Thu Oct 06, 2022 1:34 pm
Missing this post some how. Too busy recently. Anyway, the problem you discovered is due a bug introduced by a recent change in Jun. The problem is not really about topo naming. Because when a feature is deleted, the core behavior (both my branch and upstream master) is to break all link to this feature. So the sketch is effectively detached. The bug is caused by the AttachExtension reseting the placement before calculating the new position. The fix is simple, just restore the placement on abort or error. See here.
while restoring the old placement on error brings linkdaily back in parity with master branch it would be nice if the attachment itself used the toponaming algorithm, from my example using fillet/chamfer on the face it is clear that the algorithm is capable of finding the correct face after it breaks so it would seem to be just missing some implementation to make use of the algorithm for attachments. Sketching on faces is one of the big features users seem to want so it's a shame that it doesn't use the toponaming algorithm.
chennes wrote: ↑Fri Oct 07, 2022 5:07 pm
@realthunder -- in ComplexGeoData, why did you decide to use QVector for ElementIDRefs? (I ask just because it's the lone Qt inclusion in that class).
For the implicit sharing feature, same reason I use QByteArray in StringID and stuff.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
adrianinsaval wrote: ↑Thu Oct 06, 2022 5:05 pm
while restoring the old placement on error brings linkdaily back in parity with master branch it would be nice if the attachment itself used the toponaming algorithm, from my example using fillet/chamfer on the face it is clear that the algorithm is capable of finding the correct face after it breaks so it would seem to be just missing some implementation to make use of the algorithm for attachments. Sketching on faces is one of the big features users seem to want so it's a shame that it doesn't use the toponaming algorithm.
I actually implement auto fix in AttachExtension, but not when the linked object gets deleted. Haven't thought about how to handle that yet, as it is a low level action in the core. In my branch, PartDesign feature offers an option to be 'suppressed', and this can be considered as 'feature deletion' without affecting the object linking. Now that you've mentioned it, I did some test. And it seems that the aforementioned attacher changes I added a few month ago broke this auto fix function too. I just fixed it, and you can see the auto fix in action in the screencast below.
attacher.gif (505.86 KiB) Viewed 1839 times
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
From the screenshot I may see that face index is equal to 5 when fillet is activated and is equal to 6 when it is suppressed.
But the Opencascade Class "BRepFilletAPI_MakeFillet" provides the following methodes:
virtual const TopTools_ListOfShape & Generated (const TopoDS_Shape &EorV) override
Returns the list of shapes generated from the shape <EorV>. More...
virtual const TopTools_ListOfShape & Modified (const TopoDS_Shape &F) override
Returns the list of shapes modified from the shape <F>. More...
virtual Standard_Boolean IsDeleted (const TopoDS_Shape &F) override
Returns true if the shape S has been deleted.
Using them it should be very easy to associate each modified face with a preexisting face and reuse its original index.
New indices (or new names) should be introduced only for the new (curved) faces generated by the fillet.
The top face of box in the screenshot is clearly a modified (not a generated) face and should keep the index number 6 after making a fillet.
In that way the sketch attachment would be never invalidated after making or removing a fillet.
wsteffe wrote: ↑Sat Oct 08, 2022 12:50 pm
From the screenshot I may see that face index is equal to 5 when fillet is activated and is equal to 6 when it is suppressed.
But the Opencascade Class "BRepFilletAPI_MakeFillet" provides the following methodes:
virtual const TopTools_ListOfShape & Generated (const TopoDS_Shape &EorV) override
Returns the list of shapes generated from the shape <EorV>. More...
virtual const TopTools_ListOfShape & Modified (const TopoDS_Shape &F) override
Returns the list of shapes modified from the shape <F>. More...
virtual Standard_Boolean IsDeleted (const TopoDS_Shape &F) override
Returns true if the shape S has been deleted.
Using them it should be very easy to associate each modified face with a preexisting face and reuse its original index.
New indices (or new names) should be introduced only for the new (curved) faces generated by the fillet.
The top face of box in the screenshot is clearly a modified (not a generated) face and should keep the index number 6 after making a fillet.
In that way the sketch attachment would be never invalidated after making or removing a fillet.
The first two functions are the core APIs my algorithm relied on to encode the topo naming. What happens underneath is too complex for your suggestions to work. For example, the sketch shouldn't be aware of the nature of the shape it is attached to, or what kind of changes the shape is going through. In most cases, there are multiple modeling steps inside each PartDesign feature, some steps, i.e. calls to OCC classes provide the above APIs, some do not. And you are required to retain the old shape in order to do the tracing using those APIs. Not just the final result shape, but shapes from all intermediate steps. What my algorithm does is to abstract all these into pure text encoding, and allow trace back and forth without requiring the actual OCC shape.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
realthunder wrote: ↑Sat Oct 08, 2022 11:28 pm
you are required to retain the old shape in order to do the tracing using those APIs. Not just the final result shape
But only until you have generated the names of modified faces. Then you may delete the old shape. I Mean something like this:
BRepFilletAPI_MakeFillet mkFillet(baseShape);
const TopoDS_Edge edge = ...;
mkFillet.Add(radius1, radius2, edge);
TopoDS_Shape newShape = mkFillet.Shape();
for (TopExp_Explorer exp(baseShape,TopAbs_FACE); exp.More(); exp.Next()){
TopoDS_Shape F = exp.Current();
const TopTools_ListOfShape& RSlist =mkFillet.Modified(F);
TopoDS_Shape modifiedF=RSit.Value(); //RSList may hold no more than 1 face (0 if original face was unchanged)
if(modifiedF) indexAndName[modifiedF]=indexAndName[F];
}
//Here baseShape is not needed anymore
Of course the general TN algorithm should be suspended (not used) for the face names handled by this code snippet.