Add and impl Primitives #10580

Aztro-dev · 2023-11-16T04:15:13Z

Add and implement constructors for Primitives

Adds more Primitive types and adds a constructor for almost all of them
Works towards finishing Tracking Issue: Primitive Shapes #10572

Solution

Created new primitives
- Torus
- Conical Frustum
- Cone
- Ellipse
Implemented constructors (Primitive::new) for almost every single other primitive.

Jondolf

Thanks, looks very nice overall! I left some notes on consistency and docs, and also on frusta.

A viewing frustum would probably be more useful for rendering, feedback from others would be nice. ConicalFrustum also feels somewhat niche, but I have no objection against including it.

crates/bevy_math/src/primitives/dim2.rs

Jondolf · 2023-11-16T06:50:38Z

crates/bevy_math/src/primitives/dim2.rs

@@ -127,6 +172,13 @@ pub struct Triangle2d {
 }
 impl Primitive2d for Triangle2d {}

+impl Triangle2d {
+    /// Create a new `Triangle2d` from a list of Vertices
+    pub fn new(vertices: [Vec2; 3]) -> Self {


It'd be nice to have a method that takes points a, b and c instead. Taking an array doesn't feel quite as intuitive in my opinion, although it's almost the same.

Not sure which one of them should be new, but the other method could be either from_points or from_array based on what is chosen.

I would prefer this to be the new, and not include from_array at all.

crates/bevy_math/src/primitives/dim3.rs

crates/bevy_math/src/primitives/dim2.rs

Jondolf · 2023-11-16T07:44:44Z

crates/bevy_math/src/primitives/dim3.rs

+/// A conical frustum can be created
+/// by slicing off a section of a cone.
+#[derive(Clone, Copy, Debug)]
+pub struct ConicalFrustum {


This is also nice to have, but if we also want primitives to be used for rendering, a viewing frustum would be more useful. For rendering, it would be defined by an aspect-ratio, fov and the near and far planes, but maybe that's a bit too rendering-specific for primitives? More opinions on this would be useful.

If we add a viewing frustum though, it should probably be done in a separate PR since it'd touch on rendering code too and be a bit more controversial.

If we want to support different types of frusta, then perhaps a polygonal frustum with a regular polygon as a base could also be useful?

@superdump also briefly touched on this here: #10466 (comment)

Yep, we should split rendering frustra into a different PR.

Rendering frustum most likely belongs in the bounding PR. Might be good to create some helper later to create a primitive shape you can render from the 6 plane representation (which is not exactly most convinient way to render them)

crates/bevy_math/src/primitives/dim3.rs

crates/bevy_math/src/primitives/dim2.rs

crates/bevy_math/src/primitives/dim3.rs

alice-i-cecile

New shapes look great! I left some guidance on the parameterization of them.

More constructors is helpful, but we shouldn't have trivial ones, aka those that simply duplicate public fields.

Consistency with other primitive docs Co-authored-by: Joona Aalto <[email protected]>

Clarify documentation of Cone Co-authored-by: Joona Aalto <[email protected]>

crates/bevy_math/src/primitives/dim3.rs

Clarify Torus documentation Co-authored-by: Alice Cecile <[email protected]>

crates/bevy_math/src/primitives/dim3.rs

crates/bevy_math/src/primitives/dim2.rs

crates/bevy_math/src/primitives/dim3.rs

crates/bevy_math/src/primitives/dim2.rs

alice-i-cecile

Thanks for your patience :) Only a couple more nits left!

…rcle_radius

crates/bevy_math/src/primitives/dim2.rs

alice-i-cecile

Remaining items:

new constructor Triangle2d that takes points a, b and c.
Same constructor, but for Triangle3d
Iterator-based constructor for Polygon<N>.

Jondolf · 2023-11-17T15:57:17Z

Same constructor, but for Triangle3d

Triangle3d doesn't exist yet because it was unclear if it's needed: #10466 (comment)

But maybe it should be added?

alice-i-cecile · 2023-11-17T16:17:55Z

Ah, got it. Let's do that in a different PR!

Aztro-dev · 2023-11-17T16:33:35Z

new constructor Triangle2d that takes points a, b and c.

Should I still have another constructor that takes a slice of 3 Vec2s?

Jondolf · 2023-11-17T16:35:33Z

I believe not: #10580 (comment)
It would be another trivial constructor equivalent to doing Triangle2d { vertices: [...] }

Aztro-dev · 2023-11-17T16:45:39Z

Remaining Items should be completed 👍

crates/bevy_math/src/primitives/dim2.rs

Update Polyline constructor documentation Co-authored-by: Joona Aalto <[email protected]>

…ording

Jondolf

Last comments, just a few small doc things

Jondolf · 2023-11-17T17:14:42Z

crates/bevy_math/src/primitives/dim3.rs

+    }
+}
+
+/// A cone primitive.


