[Ongoing] Testing Toponaming in upstream FreeCAD

Report observations made with the new Toponaming branch.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by adrianinsaval »

chrisb wrote: Wed Sep 07, 2022 5:09 pm does this mean that the random numbering isn't changed and only the mapping is different?
from https://github.com/FreeCAD/FreeCAD/pull/7427
realthunder wrote:The purpose of the new naming algorithm is to improve robustness of sub-element references used by features in later modeling steps. To maximize source code backward compatibility, the old index based element names (e.g. Vertex3, Face2) are still accepted as sub-element references. The new Topo Naming is used internally to track changes of the element index and auto correct the index based element names in the reference.
mfro wrote: Wed Sep 07, 2022 5:24 pm My understanding (although not completely sure) is that the current state (with two PRs committed) generates and saves the information required to recover from TNP, but doesn't use it (yet). So the results appear (although not tested in every detail) just as wrong (or "compatible") as before.
That was the first PR, second PR is supposed to use it but just in Part WB. But even if used by all implemented WBs compatibility would not be an issue, this is demonstrated by the fact that files from realthunder's fork are compatible with master as long as you don't use other features that are only available in his fork, with all the stuff he added this is hard to do but at least the toponaming stuff definitively doesn't break compatibility.
User avatar
obelisk79
Veteran
Posts: 1061
Joined: Thu Sep 24, 2020 9:01 pm

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by obelisk79 »

Reading the text of the two merged PR's so far would indicate that:
- PR #1 essentially just adds the necessary API for the TNP algorithm itself.
- PR #2 integrates the TNP API functionally into the Part workbench only.
- PR #3 will integrate API calls into sketcher
- PR #4 will integrate API calls into Part Design
lamikr
Posts: 14
Joined: Fri Jul 08, 2022 11:44 pm

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by lamikr »

I did some random review of some of the patches by checking some of the ones in development/toponaming branch showed by command
git log --oneline 83d0fa6730cf09c921a639d438d7ad821d26e94c...5db5fc3886c4b4038d2203c39ef039e71054bd32

I think the code quality/readability is very good and there is well written commit descriptions and comments on code. Couple of minor things came to my mind however.

1) There are couple of "fix-typo" patches like the "db858e1f0", "b06c0cc" and "28ca0c". Maybe original patches could be improved in that way that the content of these typo-patches would already be in them? (Like some but not all of the typos fixed by db85 are introduced earlier in patch e5beb5a, so some of the content of that typo patch could already be on e5beb5a patch)

2) This is probably bigger than toponaming patch issue but to my understanding the freecad does not have any standard for line endings to force c-files to use for example to use unix line endings? (This could be forced on git-server for example either to reject or convert the line ending to favored one automaticlly but then patch following would needed to be cleared again from the line-ending changes)

Realthunder seems to use Windows line endings and this causes that there are sometimes (not often, I checked quite many patches) changes that mostly just end the line endings on many rows from unix to windows. For example patch 0afcf099df has also real changes but they kind of get hidden by changes that are just changing the line-ending as can be shown by using command

git show 0afcf099df

which shows changes like

- explicit operator bool() const
- {
+ explicit operator bool() const^M
+ {
User avatar
mfro
Posts: 663
Joined: Sat Sep 23, 2017 8:15 am

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by mfro »

lamikr wrote: Thu Sep 08, 2022 12:50 am...
2) This is probably bigger than toponaming patch issue but to my understanding the freecad does not have any standard for line endings to force c-files to use for example to use unix line endings? (This could be forced on git-server for example either to reject or convert the line ending to favored one automaticlly but then patch following would needed to be cleared again from the line-ending changes)
...
While I fully agree that these longstanding line ending issues should be attacked with priority, I'm at the opinion this should happen outside the toponaming discussion.

For @realthunder, this discussion must sound like "while you're fixing our little toponaming issue, couldn't you just fix that line ending thing as well while you're at it?". I don't think that's appropriate.
Cheers,
Markus
User avatar
mfro
Posts: 663
Joined: Sat Sep 23, 2017 8:15 am

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by mfro »

adrianinsaval wrote: Wed Sep 07, 2022 5:53 pm
mfro wrote: Wed Sep 07, 2022 5:24 pm My understanding (although not completely sure) is that the current state (with two PRs committed) generates and saves the information required to recover from TNP, but doesn't use it (yet). So the results appear (although not tested in every detail) just as wrong (or "compatible") as before.
That was the first PR, second PR is supposed to use it but just in Part WB. But even if used by all implemented WBs compatibility would not be an issue, this is demonstrated by the fact that files from realthunder's fork are compatible with master as long as you don't use other features that are only available in his fork, with all the stuff he added this is hard to do but at least the toponaming stuff definitively doesn't break compatibility.
Yes, you are right, my apologies. I read that, but didn't do much testing in Part WB and forgot as I consider myself not experienced enough with Part WB (I'm using it only if unavoidable).
Cheers,
Markus
User avatar
mfro
Posts: 663
Joined: Sat Sep 23, 2017 8:15 am

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by mfro »

