Skip to content

Expose methods to return MOAB vertices and connectivity - #100

Merged
gonuke merged 19 commits into
xdg-org:mainfrom
Waqar-ukaea:expose-moab-verts
Jun 5, 2025
Merged

Expose methods to return MOAB vertices and connectivity#100
gonuke merged 19 commits into
xdg-org:mainfrom
Waqar-ukaea:expose-moab-verts

Conversation

@Waqar-ukaea

Copy link
Copy Markdown
Collaborator

This PR adds methods to the mesh manager interface to return the coordinates of vertices in a MOAB mesh as well as the connectivity between those vertices. The motivation for publicly exposing this is to make it easier to use the MeshManager API to transfer element vertices/indices to a GPRT buffer for building acceleration structures on device.

Two methods have been added:

  • MeshManager::get_surface_vertices(MeshID surface) returns a flattened std::vector of xyz coords of all the vertices in a surface.
  • MeshManager::get_surface_connectivity(MeshID surface) returns a flattened std::vector of indices for each vertex in a surface.

I've also added a couple tests to test_moab.cpp to ensure that these methods are returning the correct the number of vertices/indices. And added stubs for the LibMeshManager.

@pshriwise please let me know if you feel like this shouldn't be exposed in the MeshManager or if it makes more sense for the methods to return non-flattened vectors, etc...

@Waqar-ukaea
Waqar-ukaea requested a review from pshriwise April 15, 2025 10:25
@Waqar-ukaea Waqar-ukaea mentioned this pull request Apr 17, 2025
13 tasks

@pshriwise pshriwise left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A couple of design thoughts to discuss, but nothing major.

We'll need to think about how to produce connectivity for libMesh surfaces at some point.

Comment thread include/xdg/mesh_manager_interface.h Outdated
Comment thread include/xdg/moab/mesh_manager.h Outdated
@Waqar-ukaea

Waqar-ukaea commented Apr 28, 2025

Copy link
Copy Markdown
Collaborator Author

We'll need to think about how to produce connectivity for libMesh surfaces at some point.

Does libmesh track any kind of connectivity between elements? Maybe called something else and not connectivity in libmesh syntax? Or would we need to add this ourselves?

@Waqar-ukaea

Copy link
Copy Markdown
Collaborator Author

I've rewritten these methods and also written a new method that returns both vertices and connectivity since I was struggling to retain the correct local connectivity for a surface when using the individual methods. Not sure if I want to keep the individual methods or just stick with the combined MeshManager::get_surface_mesh method.

I wanted to write a test for get_surface_mesh but I wasn't sure how to go about doing so without brute force testing each element so that's what I have done so far. MeshManager reads in a fairly small mesh overlap-edge.h5m and does a comparison between a flattened vector of connectivity against the expected and the same for vertices in xyz.

I actually already merged these changes into my GPRT branch and have successfully used them to ray trace against a MOAB mesh in single precision so this is honestly probably ready to be pushed into main - see https://github.com/pshriwise/xdg/pull/94#issuecomment-2841553742 for updates on that. @pshriwise just let me know if you are happy with the changes made and i'll merge.

@pshriwise pshriwise left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a couple of small things here. The switch to MeshMock isn't such a small lift but I appreciate you taking the time to make that change. Thanks @Waqar-ukaea!

Comment thread tests/test_moab.cpp Outdated
Comment thread tests/test_moab.cpp

@gonuke gonuke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I may have missed some important design discussions, but I think there are better ways to get the info from MOAB.

Most comments here are about interacting with the MOAB interface a little more cleanly.

Comment thread include/xdg/constants.h
Comment thread src/moab/mesh_manager.cpp Outdated
Comment thread src/moab/mesh_manager.cpp Outdated
auto conn = get_surface_connectivity(surface);
std::vector<moab::EntityHandle> verts;

verts.insert(verts.end(), conn.begin(), conn.end());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unless I'm missing something, this will have many/most vertices represented multiple times - is that the desired outcome? get_surface_connectivity() appears to get a list of the vertex EntityHandles for each triangle. In most surfaces, each vertex will appear in many triangles, so that this connectivity list will include each vertex multiple times.

Perhaps you mean for verts to be a std::set? But there are probably more efficient ways to get the vertices from a surface (at least in MOAB)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see below that you do remove the duplicates in that method. Probably should do the same here. Since I expect there to be more duplicates than non-duplicates, it may be valuable to avoid adding the duplicates in the first place.

Comment thread src/moab/mesh_manager.cpp Outdated
Comment thread src/moab/mesh_manager.cpp
Comment thread src/moab/mesh_manager.cpp Outdated
@pshriwise

Copy link
Copy Markdown
Collaborator

Does libmesh track any kind of connectivity between elements? Maybe called something else and not connectivity in libmesh syntax? Or would we need to add this ourselves?

Sorry I realized I never replied to this. libMesh does contain connectivity information, we'll just need to collect it element-by-element (as far as I understand it). libMesh doesn't have the batch collection methods that MOAB does.

@Waqar-ukaea

Copy link
Copy Markdown
Collaborator Author

Hi @gonuke @pshriwise, thanks for the suggestions I've gone back through the code where I use these methods downstream and realised that I don't necessarily need the get_surface_connectivity() or get_surface_vertices() methods. The problem with both of these functions is that they return global (to the MOAB instance) ids for connectivity/indices. But for my usecase, in providing GPRT with vertices and triangle indices, I need local (to the surface) indices, requiring me to map from the global connectivity - hence why I also wrote get_surface_mesh() which returns the local indices along with coords of those vertices in the correct order.

I can't really see a way to decouple this logic into two separate functions as i originally intended since I need the vertices to get the local indices. I have opted to remove the get_surface_connectivity() entirely and have kept get_surface_vertices() to return vertices without indices if that is ever required.

I have also rewritten parts of those methods to (hopefully) make use of the MOAB API in a cleaner fashion, making use of _surface_vertices() and moab_interface()->get_adjacencies()` to return vertices without duplicates.

@pshriwise

Copy link
Copy Markdown
Collaborator

Hi @gonuke @pshriwise, thanks for the suggestions I've gone back through the code where I use these methods downstream and realised that I don't necessarily need the get_surface_connectivity() or get_surface_vertices() methods. The problem with both of these functions is that they return global (to the MOAB instance) ids for connectivity/indices. But for my usecase, in providing GPRT with vertices and triangle indices, I need local (to the surface) indices, requiring me to map from the global connectivity - hence why I also wrote get_surface_mesh() which returns the local indices along with coords of those vertices in the correct order.

I can't really see a way to decouple this logic into two separate functions as i originally intended since I need the vertices to get the local indices. I have opted to remove the get_surface_connectivity() entirely and have kept get_surface_vertices() to return vertices without indices if that is ever required.

I have also rewritten parts of those methods to (hopefully) make use of the MOAB API in a cleaner fashion, making use of _surface_vertices() and moab_interface()->get_adjacencies()` to return vertices without duplicates.

Thank you @Waqar-ukaea! I'll have another look shortly, but what you've said makes sense. Down the road, I think accessing this info from the MBDirectAccess class will be more efficient as the triangle/element connectivity maps to the global ID space of the triangles naturally there, but I really don't want to lose focus of the main goal for this PR -- establishing ray tracing with GPRT.

@gonuke gonuke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are good changes @Waqar-ukaea - I have one small suggestion for modularity.

Comment thread src/moab/mesh_manager.cpp Outdated
Comment thread src/moab/mesh_manager.cpp Outdated
Comment thread src/moab/mesh_manager.cpp Outdated

@pshriwise pshriwise left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm happy moving forward with the changes here, knowing that there will likely be subsequent changes to these methods as we navigate the execution of this work on GPU.

@pshriwise

Copy link
Copy Markdown
Collaborator

@gonuke if you're happy with this after another look feel free to approve and merge.

@gonuke

gonuke commented Jun 3, 2025

Copy link
Copy Markdown
Collaborator

I thought I was waiting for one final commit with some suggestions that I made and @Waqar-ukaea seemed to agree to in comments...

@Waqar-ukaea

Copy link
Copy Markdown
Collaborator Author

@gonuke I think I have added all the changes you suggested now as well as added the MockMesh test as @pshriwise suggested. Just rebased the main branch into here too so assuming CI passes this one should be good to merge.

@gonuke gonuke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have a couple of non-blocking queries here in tests. I'll wait to merge in case there are quick responses - feel free to let me know that you'd prefer to leave it as is and I'll merge.

Comment thread tests/test_moab.cpp Outdated
Comment thread tests/test_moab.cpp
@gonuke
gonuke merged commit b5350eb into xdg-org:main Jun 5, 2025
@Waqar-ukaea
Waqar-ukaea deleted the expose-moab-verts branch October 30, 2025 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants