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

Implemented voxel-mesh intersection #102

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BIackthorn
Copy link

@BIackthorn BIackthorn commented Jan 9, 2025

Contributing Guidelines

Description

The mesh_boundary_masker previously used warp aabb intersection testing to determine when voxels intersect with the mesh. This just found the voxels that intersected with the triangle bounding boxes and not the triangles themselves. I added warp functions to do the triangle intersection testing after the aabb test was passed, so only those voxels that actually intersect the triangles are made solid.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • All pytest tests pass

All tests passing except JAX macroscopic (will be fixed in other PR)

Linting and Code Formatting

Make sure the code follows the project's linting and formatting standards. This project uses Ruff for linting.

To run Ruff, execute the following command from the root of the repository:

ruff check .
  • Ruff passes

Old:
windtunnel_3d_1000_old

New:
windtunnel_3d_1000_new

Timing for SAE_Coarse stl file and the cached code:
New mesh masker completed in 0.004960 seconds.
Old mesh masker completed in 0.004057 seconds.

For DrivAer-Notchback.stl:
New method: 0.45 sec
Old method : 0.457865 seconds
Older method (wp.mesh_query_point_no_sign): 0.457386

Copy link

github-actions bot commented Jan 9, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@hsalehipour hsalehipour self-requested a review January 9, 2025 17:18
@BIackthorn
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@hsalehipour
Copy link
Collaborator

Thanks for the contribution. Can you please modify the "description" part on the top of this PR and explain what the issue was and what you did to fix it?

@nmorrisad
Copy link

I have read the CLA Document and I hereby sign the CLA

Copy link
Collaborator

@hsalehipour hsalehipour left a comment

Choose a reason for hiding this comment

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

Looks great! thank you.

@mehdiataei
Copy link
Contributor

Thanks Nigel. A couple of things:

  1. It doesn't look like ruff formatting is applied. I see extra spaces, spaces missing, etc. that should be easily fixed by ruff formatter.
  2. Can you improve the naming of the variables? We usually use more descriptive longer names that would help with others understanding the code.

Thanks.

@BIackthorn
Copy link
Author

Ok, please take a look at the changes.

@mehdiataei
Copy link
Contributor

Thanks Nigel. Can you squash the two commits with "Implemented voxel-mesh interaction"?

@BIackthorn
Copy link
Author

BIackthorn commented Jan 9, 2025

@mehdiataei
Copy link
Contributor

I can. But it can lead to bad commit messages. If you couldn't do it I'll squash and merge. A few more comments:

  1. ne0, ne1, and de in mesh_voxel_intersect are not immediately intuitive. Can you use better names for them? such as normal_edge0_pre etc.?
  2. Can you add detailed docstrings explaining the purpose, inputs, outputs, and any assumptions for each function?
  3. Currently, there's no check for triangles with zero normals. For example, with zero normal both dist1 and dist2 become zero and if (0 + 0) * (0 + 0) <= 0.0: will pass and falsely indicating an intersection with every voxel box.

@mehdiataei
Copy link
Contributor

When making changes you can simply do git commit --amend (and them git push -f ....) so you don't have to create a new commit for the changes.

@BIackthorn BIackthorn force-pushed the feature-voxel-intersect branch from cccdd25 to cd711d2 Compare January 10, 2025 20:44
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.

4 participants