PR #4752 Topological Naming

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: PR #4752 Topological Naming

Post by wsteffe »

adrianinsaval wrote: Wed Feb 09, 2022 6:20 pm submit a PR if it's so easy to do..
Before making a PR we should agree, in example, on a design flow in which it is possible to pad multiple solids from disjoint closed wires placed in a single sketch. Unfortunately many FC developers do not agree on this idea and would surely reject any PR.
If and when I had some spare time to work on the FC code, I would better try to do it in the RT branch which I am using more regularly.
In example I would like to improve (because I need it) the Filling Feature of Surface WB (see https://github.com/realthunder/FreeCAD/issues/248).

IMO the biggest problem of FC project is that it the missing of a project leader with a clear vision on the project goals and of what has to be done to achieve them. Unfortunately I do not see anything like that. No world is coming even to clarify how the project should deal with the TN problem which has been debated several times in many threads.
User avatar
onekk
Veteran
Posts: 6146
Joined: Sat Jan 17, 2015 7:48 am
Contact:

Re: PR #4752 Topological Naming

Post by onekk »

wsteffe wrote: Wed Feb 09, 2022 6:35 am But what do you want to test ?

It has been said several times that the current PR doesn't add any new feature but just a basic infrastructure aimed to support following TN related PRs.
So there isn't much to test apart seeing that it doesn't introduce any substantial regression. But this was already checked.

Mostly TNP will not add new features, it add consistency when dealing with some operations that will change the "reference number" of some component like a face or an edge.

Main concerns is most people are speaking about TNP and don't know what is related to, did you read:

https://wiki.freecadweb.org/Topological_naming_problem

Regards

Carlo D.
GitHub page: https://github.com/onekk/freecad-doc.
- In deep articles on FreeCAD.
- Learning how to model with scripting.
- Various other stuffs.

Blog: https://okkmkblog.wordpress.com/
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: PR #4752 Topological Naming

Post by wsteffe »

onekk wrote: Wed Feb 09, 2022 7:11 pm Mostly TNP will not add new features
I said it badly. Actually I wanted to say that current PR doesn't add anything to really address the TN problem. RT said that that the real TN stuff will come in next PRs dedicated to specific WBs (mainly Sketch and Part Design). This PR adds code lines that are not yet used but will be needed and used by code submitted in the following PRs. After having merged this PR you will not see any difference with respect to the current release.

So, again, what do you want to test ?
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: PR #4752 Topological Naming

Post by adrianinsaval »

wsteffe wrote: Wed Feb 09, 2022 7:08 pm Before making a PR we should agree, in example, on a design flow in which it is possible to pad multiple solids from disjoint closed wires placed in a single sketch. Unfortunately many FC developers do not agree on this idea and would surely reject any PR.
The developers who set that limitation and were vehemently against removing it are not active any more, there's no one currently active that would oppose it if it's done correctly AFAIK. We are off topic though so better to either leave it there or continue in a separate thread.
IMO the biggest problem of FC project is that it the missing of a project leader with a clear vision on the project goals and of what has to be done to achieve them.
partially agree, but this is also off-topic, some discussion surrounding this subject has been going on in the FreeCAD Project Association announcment thread
If and when I had some spare time to work on the FC code, I would better try to do it in the RT branch which I am using more regularly.
In example I would like to improve (because I need it) the Filling Feature of Surface WB (see https://github.com/realthunder/FreeCAD/issues/248).
Sad but understandable, this attitude but from people actually submitting PRs is what I fear is coming if the TN merge remains stalled. On the other hand, given the likely license of those changes it's always possibly to cherry pick, how likely that is to happen and how simple it would be is a different matter.
onekk wrote: Wed Feb 09, 2022 7:11 pm Mostly TNP will not add new features, it add consistency when dealing with some operations that will change the "reference number" of some component like a face or an edge.
TN algorithm and automatic fixing of broken references (and assisted fixing) are features, and big ones at that. Current PR does not have that so those features cannot be tested. According to realthunder it only adds underlying functions that those features would use. Said functions cannot be tested by just using FreeCAD either because no feature uses them.
User avatar
onekk
Veteran
Posts: 6146
Joined: Sat Jan 17, 2015 7:48 am
Contact:

Re: PR #4752 Topological Naming

Post by onekk »

Yes, but a starting point is always a starting point.

I've not the whole image in mind as it is not recent, but when the merge was started I remember that actual PR is working adding the improved reference to objects ready to be consumed by more high level functions.

if the infrastructure will not add problems next step would be merging PR that modify existing methods to use the infrastructure to mitigate the TNP.

As many things are modified a gradual approach was chosen, maybe this thing has not been put in sufficient view to avoid the actual sentiment mist users have regarding TNP.

EDIT: Sorry for typing errors.

Regards

Carlo D.
Last edited by onekk on Fri Feb 11, 2022 9:55 am, edited 1 time in total.
GitHub page: https://github.com/onekk/freecad-doc.
- In deep articles on FreeCAD.
- Learning how to model with scripting.
- Various other stuffs.

Blog: https://okkmkblog.wordpress.com/
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: PR #4752 Topological Naming

Post by adrianinsaval »

No, most of the people here have seen the statements and what the plan once was, visibility is not the problem IMO because even if every single person in the world had seen the plan it doesn't matter because it has not moved forward. The PR that should have been the quickest has taken months (with no progress), hopes that this will get in and then the rest are getting lower and lower by the hour.
User avatar
jhaand
Posts: 44
Joined: Thu Apr 01, 2021 12:22 pm
Location: Venlo, The Netherlands
Contact:

Re: PR #4752 Topological Naming

Post by jhaand »

wsteffe wrote: Wed Feb 09, 2022 7:40 pm
onekk wrote: Wed Feb 09, 2022 7:11 pm Mostly TNP will not add new features
I said it badly. Actually I wanted to say that current PR doesn't add anything to really address the TN problem. RT said that that the real TN stuff will come in next PRs dedicated to specific WBs (mainly Sketch and Part Design). This PR adds code lines that are not yet used but will be needed and used by code submitted in the following PRs. After having merged this PR you will not see any difference with respect to the current release.

So, again, what do you want to test ?
If this code only refactors some code to make place for the real TNP solution, from a high level there's not much you can do. We can do a regression test to see if nothing changes and maybe add unit tests to see if the touched functions still behave the same. It's possible to do this, but it remains a lot of work that remains under the radar.

The next PR's will make it interesting to see if the problem is fixed and what keels over during regression testing.
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: PR #4752 Topological Naming

Post by wsteffe »

jhaand wrote: Thu Feb 10, 2022 6:19 pm If this code only refactors some code to make place for the real TNP solution, from a high level there's not much you can do. We can do a regression test to see if nothing changes and maybe add unit tests to see if the touched functions still behave the same.
Yes, this is the situation. And many regression tests were already done about one year ago just after the PR has been submitted.
Nobody has found any regression and in fact the PR was approved by yorik.

This approval was not sufficient to merge. Nobody (not wmayer not yorik and not anyone else) has said what is required before merging. This precise question was made by someone in the PR thread but nobody bothered to answer to him. The PR was tagged with testers needed but nobody ever said what kind of other tests must be done.
Last edited by wsteffe on Thu Feb 10, 2022 9:16 pm, edited 1 time in total.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: PR #4752 Topological Naming

Post by adrianinsaval »

wsteffe wrote: Thu Feb 10, 2022 8:44 pm
If this code only refactors some code to make place for the real TNP solution, from a high level there's not much you can do. We can do a regression test to see if nothing changes and maybe add unit tests to see if the touched functions still behave the same.
uumm... that wasn't me... please correct the quote...
Yes, this is the situation. And many regression tests were already done about one year ago just after the PR has been submitted.
Nobody has found any regression and in fact the PR was approved by yorik.
I'm not sure if many is a correct word to describe this. There were just a few.
Nobody (not wmayer not yorik and not anyone else) has said what is required before merging.

Wrong, it was said countless times in a thousand threads. Someone with enough knowledge has to review the actual code, not just have a passing look and test the compiled software. Only wmayer is believed to have enough skill for this but he has not reviewed yet. Plus a rebase is likely required since it's been so long.
The PR was tagged with testers needed but nobody ever said what kind of other tests must be done.
Agreed.
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: PR #4752 Topological Naming

Post by wsteffe »

adrianinsaval wrote: Thu Feb 10, 2022 8:53 pm uumm... that wasn't me... please correct the quote...
I am sorry. Correction done.
adrianinsaval wrote: Thu Feb 10, 2022 8:53 pm Wrong, it was said countless times in a thousand threads.
May be I lost all these thousand of threads. But may you please find the reply to the question "What is currently hindering this and the following topo merges?" made by chrismettal the Oct 25 2021 on the github thread related to this PR. I mean a response from a project leader or at least a core developer given inside the same thread where the question was asked. ?
Post Reply