Topological Naming, My Take

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
Zolko
Veteran
Posts: 2213
Joined: Mon Dec 17, 2018 10:02 am

Re: Topological Naming, My Take

Post by Zolko »

adrianinsaval wrote: Thu Sep 15, 2022 2:03 pm I think he has, but zolko kinda wanted to start over just for sketches.
It's not that simple as that. And I still/again think that the current plan to merge the topo-naming solution is wrong. Now, of course, I could just lay back and see this whole thing fail and come back and say "I told you so", or hope for the best that I'm wrong and it succeeds, but that would not be fair to any of us. And least of all to realthunder. I really appreciated to work we had done on App::Link and Assembly4 in the days of pre-0.19 (he did most of the coding of course !) and I wouldn't stand by if the problem I've seen is real. Please read carefully.

You're right that my staring point was not very scientific, merely an intuition that this was not going to fly. As it happens, my intuitions are often right and I have a tendency to follow them. It did bring me into trouble sometimes, but it also saved quite many projects. If I'm wrong you can put the blame on me, I won't die from it.

As it happens, user GuGa has raised a problem in the German forum. Admittedly, he presented his case quite badly, but he did uncover a real problem with LinkDaily topo-naming method. Unfortunately, people didn't understood what he was after and the thread has been blocked. The problem is very easy to reproduce:

  • in a new document, using only PartDesign, create a new Body.
  • create a sketch, draw a rectangle, exit.
  • pad the sketch
  • select an edge and make a fillet
  • select a face, draw a sketch (a hole) and make a pocket through all
  • delete the fillet
  • the hole disappears, and the sketch of it is on a wrong face
This using LinkDaily from july 2022 :

Code: Select all

OS: Debian GNU/Linux 11 (bullseye) (KDE/plasma)
Word size of FreeCAD: 64-bit
Version: 2022.709.26244 +5001 (Git) AppImage
Build type: Release
Branch: LinkDaily
Hash: 096210d21183e9dfdc3b25777760bfb6c00a210b
Python version: 3.9.13
Qt version: 5.12.9
Coin version: 4.0.1
OCC version: 7.5.3
Locale: English/United Kingdom (en_GB)

I then have tried to test such features, and as I have found out the LinkDaily toponaming algorith is very good for additions and modifications, but it can't deal at all with deletions. It's actually worse than stock FreeCAD for this. Having the bad reputation I have, I didn't want to be the harbinger of bad news, so I decided to let it be. But since I'm quoted here I want to bring my explanation, and also a plan forward.

So the first question is: can people reproduce the problem from the steps above, using LinkDaily ? If yes, I think that apologies to user GuGa would be welcome (and if no, please blame me and leave him alone):

