[Open] Topological Naming code

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!
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Re: Topological Naming code

Post by jrheinlaender »

I had the idea of doing a "simple but imperfect" implementation of topological naming. The idea being that it can be reviewed and tested a lot faster so we can handle at least most of the "jumping sketch" and "lost edge reference for fillet" problems as soon as possible.

Its here:

https://github.com/jrheinlaender/FreeCA ... naminglite

The goal was to keep the code as simple as possible and the changes to PartDesign Feature code as small as possible. The trade-off being that not all situations are handled. I estimate that 50-80% of the "jumping sketch" etc. problems will be caught by this code.

Let me know what you think. The place to start looking at the code is in Part/App/PartFeature.h
wmayer
Founder
Posts: 20244
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Topological Naming code

Post by wmayer »

Very interesting! Is this already tested for sketches to fix this jumping problem? Do you have examples where it fails? Then what is the reason to start another naming implementation? AFAIK you've already implemented the naming stuff in a separate branch. Are there any attempts to merge both or what are your plans?

Programming stuff:

Code: Select all

/// Clear the data structure created by extractSubShapes() 
void clearSubShapes(std::map<TopAbs_ShapeEnum, TopTools_IndexedMapOfShape*> map) 
{ 
    for (int t = 0; t < numShapeTypes; t++) 
        delete map[shapeTypes[t]]; 
}
This produces some dangling pointers because the keys are still there. And in the case a key is not present in the map you create it this way.

Code: Select all

/// Clear the data structure created by extractSubShapes() 
void clearSubShapes(std::map<TopAbs_ShapeEnum, TopTools_IndexedMapOfShape*> map) 
{ 
    for (int t = 0; t < numShapeTypes; t++) {
        std::map<TopAbs_ShapeEnum, TopTools_IndexedMapOfShape*> map:: iterator it;
        it = map.find(shapeTypes[t]);
        if (it != map.end()) {
            delete it->second;
            it->second = 0;
        }
    }
}
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Re: Topological Naming code

Post by jrheinlaender »

Then what is the reason to start another naming implementation? AFAIK you've already implemented the naming stuff in a separate branch.
The other branch is highly complex and will therefore take a long time to find its way into master (its already about a year old!!) Also, it only handles the history building at this moment. How to actually adjust properties like the sketch face using the history is still totally unclear (according to jriegel).
So the idea was to have a simple implementation that handles most cases and which (hopefully) can be merged quickly.
Are there any attempts to merge both or what are your plans?
No, because two totally different methods are being used. Exchanging one naming implementation for another later on should be no problem as long work correctly, since naming should happen in the background, "unseen" by the user, anyway.
Is this already tested for sketches to fix this jumping problem? Do you have examples where it fails?
As I said, it works "most of the time". In the code I have documented the cases that are not covered (e.g. the end shape of BRepFeat_MakePrism). There are plenty of cases where it fails, but in that case the code simply doesn't alter anything so we are no worse off than before.

So, my reasons for doing this are:
1. Some naming is better than no naming at all
2. The changes to the Feature::execute() methods are minimal
3. For the cases when this method works, the user will be happy. For the cases that are not handled the user won't be any more upset than he is already
4. It was simple and quick to implement :-)

Thanks for pointing out the error in the code!
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Re: Topological Naming code

Post by jrheinlaender »

This produces some dangling pointers because the keys are still there. And in the case a key is not present in the map you create it this way.
I thought about this again. The dangling pointers don't worry me because the map will run out of scope immediately afterwards everywhere that it is cleared in this way. And I am sure all the keys are in the map, otherwise there would have been a crash much earlier on. So isn't this the simplest way of freeing the pointer memory?
wmayer
Founder
Posts: 20244
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Topological Naming code

Post by wmayer »

This is true as long as the function is used exactly this way. But what if someone else later uses this function and doesn't know about this? After all the function causes undefined behaviour in certain situations and this is something that should be avoided. Furthermore, the fixed version is not that much slower so that it does no harm.
User avatar
jriegel
Founder
Posts: 3369
Joined: Sun Feb 15, 2009 5:29 pm
Location: Ulm, Germany
Contact:

Re: [Open] Topological Naming code

Post by jriegel »

I still leave that open...
Stop whining - start coding!
Post Reply