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

Example for random sampling #13507

Merged
merged 4 commits into from
May 27, 2024

Conversation

mweatherley
Copy link
Contributor

@mweatherley mweatherley commented May 25, 2024

Objective

We introduced a bunch of neat random sampling stuff in this release; we should do a good job of showing people how to use it, and writing examples is part of this.

Solution

A new Math example, random_sampling, shows off the ShapeSample API functionality. For the moment, it renders a cube and allows the user to sample points from its interior or boundary in sets of either 1 or 100:
Screenshot 2024-05-25 at 1 16 08 PM

On the level of code, these are reflected by two ways of using ShapeSample:

// Get a single random Vec3:
let sample: Vec3 = match *mode {
    Mode::Interior => shape.0.sample_interior(rng),
    Mode::Boundary => shape.0.sample_boundary(rng),
};
// Get 100 random Vec3s:
let samples: Vec<Vec3> = match *mode {
    Mode::Interior => {
        let dist = shape.0.interior_dist();
        dist.sample_iter(&mut rng).take(100).collect()
    }
    Mode::Boundary => {
        let dist = shape.0.boundary_dist();
        dist.sample_iter(&mut rng).take(100).collect()
    }
};

Testing

Run the example!

Discussion

Maybe in the future it would be nice to show off all of the different shapes that we have implemented ShapeSample for, but I wanted to start just by demonstrating the functionality. Here, I chose a cube because it's simple and because it looks good rendered transparently with backface culling disabled.

@mweatherley mweatherley added C-Examples An addition or correction to our examples A-Math Fundamental domain-agnostic mathematical operations X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 25, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 25, 2024
Copy link
Contributor

@lynn-lumen lynn-lumen left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

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.

I don't have much to say about the code itself, but I will note the following:

  • Camera motion felt too fast, I think it would be good if you first converted mouse delta to a screen-space percentage and then scaled that by some sensitivity factor, rather than choosing an arbitrary pixel -> radian value.
  • The code should explain more that the sampling does NOT take into account the transform of the mesh, the fact they agree is due to how the scene is constructed. (Maybe show the user how to use the cube entity's global transform to transform the sample to world-space?)
  • Would be nice if: You could tilt camera up/down.
  • Would be nice if: (as you said) You could look at various other shapes
  • Would be nice if: Some sort of batching for the point spheres to let me to look at more of them!
  • Would be nice if: Some sort of free-running mode where points spawn (and despawn) by themselves).

We can tackle the would be's in another example to improve the readability of this one or just not at all.

@mweatherley
Copy link
Contributor Author

* Camera motion felt too fast, I think it would be good if you first converted mouse delta to a screen-space percentage and then scaled that by some sensitivity factor, rather than choosing an arbitrary pixel -> radian value.

Hmm, I do use pretty low mouse sensitivity! I actually just copied what I did in the align example. To me the most important thing is that you can see the thing from different angles to get a sense of depth, so I'd be more inclined to just tweak the magic number than doing any kind of more complex logic, but I can definitely tweak it.

* The code should explain more that the sampling does NOT take into account the transform of the mesh, the fact they agree is due to how the scene is constructed. (Maybe show the user how to use the cube entity's global transform to transform the sample to world-space?)

Yeah, I should have addressed that more explicitly. Maybe I'll add the transformation to the sampling code even if it doesn't do anything, just to give the right idea.

@IQuick143
Copy link
Contributor

My rationale for the suggestion of using window units is that:
A) imo it's fairly intuitive when I "grab" the screen, move it by some amount, and it rotates proportionally.
B) it deals pretty much automatically with resolution differences.
C) the magic constant is in more tangible units imo

But it's definitely not a blocker, just some polish I felt would be good.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 25, 2024
@alice-i-cecile
Copy link
Member

I'll merge this Monday: feel free to tweak it until then :)

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Clean and fun to use! Looks good :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 27, 2024
Merged via the queue into bevyengine:main with commit 787df44 May 27, 2024
31 checks passed
@mweatherley mweatherley deleted the sampling-example branch May 27, 2024 19:15
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-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants