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

Construct shapes in memory and support more shapes #1436

Merged
merged 58 commits into from
Jan 14, 2025

Conversation

gunney1
Copy link
Contributor

@gunney1 gunney1 commented Oct 2, 2024

Extend interface for intersection shaping

  • This PR is an extension of the support for intersection shaping
  • It does the following:
    • Adds a number of shapes to intersection shaping, such as analytical spheres, cones, cylinders. Currently, intersection shaping supports only shapes described by tet meshes and C2C surfaces of revolution.
    • Adds interfaces for specifying the shapes in memory (instead of reading them from files)
    • Re-factors some code to reduce dependence on the MFEM mesh interface, as an initial step toward supporting Blueprint meshes. The full Blueprint support is in a companion PR.

This PR is the first of 3 related PRs. They are separated to keep the code changes to a manageable size.

  1. This PR is a first pass at supporting more shapes generating those shapes in memory. We would like to get something working asap for an application requirement. We will revisit the interface and performance by Spring 2025.
  2. The second PR is to extend support to Blueprint meshes. Support Blueprint mesh in shaping #1455. It should be reviewed AFTER this one is approved.
  3. In the first 2 PRs (including this one), I tried to minimize re-factoring and moving codes around. The third PR will be to re-factor and clean up. See issue Revisit interface design and performance in IntersectionShaper #1445.

gunney1 added 26 commits July 19, 2024 08:58
This commit adds test code for testing in-memory shape construction
and shaping with those objects.
Fix by changing the pointer references to shared_ptr references.
This bug was recently created when factoring out the generation
of the mesh so it can be saved outside the scope it was generated
in.
to ensure the user provided a blueprint mesh.
@gunney1 gunney1 self-assigned this Oct 2, 2024
@gunney1 gunney1 added Quest Issues related to Axom's 'quest' component Klee Related to the Klee component labels Oct 2, 2024
@gunney1 gunney1 requested a review from Arlie-Capps January 6, 2025 13:21
Copy link
Contributor

@bmhan12 bmhan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I appreciate the "todo" issue tracking in #1445.

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gunney1!

Please ensure that API changes are documented in the RELEASE_NOTES.

I have a concern about whether the proposed change to the inout octree API will work with our Fortran interface. Please test this with shroud's make generate command followed by running the Fortran tests.

src/axom/klee/Geometry.hpp Outdated Show resolved Hide resolved
src/axom/klee/Geometry.hpp Show resolved Hide resolved
src/axom/klee/Geometry.hpp Show resolved Hide resolved
src/axom/klee/Geometry.hpp Show resolved Hide resolved
src/axom/klee/GeometryOperatorsIO.hpp Outdated Show resolved Hide resolved
src/axom/quest/DiscreteShape.cpp Show resolved Hide resolved
src/axom/quest/DiscreteShape.cpp Outdated Show resolved Hide resolved
src/axom/quest/DiscreteShape.cpp Show resolved Hide resolved
src/axom/quest/examples/quest_shape_in_memory.cpp Outdated Show resolved Hide resolved
@gunney1
Copy link
Contributor Author

gunney1 commented Jan 11, 2025

Thanks @gunney1!

Please ensure that API changes are documented in the RELEASE_NOTES.

I have a concern about whether the proposed change to the inout octree API will work with our Fortran interface. Please test this with shroud's make generate command followed by running the Fortran tests.

All the tests passed after make generate. Does that include the Fortran tests? Or is there a separate make target for Fortran tests? I can't tell from the make help output.

@kennyweiss
Copy link
Member

kennyweiss commented Jan 11, 2025

I have a concern about whether the proposed change to the inout octree API will work with our Fortran interface. Please test this with shroud's make generate command followed by running the Fortran tests.

All the tests passed after make generate. Does that include the Fortran tests? Or is there a separate make target for Fortran tests? I can't tell from the make help output.

Thanks @gunney1. I looked a bit closer, and we don't expose the inout_init() overload with the mint::Mesh, so I think it's ok:

## InOut query methods
- decl: int inout_init( const std::string& fileName, MPI_Comm comm)
format:
function_suffix: _mpi
cpp_if: ifdef AXOM_USE_MPI
- decl: int inout_init( const std::string& fileName)
format:
function_suffix: _serial
cpp_if: ifndef AXOM_USE_MPI

@ltaylor16 -- could you please look at the changes to inout.*pp and quest_shroud.yaml and confirm?

@Arlie-Capps
Copy link

Brian, I appreciate that you split this into three PRs. I'll be reviewing this one today.

Comment on lines 112 to 113
* \brief Loads the shape from file into m_surfaceMesh and, if its a C2C
* contour, computes a revolvedVolume for the shape.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"if it's a C2C contour" or "if the file is a C2C contour"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-worked the comment, which was written when this step was undergoing significant changes. Now, there are non-file shapes. This function now ensures that the discrete shape representation is ready for use, computing it if needed. Computing the revolved volume for C2C shapes is there for backward compatibility.

Copy link

@Arlie-Capps Arlie-Capps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Brian.

@gunney1 gunney1 merged commit 550b991 into develop Jan 14, 2025
13 checks passed
@gunney1 gunney1 deleted the feature/gunney/construct-shapes-in-memory branch January 14, 2025 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Klee Related to the Klee component Quest Issues related to Axom's 'quest' component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants