@marioalexis84 can you weigh in ?

Version 4? Does this imply there have been earlier PRs or issues?
This appears to be from a first time contributor. @mac-the-bike can we get a little more context?

The PR doesn't note a known bug or issue in the tracker. Can we get some context on what problem this is solving?
Please note the contributing.md file in the source tree for a discussion of issue creation.

@sliptonic historically there have been 2 PRs before this:
#7560
#7769

Related forum thread: https://forum.freecad.org/viewtopic.php?f=18&t=70978

Note: I've fixed the headers in a separate branch luzpaz@05b3ab2

I tried pushing them to this PR without success.

PR: 11492
yes this is about version 4. the other versions seemed to have timed out.
this bunch of code doesn't correct any bug - it is a new feature
more details, i hope, in
https://forum.freecad.org/viewtopic.php?t=70978

the reason for writing this was to enable users to display parts of their mesh that were hidden by other parts.
this would enable users to look at the mesh going through the thickness of the structure to ensure that a sensible mesh density is being used.

i had a look at the windows error - my knowledge of c++ is not sufficient to correct the problem, as you have already deduced. these definitions should be in:
FemSetElementNodesObject.h

@marioalexis84 did you get a chance to review?

Aside, could a merge dev please push my changes #11492 (comment) ?

@adrianinsaval can you help here ? TIA

@marioalexis84 did you get a chance to review?

I will be able to work on this in 2 weeks

I am in the middle of changing the code in order to make it more robust - so for the moment no more checks. (I am sorry this is abrupt, it is not meant to be.)
I will place something here to say that I am satisfied with the modded code and checking can continue

@mac-the-bike did you pull in my changes ? (#11492 (comment))

luzpaz: the short answer is no.
05b3ab2 - I have copied this code and I am in the process of comparing it with my code.

I have made changes to 5 files, (FemSetElementNodesObject.h, Command.cpp, TaskCreateElementSet.cpp, TaskDlgCreateElementSet.cpp, README.md) since my latest go in November. I will incorporate your changes into these files.

The other files that you have changed can be left as they are.

I hope this is clear.

Now for some help, how do I create a branch that is derived from your branch 05b3ab2? ( I use the github GUI.)

luzpaz
"05b3ab2 - I have copied this code and I am in the process of comparing it with my code"

I have checked the stuff from what I thought was 05b... but I think I have copied the FreeCAD master.

This:
github.com/luzpaz/FreeCAD/commit/05b3ab208ae0bd28a0d76b821949eeb5218f3a4d
contains your title and version changes but my changes seem to have disappeared.

I have looked at:
main...luzpaz:FreeCAD:fix-headers - call this (1)
and these files have my changes and your changes included.

I have also noted that there are some other changes, as in Shortcuts.cfg, that I also need to include.

How do I get a copy of the code in (1)?

@mac-the-bike the diff for my branch is at https://github.com/luzpaz/FreeCAD/commit/05b3ab208ae0bd28a0d76b821949eeb5218f3a4d.diff (i just added .diff to the end of the url)

So you can easily run something like this one-liner from the CLI: (make sure you're in your branch)

curl -s https://github.com/luzpaz/FreeCAD/commit/05b3ab208ae0bd28a0d76b821949eeb5218f3a4d.diff | git apply -v

Wed 24 Jan 15:07:44 GMT 2024

luzpaz

I have reloaded my code - this includes your latest changes together with updates that have occurred since I first downloaded the code, December 21. (The changes that affect my code.)

It is in: mac-the-bike/FreeCAD, with branch: mac10

You can take this new version and use it for yourself.

The name for the changes is:
Severalth Version of the Erasing Elements Code

I hope that this is the last set of changes I am going to do(?)

Thanks

OS: Ubuntu 22.04.3 LTS (GNOME-Flashback:GNOME/gnome-flashback-metacity)
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.35434 (Git)
Build type: Unknown
Branch: main
Hash: ab386dc
Python 3.10.12, Qt 5.15.3, Coin 4.0.0, Vtk 7.1.1, OCC 7.5.1
Locale: English/United Kingdom (en_GB)

I compiled the mac10 branch on Ubuntu 22.04.3 and tested this tool. It's really cool. Can you just consider adding those two little features ?

• automatically changing the subject of this operation for subsequent uses on non-result meshes - currently, to use the tool multiple times in a row, a user has to manually select the new mesh object in the tree as the target for this tool, otherwise, the result is applied to the original mesh and thus unexpected. Such a change, if possible, would make it much easier to keep "digging" in the mesh but if it can't be done so easily then never mind.
• automatically turning on the visibility of the original mesh when the Restore button is clicked (otherwise, there's an impression that something went wrong because the mesh disappears completely)
• it would be also great to have individual element selection in addition to polygon selection but it could be added in the future

If it's pretty much ready, I think that you can switch it from Draft to Open.

@mac-the-bike just a heads up for the upcoming feature freeze:
If you want this draft PR to be considered for a version 1.0 release, it should be marked ready for review by Monday, 2024-06-03 at the latest. (further details in the 1.0 feature freeze forum announcement).

this is the conflict:

<<<<<<< erase-version-4
          << "FEM_CreateNodesSet"
          << "FEM_CreateElementsSet"
=======
          //          << "FEM_CreateNodesSet"
>>>>>>> main
          << "FEM_FEMMesh2Mesh";

This is what I want (i.e. inclusion of CreateElementsSet).
So I don't really understand the conflict.

this breaks compilation on windows with conda:

D:\a\FreeCAD\FreeCAD\src\Mod\Fem\Gui\Command.cpp(1254): warning C4273: 'elementsName': inconsistent dll linkage
D:\a\FreeCAD\FreeCAD\src\Mod/Fem/App/FemSetElementNodesObject.h(59): note: see previous definition of 'public: static std::basic_string<char,std::char_traits<char>,std::allocator<char> > Fem::FemSetElementNodesObject::elementsName'
D:\a\FreeCAD\FreeCAD\src\Mod\Fem\Gui\Command.cpp(1254): error C2491: 'Fem::FemSetElementNodesObject::elementsName': definition of dllimport static data member not allowed
D:\a\FreeCAD\FreeCAD\src\Mod\Fem\Gui\Command.cpp(1255): warning C4273: 'uniqueElementsName': inconsistent dll linkage
D:\a\FreeCAD\FreeCAD\src\Mod/Fem/App/FemSetElementNodesObject.h(60): note: see previous definition of 'public: static std::basic_string<char,std::char_traits<char>,std::allocator<char> > Fem::FemSetElementNodesObject::uniqueElementsName'
D:\a\FreeCAD\FreeCAD\src\Mod\Fem\Gui\Command.cpp(1255): error C2491: 'Fem::FemSetElementNodesObject::uniqueElementsName': definition of dllimport static data member not allowed

@wwmayer do you have an idea how this could be fixed?

They are also public member variables, which is a bit of a "code smell". I can't quite figure out what's going on with them -- why is elementsName not just a const (or better, a const method)? And why does uniqueElementsName exist at all?

do you have an idea how this could be fixed?

Will have a look later today. ATM I am busy to clean up some other messes.

do you have an idea how this could be fixed?

Will have a look later today. ATM I am busy to clean up some other messes.

I'd like to know more.

We're doing the best we can with the merge process, which is working well. IMHO, an occasional revert is expected If we're going to move fast. It's not the worst thing in the world.

That said, we should learn as much as we can from every mistake and mess. Is there anything we could have done in the merge process to avoid the mistakes?

Two years ago there were some discussions about improving the code quality but basically nothing is practised.

• Clean code is hardly ever demanded (i.e. avoid code duplication, do not break SRP, write small functions, avoid mutable global state (e.g. accessing user parameters in low-level classes is one of the most annoying things), ...)
• Compiler warnings are never criticized
• Very often linter warnings are not criticized
• Serious side-effects are rarely recognized
• Occasionally PR with failing CI tests are merged
• Sometimes PR that do not even compile are merged (like this one)
• ...

It is very frustrating to spend day after day to get the most serious issues fixed again.

Two years ago there were some discussions about improving the code quality but basically nothing is practised.

* Clean code is hardly ever demanded (i.e. avoid code duplication, do not break SRP, write small functions, avoid mutable global state (e.g. accessing user parameters in low-level classes is one of the most annoying things), ...)

* Compiler warnings are never criticized

* Very often linter warnings are not criticized

* Serious side-effects are rarely recognized

* Occasionally PR with failing CI tests are merged

* Sometimes PR that do not even compile are merged (like this one)

* ...

It is very frustrating to spend day after day to get the most serious issues fixed again.

Indeed. This is good feedback but I'm not sure exactly how to proceed.

The maintainers review PRs every week and there are a LOT of PRs to review. Fortunately most are small.

Assuming it complies with the overall contributing policy, our procedure is to look for a reason to reject it:

1. The discussion in the PR is ongoing with someone asking for change - make a comment and/or move to draft
2. It fails CI (real failure) - comment and ask submitter to fix it.
3. merge conflicts - comment and ask submitter to rebase.
4. we don't feel collectively qualified to approve. Ask one or more experts to code review.
5. Touches the UI - Ask DWG to approve.

If it passes those five filters, we move ahead.

We're very happy to push back on code quality. When we see questionable code, we ask for code reviews from experts. We try not to request reviews for every little thing. Even so, lots of times we never get a reply from the reviewer. Then we're stuck. We either take the code as-is or the PR sits in limbo. When this happens, we assume that if the reviewer didn't answer, they must be ok with the code.

We never want to ignore the CI but when CI failures happen, we are again stuck. Either all PRs sit until the issue is resolved, or we assume that the CI failure is noise. That's a bad assumption but often unavoidable. The Conda failure has been happening for months. It's purely a timeout on github and has nothing to do with the code.

This particular issue is the case where we made a mistake. We assumed that the CI failure we was the same false positive that was affecting other PRs. It was, in fact, masking a real failure.

Assuming it complies with the overall contributing policy, our procedure is to look for a reason to reject it:

Do reviewers checkout a PR locally, build it and test it? At the time when I mainly did the code review I almost always did it in these steps:

• In case of a bug I tried to reproduce it
• Check if the PR includes a unit test. In most cases it didn't so I started to write one and prepared a new PR to merge it afterwards
• Checkout the PR and confirm that it fixes the issue
• When building the PR watch out for compiler and linter warnings. If found some I either reported them or fixed them in a new PR
• Do some more testing to figure out possible regressions
• A failing or breaking PR was not accepted -- unless it was easy to fix myself. If the contributor didn't react on complaints the PR started to rot.

This is a very time-consuming way of checking a PR but also a good way to avoid serious regressions.

Even so, lots of times we never get a reply from the reviewer. Then we're stuck. We either take the code as-is or the PR sits in limbo. When this happens, we assume that if the reviewer didn't answer, they must be ok with the code.

So, am I right that if a reviewer doesn't reply on requests but the CI pass then you will merge the PR? And how do know that the PR doesn't cause problems?

The Conda failure has been happening for months. It's purely a timeout on github and has nothing to do with the code.

If a PR failed because of a timeout it's fine to ignore the specific CI that caused it.

This particular issue is the case where we made a mistake. We assumed that the CI failure we was the same false positive that was affecting other PRs. It was, in fact, masking a real failure.

Hmm, but it was already realized in December 2023 that this PR fails on Windows (see #11492 (comment)). Since then the PR was not touched to fix the failure.

Do reviewers checkout a PR locally, build it and test it?
Yes, mostly. There are plenty of trivial PRs that don't get this level of scrutiny. If the PR only includes icon changes for example. Or one-liners that only change a text string.

Anything that actually fixes a bug or adds a feature gets built by somebody.

* Check if the PR includes a unit test. In most cases it didn't so I started to write one and prepared a new PR to merge it afterwards

This is definitely an area where we can improve. I'm not suggesting that the maintainers/reviewers should be writing the unit tests but that we should insist on more unit tests from contributors. It would help to have guidelines for when unit tests are required.

* When building the PR watch out for compiler and linter warnings. If found some I either reported them or fixed them in a  new PR

It will also help to get the rest of the application under recommit. CAM still isn't but will be soon.

This is a very time-consuming way of checking a PR but also a good way to avoid serious regressions.

You are willing to take on more personally with the PRs than most maintainers. Writing those additional unit tests, fixing linter problems, fixing small regressions is going above and beyond the call of duty. I think we should have a high (but clear) standard for acceptance and it should be the contributors responsibility to meet it.

Even so, lots of times we never get a reply from the reviewer. Then we're stuck. We either take the code as-is or the PR sits in limbo. When this happens, we assume that if the reviewer didn't answer, they must be ok with the code.

So, am I right that if a reviewer doesn't reply on requests but the CI pass then you will merge the PR? And how do know that the PR doesn't cause problems?

Most of the time somebody has built the PR and tried things. Obviously it's impossible to guarantee that it won't cause some other problem. Reviewers can only do their best and unit test coverage isn't complete. If we're only going to merge code that we're 100% confident of, we're going to move much much slower. That will be frustrating to both users and contributors. A good question is: How much risk are we willing to accept?

The Conda failure has been happening for months. It's purely a timeout on github and has nothing to do with the code.

If a PR failed because of a timeout it's fine to ignore the specific CI that caused it.

Ok, but that's a form of risk-tolerance. The timeout failure could still be masking a deeper failure.

This particular issue is the case where we made a mistake. We assumed that the CI failure we was the same false positive that was affecting other PRs. It was, in fact, masking a real failure.

Hmm, but it was already realized in December 2023 that this PR fails on Windows (see #11492 (comment)). Since then the PR was not touched to fix the failure.

Right. Exactly. In retrospect, this one is pure human failure. Not any one person but collectively. We had 6 or 8 people in the meeting. We had reviewed something like 40 PRs already. There was discussion that indicated it was ready to go and a good feature. We collectively looked at it and made a mistake. The lesson I learned from this one is this heuristic: The older a PR is, the higher the level of scrutiny should be.

Considering the number of PRs reviewed every week, the process works. The more people attend the merge meetings the better. Not only do we get a lot of stuff done, but we're learning from each other how to scrutinize things, how to push back on contributors, and we're improving our processes.

Mistakes will still happen and we should do this kind of post-mortem every time.

There are some concrete steps that we can take to address some of these failures:

1. I've adjusted my local compilation settings to change some of the warnings that @wwmayer has flagged as errors. I get a LOT of compiler warnings on the platforms that I compile on, a lot of them in 3rd-party code, so this will help me to isolate the ones that are actually important. Note that OndselSolver is a major offender here: there are several PRs in the OndselSolver queue that resolve these compiler warnings, I have to hand-apply those fixes right now to get a successful compilation with my current clang flags (-DCMAKE_CXX_FLAGS="-Werror=c++11-compat-deprecated-writable-strings -Werror=uninitialized -Werror=unused-parameter -Werror=unused-variable -Werror=nonnull -Werror=overlength-strings -Werror=strncat-size -Werror=strncat-size -Wno-deprecated-declarations"). Once Ondsel resolves those, we could consider making those flags the defaults.
2. We must double check all CI failures -- the MacOS Conda thing is supposed to be rare now, so the vast majority of CI failures represent real problems. We can't just assume that a failure that we are seeing is "the same old thing" (otherwise what is the point of the CI in the first place).
3. We need CI that uses precompiled headers -- as I'm sure @PaddleStroke will attest, it is quite common for us to break MSVC compilation because no one one the team is using MSVC as their normal compiler toolchain. PCH problems are a v very common failure for us.
4. We probably ought to cap the PR Review Meeting length at one hour. Longer than that and fatigue has set in: we aren't doing our best work then, and everyone needs a break. If one hour isn't enough right now (because of the upcoming release) then we need a second meeting.
5. We need to find a way to better-leverage the talent of the people attending the review meeting. Right now there's still a fair bit of nodding and rubber-stamping. These are talented developers -- let's try to find a way to better involve the non-Maintainers in the process. Maybe farm out like "Someone please look at the CI output at make sure the new code isn't introducing more linter errors", "Someone compile this on platform X and verify that no new compiler warnings are introduced", "If this fixes a bug, someone determine if it's a testable bug, and ask the dev to write a test if it is".

I've adjusted my local compilation settings

It's basically not our task to fix warnings of 3rd party libraries. This is up to their maintainers. To handle FreeCAD related warnings I've recently added a CMake option FREECAD_WARN_ERROR (only gcc and clang) that stops with an error if a warning is raised -- but only for FreeCAD code.

However, it's not recommended to make this on by default or activate it for the CI builds because for newer compilers we will get many warnings that we cannot fix (e.g. OCC code causes many deprecation warnings due to a deprecated iterator declaration).

We must double check all CI failures

Fully agree with you.

We need CI that uses precompiled headers

It's nice to have but I don't consider it an urgent issue. Failures with missing headers is something we get from time to time due to the huge amount of library versions. Usually such failures are found and fixed very quickly.

In terms of clean code there is this nice little overview of one of Uncle Bob's book: https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29

thanks everybody for all your help.
Now...Adrián Insaurralde Avalos:

Problems with elementsName and uniqueElementsName

These appear in both:
./src/Mod/Fem/App and ./src/Mod/Fem/Gui directories which may be the cause of the "inconsistent dll linkage" (this is based on not much evidence.)

Solution:
remove the two entries in FemSetElementNodesObject.h. as these variables only appear in this file.

My only excuse for this situation is that I compiled in Ubuntu 22.04 with no problems.

@mac-the-bike I documented this tool on the wiki (https://wiki.freecad.org/FEM_CreateElementsSet) but I have one doubt - what is the purpose of the Copy button? How is it supposed to work and when can it be used?

FEA-eng,

Thanks for doing this work - I wouldn't know where to start.

COPY - this is explained in the user's guide - I hope it is an explanation.

I am attaching 3 files:

1. user's guide
   You can use as much of this as you like.
   Note that the last few lines contain an additional bit of help for the "Mesh-> FEM mesh to mesh" option; it would be good if these lines could be added to the original help for this option.

2. first picture: full mesh

3. second picture: erased mesh
   Again, use as much as you want.

ALSO,
Your last comment about selecting individual elements - I will look at it but it unlikely that I will be able to implement anything as it is completely out of my experience.

user_guide.txt