As @NiseVoid suggested (#10580 (comment)), maybe we should mention where the origin of the cone should be so that different integrations have consistent representations?

If the options are the base and apex (tip), the base is more intuitive for me personally. I view cones like Wikipedia's description:

a three-dimensional geometric shape that tapers smoothly from a flat base (frequently, though not necessarily, circular) to a point called the apex or vertex.

crates/bevy_math/src/primitives/dim3.rs

Co-authored-by: Joona Aalto <[email protected]>

Jondolf

LGTM, thanks for your patience! The cone origin thing (#10580 (comment)) would be nice to reach a consensus on, but it's not a blocker for me in terms of this PR.

# Add and implement constructors for Primitives - Adds more Primitive types and adds a constructor for almost all of them - Works towards finishing bevyengine#10572 ## Solution - Created new primitives - Torus - Conical Frustum - Cone - Ellipse - Implemented constructors (`Primitive::new`) for almost every single other primitive. --------- Co-authored-by: Joona Aalto <[email protected]> Co-authored-by: Alice Cecile <[email protected]>

Aztro-dev added 2 commits November 15, 2023 16:44

Add new primitives

3c439a1

Impl new functions for primitive shapes

4c79096

Jondolf reviewed Nov 16, 2023

View reviewed changes

nicopap added the C-Feature A new feature, making something new possible label Nov 16, 2023

alice-i-cecile added the A-Math Fundamental domain-agnostic mathematical operations label Nov 16, 2023

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Nov 16, 2023

View reviewed changes

Aztro-dev and others added 3 commits November 16, 2023 10:42

Update crates/bevy_math/src/primitives/dim2.rs

5e12cc9

Consistency with other primitive docs Co-authored-by: Joona Aalto <[email protected]>

Implement requested changes and change up constructors a bit

e976114

Update crates/bevy_math/src/primitives/dim3.rs

5200e06

Clarify documentation of Cone Co-authored-by: Joona Aalto <[email protected]>

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved

Aztro-dev and others added 5 commits November 16, 2023 12:52

Implement requested changes

926c2df

Remove more trivial constructors

235b473

Update crates/bevy_math/src/primitives/dim3.rs

2efd563

Clarify Torus documentation Co-authored-by: Alice Cecile <[email protected]>

Should be the last of the trivial constructors

9d5cbc3

Implicit return

ca5f977

Jondolf reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved

Aztro-dev added 2 commits November 16, 2023 13:00

Added index enumeration

5bf8491

Minor capsule documentation fix

5e93e79

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 16, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved

Aztro-dev added 3 commits November 16, 2023 20:33

Iterator fix

9422276

Fix CI error

c543697

take() instead of checking for iterator size

05d488f

alice-i-cecile reviewed Nov 17, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 17, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Show resolved Hide resolved

alice-i-cecile requested changes Nov 17, 2023

View reviewed changes

Implement from_iter() for structs and check for non-positive circumci…

5340e96

…rcle_radius

alice-i-cecile reviewed Nov 17, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Nov 17, 2023

View reviewed changes

Polygon iterator constructor and Triangle2d a,b,c constructor

cf2b44e

Jondolf reviewed Nov 17, 2023

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

Aztro-dev and others added 2 commits November 17, 2023 10:58

Update crates/bevy_math/src/primitives/dim2.rs

0550f30

Update Polyline constructor documentation Co-authored-by: Joona Aalto <[email protected]>

Update FromIterator constructors to have more general documentation w…

5b7e525

…ording

alice-i-cecile approved these changes Nov 17, 2023

View reviewed changes

alice-i-cecile requested review from Jondolf and NiseVoid November 17, 2023 17:01

Jondolf suggested changes Nov 17, 2023

View reviewed changes

Aztro-dev and others added 2 commits November 17, 2023 11:32

Update dim3.rs

fce6f02

Co-authored-by: Joona Aalto <[email protected]>

Update dim3.rs docs

2bc9c0c

Co-authored-by: Joona Aalto <[email protected]>

Jondolf approved these changes Nov 17, 2023

View reviewed changes

alice-i-cecile mentioned this pull request Nov 17, 2023

Clarify where the origin of the Cone primitive should lie #10616

Closed

alice-i-cecile added this pull request to the merge queue Nov 17, 2023

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 Nov 17, 2023

Merged via the queue into bevyengine:main with commit 33cd59f Nov 17, 2023
22 checks passed

Jondolf mentioned this pull request Nov 18, 2023

Tracking Issue: Primitive Shapes #10572

Open

41 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and impl Primitives #10580

Add and impl Primitives #10580

Aztro-dev commented Nov 16, 2023

Jondolf left a comment

Jondolf Nov 16, 2023

alice-i-cecile Nov 16, 2023

Jondolf Nov 16, 2023 •

edited

Loading

Jondolf Nov 16, 2023

alice-i-cecile Nov 16, 2023

NiseVoid Nov 16, 2023

alice-i-cecile left a comment

alice-i-cecile left a comment

alice-i-cecile left a comment

Jondolf commented Nov 17, 2023

alice-i-cecile commented Nov 17, 2023

Aztro-dev commented Nov 17, 2023 •

edited

Loading

Jondolf commented Nov 17, 2023

Aztro-dev commented Nov 17, 2023

Jondolf left a comment

Jondolf Nov 17, 2023 •

edited

Loading

Jondolf left a comment

Add and impl Primitives #10580

Add and impl Primitives #10580

Conversation

Aztro-dev commented Nov 16, 2023

Add and implement constructors for Primitives

Solution

Jondolf left a comment

Choose a reason for hiding this comment

Jondolf Nov 16, 2023

Choose a reason for hiding this comment

alice-i-cecile Nov 16, 2023

Choose a reason for hiding this comment

Jondolf Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Jondolf Nov 16, 2023

Choose a reason for hiding this comment

alice-i-cecile Nov 16, 2023

Choose a reason for hiding this comment

NiseVoid Nov 16, 2023

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

Jondolf commented Nov 17, 2023

alice-i-cecile commented Nov 17, 2023

Aztro-dev commented Nov 17, 2023 • edited Loading

Jondolf commented Nov 17, 2023

Aztro-dev commented Nov 17, 2023

Jondolf left a comment

Choose a reason for hiding this comment

Jondolf Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Jondolf left a comment

Choose a reason for hiding this comment

Jondolf Nov 16, 2023 •

edited

Loading

Aztro-dev commented Nov 17, 2023 •

edited

Loading

Jondolf Nov 17, 2023 •

edited

Loading