I think there is something wrong with referencing external (Part) geometry from Sketcher. I'm not sure if this is an uncatched bug or just expected behaviour caused by the intermediate state of the pull requests (if the latter, just disregard).
  1. load attached file in toponaming.
  2. Do the (required) refresh to regenerate the element map
  3. edit "Sketch001"
  4. select the vertical (external) edge (red arrow in the screenshot)
    2022-09-09_11-28.png
    2022-09-09_11-28.png (15.84 KiB) Viewed 15063 times
  5. select the lower point of the construction line (at the left end of the "5 mm" measurement)
  6. select Sketcher_ConstrainPointOnObject

Code: Select all

09:46:49  Traceback (most recent call last):
  File "<string>", line 1, in <module>
<class 'IndexError'>: Constraint has invalid indexes
09:46:49  App.getDocument('L_Profile').getObject('Sketch001').addConstraint(Sketcher.Constraint('PointOnObject',3,1,-13)) 
If you replace (6) with Sketcher_ConstrainVertical you'll be granted with a segmentation fault.

Neither the traceback nor the segmentation fault happen in the latest AppImage.
Attachments
L-Profile.FCStd
(16.87 KiB) Downloaded 68 times
Cheers,
Markus
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by wsteffe »

On LinkDaily (which includes all toponaming stuff plus many other things) the ConstrainPointOnObject works without problems.
Attachments
L-Profile.png
L-Profile.png (279.42 KiB) Viewed 14990 times
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by adrianinsaval »

mfro wrote: Fri Sep 09, 2022 9:33 am

Code: Select all

09:46:49  Traceback (most recent call last):
  File "<string>", line 1, in <module>
<class 'IndexError'>: Constraint has invalid indexes
09:46:49  App.getDocument('L_Profile').getObject('Sketch001').addConstraint(Sketcher.Constraint('PointOnObject',3,1,-13)) 
Confirmed, also I'm not getting a highlight color when the external geometry is selected

Code: Select all

OS: Linux 5.19.7-arch1-1 (KDE/plasma)
Word size of FreeCAD: 64-bit
Version: 0.21.29382 +43 (Git)
Build type: None
Branch: development/toponaming
Hash: 5db5fc3886c4b4038d2203c39ef039e71054bd32
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) ]
Installed mods: 
  * fasteners 0.4.6
  * Assembly3 0.11.3
  * CfdOF 1.17.9
  * Plot 2022.4.17
  * Curves 0.5.8
  * DynamicData 2.46.0
  * Dracula 0.0.2
  * 3DfindIT 1.2.0
  * SelectorToolbar
  * FC_SU
  * A2plus 0.4.59
  * AirPlaneDesign
  * Glass
  * Assembly4 0.12.4
  * CurvedShapes 1.0.4
  * CubeMenu
  * sheetmetal 0.2.57
  * CfdOF.backup1662832266.245604
@realthunder are you aware of this?
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by adrianinsaval »

another bug found in today's snap build, part extrude with taper angle works incorrectly (nested profiles are filled), here a comparison in master and in toponaming branch:
comparison.png
comparison.png (24.52 KiB) Viewed 14758 times

Code: Select all

OS: Ubuntu Core 20 (KDE/plasma)
Word size of FreeCAD: 64-bit
Version: 0.21.29382 +53 (Git) Snap 289
Build type: Release
Branch: development/toponaming
Hash: f24c64c96c8a1a6ac8183c715be25e2122376af2
Python 3.8.10, Qt 5.15.5, Coin 4.0.0, Vtk 7.1.1, OCC 7.6.3
Locale: English/United Kingdom (en_GB)
don't know if this is just a bug in the snap build or of the branch
Attachments
tpn_test.FCStd
(14.59 KiB) Downloaded 63 times
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: [Ongoing] Testing Toponaming in upstream FreeCAD

Post by wsteffe »

Regarding the L-Profile constraint, I think that now, after merging of toponaming3 which provides support for sketch, the problem is solved also in branch development/toponaming.

Following image is made with last snap of this branch.
L-Profile.png
L-Profile.png (162.97 KiB) Viewed 14351 times
Post Reply