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

Adding sample_mesh.py function #29

Merged
merged 2 commits into from Sep 16, 2022
Merged

Adding sample_mesh.py function #29

merged 2 commits into from Sep 16, 2022

Conversation

odedstein
Copy link
Collaborator

Adding a function to sample a mesh. Future possible additions:

  • tet meshes (currently only polyline and triangle mesh)
  • blue noise (currently only uniform / white noise)

We might want to consider deprecating random_points_on_polyline. For now, I have just updated that function to use sample_mesh.

sample_mesh is also a nice example of how I think we should handle random number generation in the library in the future.

@sgsellan
Copy link
Owner

Thanks! I took the liberty of making a few changes:

  • Switched the name sample_mesh to random_points_on_mesh. I think I feel strongly about this. sample_mesh doesn't tell you what you're sampling (is it a function?), and it sounds ambiguous (is it generating a "sample" mesh?). random_points_on_mesh is completely unambiguous.
  • I went through all other library functions and it doesn't seem like there's any other functions where we use a random seed, so no changes needed. But I agree to do it like this from now on. We do use random seeding in tests. Would be nice to change it, but less critical than updating the library functionality itself.
  • I added a deprecation warning to random_points_on_polyline saying we'll deprecate it in the next release (0.0.3).

If you agree with these changes and checks run, I am happy to merge.

I think I'll make the 0.0.2 release out of this and the last PRs with all the BVH functionality. Do you agree?

@odedstein
Copy link
Collaborator Author

I'm fine with the renaming and the other changes.

@sgsellan sgsellan merged commit 2e2ceef into sgsellan:main Sep 16, 2022
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.

None yet

2 participants