Refactor primitive meshing #12793

gibletfeets · 2024-03-30T05:08:59Z

Objective

Primitive meshing is suboptimal
Improve primitive meshing

Solution

Unify all circle generation logic to be based off of the same functionality
Allows easier modification of suboptimal circle generation logic.

Among a set of a few PRs to refactor and improve primitive meshing.

Summary of changes

3d geometry that uses circle generation has been refactored to utilize CircleIterator. Behavior has not been changed for all shapes, except for capsule.rs.
- This has resulted in a significant improvement in speed in cases such as torus.rs, and worst-case, equivalent speed but (hopefully) better readability in cases such as capsule.rs.
Various variables that specified the parameters of the geometries to be created have been changed from u32 to usize to keep things more in line with sphere.rs, and to allow for somewhat more convenient array indexing calculations for capsule.rs. usize may not be ideal, and it may be more favorable to use u32 for these values instead.
For capsule.rs, more significant breaking changes have been made:
- The latitudes variable - which after integer division by 2 specified the quantity of lines of longitude, has been renamed to stacks, to be consistent with how sphere.rs behaves. stacks now defines the quantity of quad rows on each hemisphere. Accordingly, the setter has also been renamed.
  - For migration, the original latitudes number should be halved. If it was an odd number, this never affected capsule geometry anyway.
- The longitudes variable - which defined the quantity of lines of latitude, has been renamed to sectors, to also be consistent with how sphere.rs behaves. The setter for this variable has also been renamed.
  - No other behavior has been changed, so migration should only require renaming.

gibletfeets · 2024-03-30T05:12:05Z

Images displaying that the new mesh code seems to work as intended:

Benchmark from unmodified logic:

Benchmark from changed logic, showing minimal changes (as should be expected - the functionality was simply moved.):

crates/bevy_render/src/mesh/primitives/circle_iterator.rs

Fixing test issues Co-authored-by: vero <email@atlasdostal.com>

…tfeets/bevy into refactor-primitive-meshing

gibletfeets · 2024-03-30T06:48:04Z

Preemptively apologizing for the awful git history, I am still in the process of learning git.

rparrett · 2024-03-30T13:17:58Z

I sort of recall this sort of refactor being discussed in discord, and I think the main optimization being the ability to pre-compute a lookup table of sin/cos values when the iterator is constructed. That would prevent some needless trig in loops in the torus/cylinder. Have you tried that?

rparrett · 2024-03-30T13:11:24Z

crates/bevy_render/src/mesh/primitives/circle_iterator.rs

