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

Uniform point sampling methods for some primitive shapes. #12484

Merged
merged 18 commits into from
Mar 17, 2024

Conversation

13ros27
Copy link
Contributor

@13ros27 13ros27 commented Mar 14, 2024

Objective

Give easy methods for uniform point sampling in a variety of primitive shapes (particularly useful for circles and spheres) because in a lot of cases its quite easy to get wrong (non-uniform).

Solution

Added the ShapeSample trait to bevy_math and implemented it for Circle, Sphere, Rectangle, Cuboid, Cylinder, Capsule2d and Capsule3d. There are a few other shapes it would be reasonable to implement for like Triangle, Ellipse and Torus but I'm not immediately sure how these would be implemented (other than rejection which could be the best method, and could be more performant than some of the solutions in this pr I'm not sure). This exposes the sample_volume and sample_surface methods to get both a random point from its interior or its surface. EDIT: Renamed sample_volume to sample_interior and sample_surface to sample_boundary

This brings in rand as a default optional dependency (without default features), and the methods take &mut impl Rng which allows them to use any random source implementing RngCore.


Changelog

Added

Added the methods sample_interior and sample_boundary to a variety of primitive shapes providing easy uniform point sampling.

@james7132 james7132 added A-Math Fundamental domain-agnostic mathematical operations C-Usability A simple quality-of-life change that makes Bevy easier to use labels Mar 14, 2024
@NthTensor
Copy link
Contributor

This is perfect! I'll verify correctness and approve tomorrow, but everything looks great.

I will just note for other reviews, because it's worth repeating: This rand dependency doesn't include any actual pseudorandom number generation, it just an API consumer. This means it's compatible with the deterministic-ecs magic provided by crates like bevy_rand!

@JohnTheCoolingFan
Copy link
Contributor

Awesome feature. But I would like to know how uniform the distribution of sampled points is. Glancing over the implementation, particularly circle/sphere and capsule I suspect have a non-uniform sampling, meaning the "density" of points would not be uniform across the domain, in Cartesian coordinates.

@mweatherley
Copy link
Contributor

Awesome feature. But I would like to know how uniform the distribution of sampled points is. Glancing over the implementation, particularly circle/sphere and capsule I suspect have a non-uniform sampling, meaning the "density" of points would not be uniform across the domain, in Cartesian coordinates.

I am fairly certain they are uniform.

@13ros27
Copy link
Contributor Author

13ros27 commented Mar 15, 2024

Awesome feature. But I would like to know how uniform the distribution of sampled points is. Glancing over the implementation, particularly circle/sphere and capsule I suspect have a non-uniform sampling, meaning the "density" of points would not be uniform across the domain, in Cartesian coordinates.

Provided I haven't made any mistakes these should be uniform. The circle and sphere in particular use this disk sampling method (and the sphere equivalent) which uses a square root to skew the points towards the outside for balance. And the capsules are split by surface area / volume (lines like rng.gen_bool((cylinder_vol / capsule_vol) as f64)) and then they just use the methods for circle/sphere/rectangle/cylinder as appropriate.

@Bluefinger
Copy link
Contributor

We probably could do with some statistical tests to give us more confidence that we are indeed getting uniform distributions and sampling. For reference, I do such tests here for turborand: https://github.com/Bluefinger/turborand/blob/main/tests/smoke/mod.rs#L657

Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

Just a couple mathematical issues, otherwise this looks good to me! Thanks for putting in the work to do this!

(I realize now that my comments on Cylinder and Capsule3d were left in weird places, hopefully that's not too confusing.)

crates/bevy_math/src/shape_sampling.rs Show resolved Hide resolved
crates/bevy_math/src/shape_sampling.rs Show resolved Hide resolved
crates/bevy_math/src/shape_sampling.rs Show resolved Hide resolved
crates/bevy_math/src/shape_sampling.rs Show resolved Hide resolved
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Really pleased with this. Consensus on if a statistical test is blocking here? I'm inclined to move forward without it.

Future work:

  • Make primitive meshes and primitive uniform point clouds "agree"
  • Experiment with rejection sampling, see if it's faster anywhere
  • Think about implementing ShapeSample on triangle meshes maybe

@NthTensor
Copy link
Contributor

Oh also can we change sample_volume/sample_surface to sample_interior/sample_boundary? It's more correct in the 2d case.

@Bluefinger
Copy link
Contributor

I don't think the tests should block this, but maybe be part of follow-up work (since I think there'll be some work to figure out the best way to do them for the provided methods).

@13ros27
Copy link
Contributor Author

13ros27 commented Mar 15, 2024

I've added tests for Circle.sample_interior and Circle.sample_boundary. The sample_boundary test just uses the angle its at (essentially which wedge of the circle it is in) and sample_interior checks which random box the point is in (it can be in multiple). The boxes it uses are
image

@pablo-lua pablo-lua added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 17, 2024
Copy link
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

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

Seems good, but I will say "Tests, tests, tests".
I haven't found any math mistakes but I still don't feel too sure to sign this off myself.


fn sample_boundary<R: Rng + ?Sized>(&self, rng: &mut R) -> Vec3 {
let primary_side1 = rng.gen_range(-1.0..1.0);
let primary_side2 = rng.gen_range(-1.0..1.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

primary_side2... sounds almost like a secondary_side. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my naming there is a little questionable, its really the sides that are fully varied in comparison to the one which is just -1 or 1

@alice-i-cecile alice-i-cecile added the C-Needs-Release-Note Work that should be called out in the blog due to impact label Mar 17, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2024
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Mar 17, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2024
Merged via the queue into bevyengine:main with commit 948ea31 Mar 17, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Needs-Release-Note Work that should be called out in the blog due to impact C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants