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

Fix Triangle2d/Triangle3d interior sampling to correctly follow triangle #12766

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

mweatherley
Copy link
Contributor

@mweatherley mweatherley commented Mar 28, 2024

Objective

When I wrote #12747 I neglected to translate random samples from triangles back to the point where they originated, so they would be sampled near the origin instead of at the actual triangle location.

Solution

Translate by the first vertex location so that the samples follow the actual triangle.

@matiqo15 matiqo15 added C-Bug An unexpected or incorrect behavior A-Math Fundamental domain-agnostic mathematical operations labels Mar 28, 2024
@bushrat011899
Copy link
Contributor

I agree this solves the bug, not sure how I missed it in the initial review! Just as an aside, this function can be made a lot simpler now that I'm looking at it more closely:

/// Interior sampling for triangles which doesn't depend on the ambient dimension.
fn sample_triangle_interior<P: NormedVectorSpace, R: Rng + ?Sized>(
    [a, b, c]: [P; 3],
    rng: &mut R,
) -> P {
    // Generate random points on a parallelipiped and reflect so that
    // we can use the points that lie outside the triangle
    let u = rng.gen_range(0.0..=1.0);
    let v = rng.gen_range(0.0..=1.0);

    a * (1. - u - v).abs() + b * u + c * v;
}

This removes 1 P-space vector addition and eliminates the branch, and also includes the fix you've developed. Even without this suggestion, the fix you've provided makes sense and I approve!

@mweatherley
Copy link
Contributor Author

I agree this solves the bug, not sure how I missed it in the initial review! Just as an aside, this function can be made a lot simpler now that I'm looking at it more closely:

/// Interior sampling for triangles which doesn't depend on the ambient dimension.
fn sample_triangle_interior<P: NormedVectorSpace, R: Rng + ?Sized>(
    [a, b, c]: [P; 3],
    rng: &mut R,
) -> P {
    // Generate random points on a parallelipiped and reflect so that
    // we can use the points that lie outside the triangle
    let u = rng.gen_range(0.0..=1.0);
    let v = rng.gen_range(0.0..=1.0);

    a * (1. - u - v).abs() + b * u + c * v;
}

This removes 1 P-space vector addition and eliminates the branch, and also includes the fix you've developed. Even without this suggestion, the fix you've provided makes sense and I approve!

I don't think this is right; when the inside of the absolute value is negative, the sum of coefficients is 2u + 2v - 1, when it should always be 1.

@bushrat011899
Copy link
Contributor

Yes you're correct, I gotta stop trying to do maths at midnight. I'd neglected the change in the coefficients of b and c.

/// Interior sampling for triangles which doesn't depend on the ambient dimension.
fn sample_triangle_interior<P: NormedVectorSpace, R: Rng + ?Sized>(
    [a, b, c]: [P; 3],
    rng: &mut R,
) -> P {
    // Generate random points on a parallelipiped
    let u = rng.gen_range(0.0..=1.0);
    let v = rng.gen_range(0.0..=1.0);

    // Reflect so that we can use the points that lie outside the triangle
    let (u, v) = if u + v > 1. {
        (1. - u, 1. - v)
    } else {
        (u, v)
    };

    // Ensure coefficients sum to 1
    let t = 1. - u - v;

    a * t + b * u + c * v;
}

Still removes the extra vector operation, but definitely more of a style choice than anything else now.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'd like to see tests that our sampling methods always return values inside of the shapes, but I won't block on it here.

@alice-i-cecile alice-i-cecile 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 29, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 29, 2024
Merged via the queue into bevyengine:main with commit bcdb20d Mar 29, 2024
30 checks passed
@mweatherley mweatherley deleted the triangle-sampling-fix branch March 29, 2024 14:30
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-Bug An unexpected or incorrect behavior 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.

4 participants