+            if self.wrap {
+                self.count -= 1;
+                Some(Vec2::new(
+                    (self.theta * self.count as f32).cos(),


Why not sin_cos?

The main goal of this step was to duplicate the existing logic and relocate it to a unified function, and then validate that it hasn't impacted performance or broken the existing meshing. I do agree that sin_cos would make sense, though.

rparrett · 2024-03-30T13:23:28Z

crates/bevy_render/src/mesh/primitives/circle_iterator.rs

+
+impl CircleIterator {
+    pub(crate) fn new(count: usize, wrap: bool) -> CircleIterator {
+        if wrap {


I think this if could potentially be removed by count: count + wrap as usize.

Correct. I received some similar feedback earlier and will revise accordingly.

rparrett · 2024-03-30T13:25:20Z

crates/bevy_render/src/mesh/primitives/circle_iterator.rs

+}
+
+impl CircleIterator {
+    pub(crate) fn new(count: usize, wrap: bool) -> CircleIterator {


A doc comment here explaining the inputs would be appreciated.

Noted, will revise.

rparrett · 2024-03-30T13:29:52Z

crates/bevy_render/src/mesh/primitives/circle_iterator.rs

+    }
+}
+impl Iterator for CircleIterator {
+    type Item = Vec2;


Just opening a discussion here:

Thoughts on using Vec2 here? Having the resulting values accessed by .x and .y seems sort of confusing.

Maybe (f32, f32) or perhaps SinCos { sin: f32, cos: f32 }?

Vec2 was chosen because the first alternative to sin/cos calls that I wanted to investigate was application of a Mat2 for rotation, which would get generated on the initialization of the iterator - which could then be subsequently cloned for creation of identical sets of unit circle points.

If this method does not yield favorable results, I agree that something else should be used (and if it does, it would still make sense to have it return something with labels that are more intuitive than .x, .y).

gibletfeets · 2024-03-30T23:21:43Z

I sort of recall this sort of refactor being discussed in discord, and I think the main optimization being the ability to pre-compute a lookup table of sin/cos values when the iterator is constructed. That would prevent some needless trig in loops in the torus/cylinder. Have you tried that?

I have yet to try a pre-computed lookup, but it is definitely a method that is worth investigating.

atlv24 · 2024-03-31T03:50:08Z

I was the one who suggested the reduction in trigonometry, the goal with this PR was to preserve existing logic and just refactor. It was not done all in one go because this way we can benchmark the refactor and the potential optimization separately

mweatherley · 2024-03-31T11:40:30Z

As I recall, the optimization that @rodolphito brought up in the first place was computing a rotation matrix for the fixed step angle and then just using that to rotate around the circle by iteratively applying it; the point of that is that you only have to do trigonometry one time and the rest is just linear algebra, so it might be a little bit faster.

On the other hand, I'm not sure it's really a priority; as we discussed, there is a concern with numerical stability when using that approach, and you run the risk, for instance, of having circular meshes with duplicated vertices not closing up properly if you aren't careful.

…tfeets/bevy into refactor-primitive-meshing

gibletfeets · 2024-04-01T14:26:32Z

So, I ended up making a few changes:

For portions that required a distribution of points/(cos, sin) values that wraps around to a value corresponding to 0 radians, I used circle_iterator.cycle().take(num_points + 1), and I removed the wrap flag from the iterator.
I changed how CircleIterator functions. It now produces its points via a fixed rotation matrix, and it uses f64 types internally in order to improve numerical precision for calculations.

Furthermore, it turned out I had made a mistake with creating the initial benchmark. It now actually creates the meshes, and I also made it create meshes at several resolutions. I have made sure to reproduce benchmarks for the appropriate stages of my changes.

Before moving original logic to CircleIterator and refactoring:

After initial refactor:

After changing iterator to use a fixed rotation matrix:

…tfeets/bevy into refactor-primitive-meshing

gibletfeets · 2024-04-05T10:28:21Z

I added additional constructors wrapping, semicircle, and quarter_circle to CircleIterator, for performing respective functions that the various refactored primitives used.

This was fairly simple and was mostly a matter of me wanting to make it more clear what was occurring when I was manually creating these forms of iterator in the various geometry implementations.

I reorganized the code for capsule generation quite a lot.

I basically reimplemented it in a way that is based somewhat off of the sphere implementation, and the only preserved detail from the original is the basic optimization ideas and the math for calculating UVs. The UV math is still the same and the revision should not yield notably distinct results.

There was also some confusing variable naming - latitudes controlled the number of lines of longitude around the hemispheres, and longitudes controlled the number of lines of latitude.

More importantly, latitudes is never used for the actual geometry calculations, and instead it gets divided by 2. This means that specifying values of 15 and 14 would yield the same geometry.

So, I changed the names to be reflective of the sphere implementation, and made it so latitudes becomes stacks, and longitudes becomes sectors. The behavior is largely the same, however, stacks is equivalent to latitudes/2, so any code that specifies the resolutions for a capsule should take this into account.

…tfeets/bevy into refactor-primitive-meshing

atlv24

its lovely to see so much red in the capsule file

atlv24 · 2024-04-07T01:31:57Z

benches/Cargo.toml

@@ -68,6 +68,12 @@ name = "torus"
 path = "benches/bevy_render/torus.rs"
 harness = false

+[[bench]]
+name = "capsule"
+path = "benches/bevy_render/capsule.rs"


Torus and capsule could probably live together in primitives.rs

atlv24 · 2024-04-07T01:34:20Z

benches/benches/bevy_render/torus.rs

+            black_box(
+                TorusMeshBuilder::new(black_box(0.5), black_box(1.0))
+                    .minor_resolution(black_box(12))
+                    .major_resolution(black_box(16))


Its interesting that torus needs the builder-style api to set the resolution while Capsule (and Sphere?) have them set from new but then also have builder methods. Would be nice to make them all consistent

atlv24 · 2024-04-07T01:43:45Z

crates/bevy_render/src/mesh/primitives/dim3/capsule.rs

-            longitudes: 32,
-            latitudes: 16,
+            sectors: 32,
+            stacks: 8,


How would you feel about unifying mesh resolution setting across primitives to something along the lines of

/// The number of segments along the U texture coordinate. u_segments: u32, /// The number of segments along the V texture coordinate. v_segments: u32,

I feel like stacks and sectors are not widespread terminology and clearly longitudes and latitudes are easy to mix up given that bevy main capsule has them flipped, plus they dont make sense for something like a cylinder. U and V is both generic over primitve and reuses existing terminology

atlv24 · 2024-04-07T01:50:35Z

crates/bevy_render/src/mesh/primitives/dim3/capsule.rs

+        let stacks_iter = CircleIterator::quarter_circle(stacks);
+        let sector_circle: Vec<Vec2> = CircleIterator::wrapping(sectors).collect();
+
+        //insertion of top hemisphere and its associated UVs and normals


i think convention is to have a space precede the comment // like this

atlv24 · 2024-04-07T01:52:31Z

crates/bevy_render/src/mesh/primitives/dim3/capsule.rs

+                let z = xz * q.y;
+                vertices.push([x, y + half_length, z]);
+                normals.push([x * length_inv, y * length_inv, z * length_inv]);
+                uvs.push([(j as f32) / sectors_f32, v]);


can probably store inv_sectors

gibletfeets added 7 commits March 29, 2024 04:27

Add files via upload

d430554

Add files via upload

c421e29

Added a benchmark for the torus mesh generation function.

90963cd

Added torus generation benchmark.

86e6de4

Merge branch 'main' of https://github.com/gibletfeets/bevy

512084e

Update torus.rs

da14a71

Update torus.rs

19017ba

Refactored circle generation logic in primitive meshes.

6e356cd

gibletfeets force-pushed the refactor-primitive-meshing branch from 49ea8c8 to 6e356cd Compare March 30, 2024 05:17

atlv24 reviewed Mar 30, 2024

View reviewed changes

crates/bevy_render/src/mesh/primitives/circle_iterator.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/mesh/primitives/circle_iterator.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/mesh/primitives/circle_iterator.rs Show resolved Hide resolved

gibletfeets and others added 3 commits March 29, 2024 22:31

Update crates/bevy_render/src/mesh/primitives/circle_iterator.rs

5a31d31

Fixing test issues Co-authored-by: vero <email@atlasdostal.com>

Update crates/bevy_render/src/mesh/primitives/circle_iterator.rs

1bd3e48

Fixing test issues Co-authored-by: vero <email@atlasdostal.com>

Update crates/bevy_render/src/mesh/primitives/circle_iterator.rs

d08fae3

Fixing test issues Co-authored-by: vero <email@atlasdostal.com>

gibletfeets force-pushed the refactor-primitive-meshing branch from f07b9b0 to d08fae3 Compare March 30, 2024 06:17

gibletfeets added 6 commits March 29, 2024 23:24

Merge branch 'main' into refactor-primitive-meshing

dd0725e

Merge branch 'main' into refactor-primitive-meshing

8049680

Merge branch 'refactor-primitive-meshing' of https://github.com/gible…

8f8c980

…tfeets/bevy into refactor-primitive-meshing

Merge branch 'refactor-primitive-meshing' of https://github.com/gible…

c5d4341

…tfeets/bevy into refactor-primitive-meshing

Merge branch 'refactor-primitive-meshing' of https://github.com/gible…

9ba2b9e

…tfeets/bevy into refactor-primitive-meshing

amogus

cfbb22a

james7132 added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change A-Math Fundamental domain-agnostic mathematical operations labels Mar 30, 2024

rparrett self-requested a review March 30, 2024 13:08

rparrett reviewed Mar 30, 2024

View reviewed changes

gibletfeets added 4 commits April 1, 2024 04:51

Merge branch 'bevyengine:main' into refactor-primitive-meshing

8e7ca84

Changed usage of circle iterator, made benchmarks actually functional.

2b8f50b

Merge branch 'refactor-primitive-meshing' of https://github.com/gible…

025591d

…tfeets/bevy into refactor-primitive-meshing

Changed iterator usage, made benchmark actually functional

188d654

gibletfeets added 4 commits April 3, 2024 01:21

Merge branch 'bevyengine:main' into refactor-primitive-meshing

57176e8

Refactored capsule, optimized circle_iterator.

f4bafb9

Merge branch 'refactor-primitive-meshing' of https://github.com/gible…

fcbb23b

…tfeets/bevy into refactor-primitive-meshing

Merge branch 'main' into refactor-primitive-meshing

de58e4f

gibletfeets added 2 commits April 5, 2024 03:33

Forgot to rename setter functions.

5792f15

Merge branch 'refactor-primitive-meshing' of https://github.com/gible…

980cbdc

…tfeets/bevy into refactor-primitive-meshing

atlv24 reviewed Apr 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor primitive meshing #12793

Refactor primitive meshing #12793

gibletfeets commented Mar 30, 2024 •

edited

gibletfeets commented Mar 30, 2024

gibletfeets commented Mar 30, 2024

rparrett commented Mar 30, 2024

rparrett Mar 30, 2024

gibletfeets Mar 30, 2024

rparrett Mar 30, 2024

gibletfeets Mar 30, 2024

rparrett Mar 30, 2024

gibletfeets Mar 30, 2024

rparrett Mar 30, 2024

gibletfeets Mar 30, 2024

gibletfeets commented Mar 30, 2024

atlv24 commented Mar 31, 2024

mweatherley commented Mar 31, 2024

gibletfeets commented Apr 1, 2024 •

edited

gibletfeets commented Apr 5, 2024

atlv24 left a comment

atlv24 Apr 7, 2024

atlv24 Apr 7, 2024

atlv24 Apr 7, 2024

atlv24 Apr 7, 2024

atlv24 Apr 7, 2024

Refactor primitive meshing #12793

Are you sure you want to change the base?

Refactor primitive meshing #12793

Conversation

gibletfeets commented Mar 30, 2024 • edited

Objective

Solution

Summary of changes

gibletfeets commented Mar 30, 2024

gibletfeets commented Mar 30, 2024

rparrett commented Mar 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibletfeets commented Mar 30, 2024

atlv24 commented Mar 31, 2024

mweatherley commented Mar 31, 2024

gibletfeets commented Apr 1, 2024 • edited

gibletfeets commented Apr 5, 2024

atlv24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibletfeets commented Mar 30, 2024 •

edited

gibletfeets commented Apr 1, 2024 •

edited