GuGa wrote: Thu Sep 08, 2022 7:35 pm ...
On a technical level, I've read realthunder's entry in the wiki (https://github.com/realthunder/FreeCAD_ ... cal-Naming) and I see that the base feature are 2 new functions setElementName() and getElementName(). This is logical and corresponds to what I have also proposed in the thread toponaming solution for sketches: it all begins with FreeCAD being able to explicitly giving names to geometric elements. So far so good.

If you grep setElementName on LinkDaily source code, you get returns only from :

Code: Select all

App/ComplexGeoData.***
Mod/Part/App/TopoShapeEx.***
Mod/Sketcher/App/SketchObject.***
which means that these 3 files (and their friends) are the only ones to directly use the setElementName() function. The function is defined in ComplexGeoData, and used only in Mod/Part/App/TopoShapeEx and Mod/Sketcher/App/SketchObject. Which also means that Sketcher is treated separately from the rest of FreeCAD, even in realthunder's branch. It wasn't my imagination to treat sketcher differently, it's the reality of FreeCAD's source code. What I proposed was merely to begin with treating the Sketcher case before the Part case.

Unfortunately, this is not the current plan:

  • The first batch of patches makes necessary core changes to build up the foundation for generation and utilization of the new Topo Naming. There is no visible changes for the end-user at this stage
  • The next batch of patches adds code to the features from Part workbench (...) And if sketch is involved (e.g. Extrusion from a sketch), then try not to modify the sketch
  • The third batch of patches modifies the Sketcher workbench to fully support the new Topo Naming
If we follow this plan, we won't see any real benefits, but also no real problems, before EVERYTHING is merged. And if/when the problems appear, we'll have to deal with a huge chunk of code already merged.

So what I propose, again, is the following:

  • merge the explicit naming of elements ...
  • ... but not realthunder's naming algorithm. Leave it to FreeCAD how elements can be explicitly named
  • fix Sketcher so that the internal naming is persistent : Constraint_19 should always be COnstraint_19 even if Constraint_18 has been deleted. Same for lines, points, ...
  • let Sketcher use setElementName() with it's own internal - now persistent - naming convention : in this case, the topological naming will, if everything goes according to plan, be solved for sketches
  • if not, we have limited amount of code to search for
  • now a big chunk of the code can be used and debugged
  • release FreeCAD v0.21
  • merge the rest of toponaming on top of the debugged code
  • release FreeCAD v1.0
try the Assembly4 workbench for FreCAD — tutorials here and here
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Topological Naming, My Take

Post by adrianinsaval »

Zolko wrote: Sun Sep 25, 2022 6:16 pm The problem is very easy to reproduce:
yeah I can reproduce too, which is weird, I could have sworn these issues weren't so common anymore there, but we always knew the algorithm can't possibly be perfect, it should be able to deal with these type of stuff though so I'm guessing the toponaming is not used for attachment? or is broken there now? @realthunder please confirm if this is supposed to work.
I say this because if you have the same scenario but instead of attaching a sketch to the face make a chamfer on the same face and then delete the fillet the algorithm is able to find the face, freecad reports and error and requires that you double click on the chamfer to confirm it found the right face, but this is good. So the method to find the face exists but it's not used by attachment.
Either way, that doesn't have much to do with sketcher toponaming IMO, it's the attachment that's missing the auto fixing capabilities and if I'm not mistaken this is Part stuff.
I then have tried to test such features, and as I have found out the LinkDaily toponaming algorith is very good for additions and modifications, but it can't deal at all with deletions
how so? in that example but using a chamfer instead of attaching sketch master breaks after deleting the fillet but linkdaily can recover immediately after double clicking the chamfer.
The function is defined in ComplexGeoData, and used only in Mod/Part/App/TopoShapeEx and Mod/Sketcher/App/SketchObject. Which also means that Sketcher is treated separately from the rest of FreeCAD, even in realthunder's branch. It wasn't my imagination to treat sketcher differently, it's the reality of FreeCAD's source code. What I proposed was merely to begin with treating the Sketcher case before the Part case.
but why should sketcher be dealt with first? AFAICT there's no real technical reason, it's just your preference. If I'm not mistaken sketches are derived from Part too so it might be necessary to start with since it's the granddaddy of most features in FreeCAD, yes for the user the sketch might be the basis to build most stuff but internally Part is the basis for most things! @realthunder in order to settle this, could you shortly explain why the patches for Part where submitted before sketcher? are there technical reasons? would there be any merit at all in dealing with sketcher first?
If we follow this plan, we won't see any real benefits, but also no real problems, before EVERYTHING is merged.
we've already seen real problems (already fixed I think) and real benefits, and it's not merged in master yet, it does look like everything will end up being merged at once though, since we're already at the sketcher stuff stage and it doesn't look like each batch has gotten very thorough testing and review individually. I'm no sure if this is really too bad though, the benefits are huge.
[*]... but not realthunder's naming algorithm. Leave it to FreeCAD how elements can be explicitly named
why? realthunder is assigning names to geometry but you want to reimplement this because... reasons? what actual flaw have you found in how realthunder's implementation is assigning names to sketch edges? why should we reject his method if nobody is really working on an alternative? It would be different if someone would indeed work on a better method but it doesn't look like that is happening.
[*]fix Sketcher so that the internal naming is persistent : Constraint_19 should always be COnstraint_19 even if Constraint_18 has been deleted.
I too would like this a lot but this has nothing at all to do with toponaming, it's just a similar problem.
[*]let Sketcher use setElementName() with it's own internal - now persistent - naming convention : in this case, the topological naming will, if everything goes according to plan, be solved for sketches
is this really different to what realthunder does? you've said yourself sketcher is using setElementName in realthunder's code, it might not be using the list index but I don't think this is important, as long as it's using a sufficiently unique and persistent identifier.

Edit: additional thoughts on this, as I said before (although not in response to you but to a silly proposal to preallocate empty list indexes) there's no real reason why we should use consecutive natural numbers as unique identifiers for geometry, I don't think it's too complicated for the software to differentiate between between d76a54sd and nla7v08w (random characters to convey the concept) as opposed to differentiating between 15 and 82, it might actually be simpler to use a hash like identifier in the long run since you don't need to store placeholders for deleted objects, you don't need to know that nla7v08w doesn't exist anymore to avoid using the name again or to ensure d76a54sd keeps the same name.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Topological Naming, My Take

Post by adrianinsaval »

@Zolko I just checked it and it seems pretty much what you want is already done, check the Document.xml of these. I did this:
• create a sketch with two concentric circles
• save as sketcher ex.fcstd
• remove the second circle and create two new ones
• save as sketcher ex2.fcstd

now see the difference in the geometry property in the xml, specifically notice the id attribute (which doesn't exist if you save in master branch)
sketcher ex.fcstd:

Code: Select all

        <Property name="Geometry" type="Part::PropertyGeometryList" status="8192">
          <GeometryList count="2">
            <Geometry type="Part::GeomCircle" migrated="1">
              <GeoExtensions count="1">
                <GeoExtension type="Sketcher::SketchGeometryExtension" id="1" internalGeometryType="0" geometryModeFlags="00000000000000000000000000000000" geometryLayer="0"/>
              </GeoExtensions>
              <Circle CenterX="0.0000000000000000" CenterY="0.0000000000000000" CenterZ="0.0000000000000000" NormalX="0.0000000000000000" NormalY="0.0000000000000000" NormalZ="1.0000000000000000" AngleXU="-0.0000000000000000" Radius="145.3911359999999888"/>
            </Geometry>
            <Geometry type="Part::GeomCircle" migrated="1">
              <GeoExtensions count="1">
                <GeoExtension type="Sketcher::SketchGeometryExtension" id="2" internalGeometryType="0" geometryModeFlags="00000000000000000000000000000000" geometryLayer="0"/>
              </GeoExtensions>
              <Circle CenterX="0.0000000000000000" CenterY="0.0000000000000000" CenterZ="0.0000000000000000" NormalX="0.0000000000000000" NormalY="0.0000000000000000" NormalZ="1.0000000000000000" AngleXU="-0.0000000000000000" Radius="102.2947560000000067"/>
            </Geometry>
          </GeometryList>
        </Property>
sketcher ex.fcstd:

Code: Select all

        <Property name="Geometry" type="Part::PropertyGeometryList" status="8192">
          <GeometryList count="3">
            <Geometry type="Part::GeomCircle" migrated="1">
              <GeoExtensions count="1">
                <GeoExtension type="Sketcher::SketchGeometryExtension" id="1" internalGeometryType="0" geometryModeFlags="00000000000000000000000000000000" geometryLayer="0"/>
              </GeoExtensions>
              <Circle CenterX="0.0000000000000000" CenterY="0.0000000000000000" CenterZ="0.0000000000000000" NormalX="0.0000000000000000" NormalY="0.0000000000000000" NormalZ="1.0000000000000000" AngleXU="-0.0000000000000000" Radius="145.3911359999999888"/>
            </Geometry>
            <Geometry type="Part::GeomCircle" migrated="1">
              <GeoExtensions count="1">
                <GeoExtension type="Sketcher::SketchGeometryExtension" id="3" internalGeometryType="0" geometryModeFlags="00000000000000000000000000000000" geometryLayer="0"/>
              </GeoExtensions>
              <Circle CenterX="-54.4034959999999970" CenterY="26.6913149999999995" CenterZ="0.0000000000000000" NormalX="0.0000000000000000" NormalY="0.0000000000000000" NormalZ="1.0000000000000000" AngleXU="-0.0000000000000000" Radius="40.7136139999999997"/>
            </Geometry>
            <Geometry type="Part::GeomCircle" migrated="1">
              <GeoExtensions count="1">
                <GeoExtension type="Sketcher::SketchGeometryExtension" id="4" internalGeometryType="0" geometryModeFlags="00000000000000000000000000000000" geometryLayer="0"/>
              </GeoExtensions>
              <Circle CenterX="229.6534729999999911" CenterY="85.3964229999999986" CenterZ="0.0000000000000000" NormalX="0.0000000000000000" NormalY="0.0000000000000000" NormalZ="1.0000000000000000" AngleXU="-0.0000000000000000" Radius="441.3701019999999744"/>
            </Geometry>
          </GeometryList>
        </Property>
the first circle i created gets id=1 and the second id=2, I delete the second circle and then create two new circles, these new circles get id=3 and id=4, I'm not sure how it keeps track of this though. I further tested removing the first circle and saving as sketcher ex3.fcstd and confirmed that the new circles kept the id=3 and id=4 respectively. So realthunder is already doing what you want, persistent integer identifiers for sketcher geometry.

These files were done with a build from today of https://github.com/FreeCAD/FreeCAD/tree ... toponaming

Code: Select all

OS: Linux 5.15.70-1-lts (KDE)
Word size of FreeCAD: 64-bit
Version: 0.21.29382 +62 (Git)
Build type: None
Branch: development/toponaming
Hash: 7cb9ba470c169a39ec14bf3c2dafc12cd67bfd9e
Python 3.10.7, Qt 5.15.6, Coin 4.0.1, Vtk 9.1.0, OCC 7.5.3
Locale: C/Default (C) [ OS: English/United Kingdom (en_GB) ]
Attachments
sketcher ex3.FCStd
(7.95 KiB) Downloaded 25 times
sketcher ex2.FCStd
(8.13 KiB) Downloaded 25 times
sketcher ex.FCStd
(7.95 KiB) Downloaded 28 times
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: Topological Naming, My Take

Post by wsteffe »

adrianinsaval wrote: Mon Sep 26, 2022 12:50 am yeah I can reproduce too, which is weird
Having reproduced it with LinkDaily I have seen that the sketch didn't went back to the original face even after having manually fixed its Attachment Support (selecting the original pad face).

Really weird. Hi @realthunder, have you seen that ?
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: Topological Naming, My Take

Post by wsteffe »

I think that it shouldn't be very difficult to fix the topological naming for what concerns the fillets/chamfers (making or deleting fillets/chamfers).

In fact in a normal (non pathological) case a new fillet introduces new (curved) faces but doesn't remove any preexisting face.
The two faces connected by a filletted edge get shrinked a little bit but are still there.

In the implementatoin the two faces are cancelled and replaced by two smaller faces and I think that, in order to fix toponaming algorithm, it would be sufficient to reuse for these new faces the names of the original faces. In that way a face attached to a sketch will never change after the creation or the removal of a fillet.
C_h_o_p_i_n
Posts: 225
Joined: Fri Apr 26, 2019 3:14 pm

Re: Topological Naming, My Take

Post by C_h_o_p_i_n »

@Zolko

... imho it does not make any difference how an daisy chained list is built - wether intrinsic while generating Names when objects come into life nor using the TNP-Solution by RT ... if too many chain-elements inbetween are deleted - no algorythm can guess where the orpahned leaves/twings should be attached.

So i think it's quite normal that a manually reatachment of any orhphaned leaves/twigs might sometime be neccessary.

regards,
Stefan
logari81
Posts: 658
Joined: Mon Jun 14, 2010 6:00 pm

Re: Topological Naming, My Take

Post by logari81 »

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.
User avatar
-alex-
Veteran
Posts: 1853
Joined: Wed Feb 13, 2019 9:42 pm
Location: France

Re: Topological Naming, My Take

Post by -alex- »

logari81 wrote: Wed Oct 05, 2022 1:53 pm I come to this thread to look for an update about the merging of realthunder's implementation.
Please see here: https://forum.freecadweb.org/viewtopic.php?f=44&t=71687
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Topological Naming, My Take

Post by adrianinsaval »

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.
User avatar
Zolko
Veteran
Posts: 2213
Joined: Mon Dec 17, 2018 10:02 am

Re: Topological Naming, My Take

Post by Zolko »

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.
try the Assembly4 workbench for FreCAD — tutorials here and here
Post Reply