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.
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
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
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.
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).
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).
load attached file in toponaming.
Do the (required) refresh to regenerate the element map
edit "Sketch001"
select the vertical (external) edge (red arrow in the screenshot)
2022-09-09_11-28.png (15.84 KiB) Viewed 15071 times
select the lower point of the construction line (at the left end of the "5 mm" measurement)
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:
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.