Add support for EGADS geometry system #330
Conversation
…EGADS API to implement functions.
…GADS source to compute number of nodes, edges, faces, or regions in a model. Hopefully those additions to EGADS will make it into the a of EGADS.
…ompile pumi. Might need to move gmi_egads.x to a new folder similar to gmi_sim. Also will need to re-write gmi_egads_load to use EGADSlite if installed with it.
…y don't work, fails to find the EGADS libraries and header files.
… mesh genrated doesn't associate mesh entities with model entities, for 3D with multiple regions EGADS doesn't create a unique index for the regions so mesh elements aren't classified on a model region. Need to sort that out.
…much further review, but code seems to run and produces a verified mesh now.
… smb. Need to check if the smb is actually correct though
…er based on closed point evaluations
…ut if it is a 2D or 3D mesh
My comment was getting at that if an application needed EGADS it would need it because somewhere it would do more than just query the geometry (it would either creating or modifying it, which would be more expensive than a query). I was also thinking about the EGADSlite paper I linked above that shows that the EGADSlite functions are thread safe and scalable, and are in general much faster than the EGADS alternatives (Table 2 from that paper).
Yes, that is the current state of the code. Based on the flag |
CMakeLists.txt
Outdated
| option(ENABLE_EGADS "Build with EGADS support" OFF) | ||
| message(STATUS "ENABLE_EGADS: ${ENABLE_EGADS}") | ||
| if(ENABLE_EGADS) | ||
| add_definitions(-DHAVE_EGADS) |
There was a problem hiding this comment.
To add compiler definitions you should be target_compile_definitions. Since you use this option in the header it will need to be public. Because this leaks into the user's downstream code you should use a prefixed name. I.e. PUMI_HAVE_EGADS
There was a problem hiding this comment.
Thanks for taking another look at this @jacobmerson. I wrote the -DHAVE_EGADS definition to be consistent with the simmetrix definition:
Lines 97 to 99 in 30a332d
Based on the way that
HAVE_SIMMETRIX is used, it seems convenient to use add_definitions so that it applies to all the executables so that multiple calls to target_compile_definitions can be skipped, but I am aware that this approach is considered bad practice in modern cmake. In any case I thought it would be best to stay consistent, though I'm not tied to the current approach.
There was a problem hiding this comment.
I would prefer that all of those other HAVE_XXX definitions get prefixed eventually and you have a valid point about consistency. The problem with adding compile definitions without a namespace is that any other consuming project linking against pumi_egads has that compile definition on their target. So, what if a consumer wants to use PUMI compiled with egads but they have conditional code behind HAVE_EGADS that they don't want to compile for some reason...currently they cannot do that since compile definitions are infectious. I think @cwsmith has last word on this sort of thing since he has to deal with all the xsdk people/rules.
The issue with the non-target version of add_definitions (especially in the top level cmake file) is that it will be applied to every single target and library at the current or lower sub-directory level. So, even random libraries that have nothing to do with egads are getting that compile definition added. Since these compile definitions will be infectious to anyone who uses any SCOREC target it has the potential to cause issues for down stream users.
There was a problem hiding this comment.
@cwsmith do you have any thoughts on the concerns raised by @jacobmerson?
There was a problem hiding this comment.
@jacobmerson is correct. We should have all the *HAVE*,*ENABLE*, etc... variables prefixed with PUMI_ to avoid conflicts with packages that use PUMI. +1 for using target_compile_definitions as well.
I created an issue to clean this up in develop after this is merged. #337
…d use that to compile/link against it. Fix bug in 2D Ugrid reader that classified some vertices wrong. Allow loading 2D egads models without needing a supplemental adjacency file
There was a problem hiding this comment.
Thank you @tuckerbabcock. I'm glad to see that this is still active. It is a good addition to PUMI and I'd like to merge it soon.
Would you mind writing up something on the https://github.com/SCOREC/core/wiki/Mesh-Generation wiki page to discuss, or provide a reference to, how EGADS models and meshes can be used?
Besides the CMake changes discussed above, are there any other changes you intend to make? If not, I think we should merge this and resolve the CMake changes within the scope of a larger PUMI CMake refactor. @jacobmerson Does that make sense to you?
| @@ -0,0 +1,256 @@ | |||
| #[=======================================================================[.rst: | |||
| FindESP | |||
| --------- | |||
There was a problem hiding this comment.
@tuckerbabcock is this something you wrote? The style of the commenting looks different which indicates it might come from elsewhere. If so, we need to include the source/licensing information.
There was a problem hiding this comment.
@jacobmerson you're right, I should have put some attribution in this file. I "wrote" it, by looking at CMake's built in "FindBoost.cmake" file (https://github.com/Kitware/CMake/blob/master/Modules/FindBoost.cmake) and used that as a template
|
@cwsmith which cmake changes are you thinking we should push off vs include with this pull request? |
I think we can skip the changes to variable names to include the PUMI prefix for now. The |
|
Hi @cwsmith: did you mean to assign this issue to me? Did you mean @tuckerbabcock? Tucker is our expert on all things EGADS. |
@jehicken Not intentionally. Sorry about that. |
|
Thank you @cwsmith for the renewed interest in this PR.
I'd be happy to write something like that up, but right now the mesh generation stuff isn't really polished at all. You have to use the ESP "analysis interface module" (AIM) I wrote for PUMI, with a slightly modified version of the ESP distribution (which I've been working on here: https://github.com/tuckerbabcock/EngSketchPad). Once you've got your mesh, you can use the gmi_egads interface I've written here with any ESP distribution. Long term, the mesh generation capabilities should not require using the modified ESP distribution (the AIM should be able to exist as a stand-alone plug-in), but I've set it up the way it is currently to make my life a little easier.
No, I think the gmi_egads interface is mostly feature complete as is right now (for everything I've needed to use it for anyway). Of course I occasionally find bugs while working on it, but those can easily be submitted as their own PRs in the future. |
gmi_egads/CMakeLists.txt
Outdated
| # this file brings PUMI_USE_EGADSLITE from CMake to C++ | ||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/gmi_egads_config.h.in" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/gmi_egads_config.h") |
There was a problem hiding this comment.
@jacobmerson this config file only defines PUMI_USE_EGADSLITE. Is conditionally calling target_compile_definitions a better approach here? It seems like it would be to me.
There was a problem hiding this comment.
target_compile_definitions would be used instead of https://github.com/tuckerbabcock/core/blob/afb21d570ad03e75c346b4a45b714a0be1dbb815/CMakeLists.txt#L103
An example of its use is here:
Line 159 in 58a8832
There was a problem hiding this comment.
@cwsmith would you prefer the target be the interface target core, or just the gmi_egads target? I think the gmi_egads target would be more explicit and therefore preferable, but I don't have strong thoughts.
For my above thought on having PUMI_USE_EGADSLITE be set with target_compile_definitions, I was imagining it would be set as a PRIVATE property of gmi_egads, since I think that would be semantically equivalent to the current setup with the config file.
There was a problem hiding this comment.
I spent some more time looking at the code and have a hopefully more complete response.
IIUC, the user needs to know if PUMI was installed with EGADS and/or EGADSLite support and gmi_egads/gmi_egads.c needs to know if EGADSLite is enabled (gmi_egads.c is only compiled if egads is enabled). If that is correct, I think the best option is to use target_compile_definitions(...) for both variables and remove the config header. We would also want to replace any use of HAVE_EGADS[LITE] with the PUMI_HAS_EGADS[LITE] variable (or equivalent).
To answer your questions:
- target: Try setting it on
gmi_egads, but that may cause problems compiling tests that use the preprocessor variables when gmi_egads is not available/installed. - visibility: If the first assumption was correct, then I think they should both have
INTERFACEscoping/visibility.
There was a problem hiding this comment.
@cwsmith I've switched over all the EGADS stuff to use target_compile_definitions, and renamed the flags from HAVE_EGADS to PUMI_HAS_EGADS. PUMI_USE_EGADSLITE is a private definition on gmi_egads since it is only needed in gmi_egads.c, while PUMI_HAS_EGADS is an interface definition on gmi_egads since its needed by people who use the library. I did not experience any issues compiling and running the tests that use these variables when gmi_egads wasn't built, so I think it should be okay.
gmi_egads/pkg_tribits.cmake
Outdated
| tribits_package(SCORECgmi_egads) | ||
|
|
||
| # these ENABLE vars are made by tribits based on ./cmake/Dependencies.cmake | ||
| option(EGADS_LITE "Enable EGADSlite" OFF) | ||
| # this file brings EGADS_LITE from CMake to C++ | ||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/gmi_egads_config.h.in" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/gmi_egads_config.h") | ||
| # make sure the compiler can find the above header | ||
| include_directories(${CMAKE_CURRENT_BINARY_DIR}) | ||
|
|
||
| include_directories(${CMAKE_CURRENT_SOURCE_DIR}) | ||
|
|
||
| #Sources & Headers | ||
| set(SOURCES gmi_egads.c) | ||
| set(HEADERS gmi_egads.h) | ||
|
|
||
| #Library | ||
| tribits_add_library( | ||
| gmi_egads | ||
| HEADERS ${HEADERS} | ||
| SOURCES ${SOURCES}) | ||
|
|
||
| tribits_package_postprocess() |
There was a problem hiding this comment.
I definitely don't understand what tribits is and I think I mostly copied this file from a different one in PUMI, so I don't have a clue if this is wrong.
There was a problem hiding this comment.
Tribits is the cmake extension used by Trilinos. When we add a new code this wrapper is needed.
There was a problem hiding this comment.
@cwsmith how does Tribits interact with target_compile_definitions set in the regular CMakeLists.txt file? Should they be the same in this file?
…VE_EGADS (renamed PUMI_HAS_EGADS) and for PUMI_USE_EGADSLITE instead of using polluting definitions and a config file. Also removed outdated FindEGADS.cmake file in favor of new FindESP.cmake file. Deleted older debug code that stuck around in various files.
|
@tuckerbabcock do you have instructions for how to install/run the other necessary components? Does EGADS always require ESP? Unless I had read the comments on this thread I would have no idea what ESP is. Is the only test of this functionality currently the "split" test? @cwsmith W.R.T. testing of the geometric kernels are there any canonical tests that should be performed? |
I don't have any specific instructions, other than what's in the readme for how to build ESP: https://acdl.mit.edu/ESP/. ESP (The Engineering Sketch Pad) contains EGADS among other components. One would typically use the ESP interface to build an EGADS model. EGADS is to parasolid what ESP is to NX.
I also added a "verify" test, but you're right, there should be more. @cwsmith please let me know if there is some list of canonical tests I should add. |
This PR adds support for using the EGADS geometry system (acdl.mit.edu/ESP).
I've previously spoken to @cwsmith and @mortezah about the implementation, which is now fairly complete. Since this is my first fairly large PR to core, please let me know what else I should add to get this merged.