Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Netgen submodule #3793

Open
wants to merge 44 commits into
base: devel
Choose a base branch
from
Open

Netgen submodule #3793

wants to merge 44 commits into from

Conversation

roystgnr
Copy link
Member

@roystgnr roystgnr commented Mar 2, 2024

This isn't in working order (I'm sure it'll fail installcheck, most of the features aren't there...) but it's passing the most basic test now and probably good enough to let others see it and to make sure I haven't regressed anything in other tests.

@moosebuild
Copy link

moosebuild commented Mar 2, 2024

Job Coverage on e970927 wanted to post the following:

Coverage

ae8cf3 #3793 e97092
Total Total +/- New
Rate 62.73% 62.81% +0.09% 86.31%
Hits 69384 69729 +345 372
Misses 41232 41282 +50 59

Diff coverage report

Full coverage report

Warnings

  • New new line coverage rate 86.31% is less than the suggested 90.0%

This comment will be updated on new commits.

@roystgnr
Copy link
Member Author

roystgnr commented Mar 4, 2024

Well, at least I didn't break installcheck in the default --disable-netgen configuration; that's a plus. Let me try enabling netgen by default and make sure CI catches the issues then.

@roystgnr
Copy link
Member Author

Okay, now we're seeing the real issues; enough to prompt cleanup of the cmake options and a stray clang warning, at least. And I think a proper manual install (with patchelf on Linux, install_name_tool on OSX, thankfully this was already figured out in moose.mk at one point...) will fix the remaining issues.

Right now this is just a test for include directory and linker issues,
but it'll also be necessary when we're actually using netgen...
This doesn't really do anything but create and delete an empty mesh,
but it helped me work out more linker and namespace issues, and it'll be
a good place to put tests for any issues that we suspect aren't in our
shim code.
We need a lot more here but this is a start
Fortunately recent clang versions flag this with a warning and our CI
for them treats warnings as errors.

Let's mark the newly-virtual functions "override" in the subclasses
while we're at it.
In the medium term I'm hoping this will be robust enough to leave on by
default; in the short term I want our CI to yell at me about any errors
this triggers.
We don't use it yet, we don't have plans to use it in the near future,
and if cmake thinks we want it but can't find it then it refuses to
build the parts of netgen we do use.
We don't need it, and it demands Tcl/Tk which we may not have and may
not want to download.
Our Civet recipe was complaining about the lack of pybind11, but I've
seen other weird Python errors here too.
This lets us (re)tetrahedralize volume meshes by turning their
(outermost) surface into a surface manifold fitting our existing API.

We'll still need to tweak this to save interior points as Steiner points
for compatibility with our 2D triangulator behavior.
Why did I not have this API take a reference???

Ought to fix that and deprecate the existing one...
And get rid of an annoying unimplemented declaration (we have the
functionality under another name though) while we're at it.
This isn't going to be as useful as I'd feared, if we can't support
no-refinement cases, but it's done and it's working for auto-refinement
cases.
This fixes our header check; we can't get away with a forward
declaration if we're asked to generate an UnstructuredMesh destructor.
I don't think the move here should require unstructured_mesh.h but my
compiler disagrees.
We need a bit of a trick here to make sure the autogenerated headers
only get generated once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants