TD: replace usage of raw pointers of Vertex and Face with shared poin…

[wwmayer]

…ters.

This fixes issue 4741: Broken File After Using Landmark Dimension in TechDraw

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, we ask you to conform to the following items. Pull requests which don't satisfy all the items below might be rejected. If you are in doubt with any of the items below, don't hesitate to ask for help in the FreeCAD forum!

• Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
• In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
• Your branch is rebased on latest master git pull --rebase upstream master
• All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
• All commit messages are well-written ex: Fixes typo in Draft Move command text
• Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
• Commit messages include issue #<id> or fixes #<id> where <id> is the FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.20 Changelog Forum Thread.

---

[chennes]

Is there something in particular you want reviewed here, or just normal testing?

[wwmayer]

Is there something in particular you want reviewed here

Well, TD is not my domain and I didn't want to simply push a fix that modifies many files that others might not be happy with. That's why I wanted to hear a second opinion.

The technical aspect:
The main problem with TD is that in many cases there is no clearly defined ownership of objects so that either some objects are never deleted or other objects twice. The bug report was about the latter.

Because of this I replaced the affected raw pointers with shared pointers to avoid double deletion and fix possible memory leaks.

[freecadci]

for feature branch PR_5231. Pipeline 425424877 was triggered at 89d57d4. All CI branches and pipelines.

[wwmayer]

If nobody has any reservations I will merge it shortly.

[chennes]

I'm always a fan of smart pointers in ownership situations, so I think this is an excellent thing.

[donovaly]

The main problem with TD is that in many cases there is no clearly defined ownership of objects so that either some objects are never deleted or other objects twice.

Yes. And TD in general needs a thorough review. I once worked to fix the mess with pages and its displayed childs but I felt I could only cure symptoms, not the main causes.
In my opinion the main issues with TD are:

• TD is slow. Even with relatively simple assemblies and parts, it takes quickly ages (Wandererfan and Klaus, the maintainer of A2plus, once tried to improve the situation but both can no loner work for FC and I don't know enough about the underlying problem.)
• no other WB is recompiled so often when code in FC changes by MSVC. So there must be a lot of unnecessary dependencies.
• there are many hacks in the code Wandererfan was also aware of but never had time to look at. He told me that he took over the code from someone else and often does not understand these hacks as well.

Long story short, when you see strange code, any improvement like this PR is highly appreciated.
I can help with testing PRs since I have a loft of TD test files collected by time.
(I am by far no TD master. I mainly worked on the UI and fixed bugs. So I more or less only improved TD using the existing infrastructure.)

[wwmayer]

Yes. And TD in general needs a thorough review. I once worked to fix the mess with pages and its displayed childs but I felt I could only cure symptoms, not the main causes.

A general rework at this point is difficult and actually not needed because the basic design still looks OK to me. A good idea always is code-refactoring where big functions are split into smaller ones. This automatically reduces duplicated code and may fix inconsistencies.

TD is slow....

For this we should have a good test file that eventually could be added to the examples directory. In a second step a profiler must be used to identify the code parts that are called too often.

no other WB is recompiled so often

TD depends on many other extension modules like Measure, Part, Spreadsheet, Drawing and Import. And of course it depends on the changes of the other modules.
A re-compile may only be needed if there are changes in header files but if only source files are changed then a re-linking is sufficient.

there are many hacks in the code

I have seen many cases where inside the constructor of a class certain things are checked and if not fulfilled the further initialization is aborted and a message is printed. To avoid possible crashes the affected methods are full of checks not to call the null pointer member variables. In some cases the better alternative would have been to throw an exception in the constructor in order to clean-up the whole instance.

[wwmayer]

OK, I press the red button now :)

[donovaly]

Do you think this can be backported?

[donovaly]

TD is slow....

For this we should have a good test file that eventually could be added to the examples directory. In a second step a profiler must be used to identify the code parts that are called too often.

@kbwbe provided last year some good test cases. I searched the forum but cannot find the thread where we once discussed with @WandererFan .

no other WB is recompiled so often

TD depends on many other extension modules like Measure, Part, Spreadsheet, Drawing and Import. And of course it depends on the changes of the other modules. A re-compile may only be needed if there are changes in header files but if only source files are changed then a re-linking is sufficient.

What I mean is that when TD has to be recompiled because something changed in the code of FC, almost all TD files are recomputed. This is not the case for the other WBs.
Therefore I assume there are some unnecessary dependencies.

[edi271]

This PR causes errors in the files CommandExtensionDims.cpp and CommandExtensionPack.cpp used in PR5144.

[donovaly]

This PR causes errors in the files CommandExtensionDims.cpp and CommandExtensionPack.cpp used in PR5144.

This PR is not yet merged and since this PR here is in and changed some D basics, you must rebase your PR to current master and then adapt your code in necessary.
I know that this is additional work for you.

As another reviewers already wrote in your PR, the best is to make PRs that each change a small portion. This assures it will get reviewed quickly and then merged.