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

Primitive Shapes #12

Merged
merged 59 commits into from
Dec 12, 2022
Merged

Primitive Shapes #12

merged 59 commits into from
Dec 12, 2022

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Apr 21, 2021

RENDERED

A proposal for 2d and 3d geometric primitives as lightweight interoperability types.

@aevyrie aevyrie marked this pull request as draft April 21, 2021 00:43
@ValorZard
Copy link

Can there be fixed point versions of these primitives for deterministic collision purposes?

@colepoirier
Copy link

Did you mean a proposal for 2d and 3d geometric primitives? :)

@OptimisticPeach
Copy link

OptimisticPeach commented Apr 21, 2021

Although orthogonal to the representation of these geometric primitives, I presume it would be logical to include mesh generation support to all of these. As such, would it be preferred to distinguish between a geometric primitive and a mesh generator, or keep them together? That is to say either this:

let bounding = bevy_geom::D3::Sphere;
// _and_
let mesh: Mesh = bevy_render::shape::Sphere::default().into();

Or

let primitive = bevy_geom::D3::Sphere;
// optionally, if necessary
let mesh = primitive.make_mesh();

In my opinion, I believe that giving bevy_geom's primitives the ability to generate the corresponding meshes would be the best option, instead of creating two separate Sphere/Torus/etc.


Note that bevy_geom::2d/3d are not valid module names; hence why I substituted with D3.

@aevyrie
Copy link
Member Author

aevyrie commented Apr 21, 2021

In my opinion, I believe that giving bevy_geom's primitives the ability to generate the corresponding meshes would be the best option, instead of creating two separate Sphere/Torus/etc.

Definitely agree. To keep the attached PRs small, I'll probably split this up:

  1. Add the primitives
  2. Refactor mesh generation
  3. Refactor other systems?

Note that bevy_geom::2d/3d are not valid module names; hence why I substituted with D3.

Nice catch, I always forget about that with "2d" and "3d" 🙄.

@alice-i-cecile
Copy link
Member

Can there be fixed point versions of these primitives for deterministic collision purposes?

This should be feasible; we'd want to pair it with a change to the Transform system to support this too though. I'm genuinely unsure about how we should best split the alternative coordinate representations overall though. It could be done with generics, basic code duplication, feature flags, and external crate...

@aevyrie
Copy link
Member Author

aevyrie commented Apr 21, 2021

I was about to say the same thing. I'm not sure I know what the best approach would be here.

Some options in order of implementation difficulty:

  1. Don't implement fixed point, all representations stay as float types. If someone needs fixed point for simulation, that can be handled internally in their physics system, which can just convert the outputs to floats.
  2. Don't implement fixed point yet and make it a separate RFC. The scope of that change might benefit from it. Downside is that the longer it waits, the harder it will be to implement.
  3. Implement fixed point for primitives and transforms. This will almost certainly complicate the API, unless someone has a clever/existing solution without that drawback.

@alice-i-cecile
Copy link
Member

Don't implement fixed point yet and make it a separate RFC. The scope of that change might benefit from it. Downside is that the longer it waits, the harder it will be to implement.

I think this is the way to go; the scope is large, it cuts across quite a few domains and it deserves its own motivation.

@colepoirier
Copy link

Don't implement fixed point yet and make it a separate RFC. The scope of that change might benefit from it. Downside is that the longer it waits, the harder it will be to implement.

I think this is the way to go; the scope is large, it cuts across quite a few domains and it deserves its own motivation.

Is this the relevant issue from the main bevy repo? bevyengine/bevy#1680

@ghost
Copy link

ghost commented Apr 21, 2021

I recently had a use case for drawing points/single pixels.

Should this be considered a special case of... several things?
I mean a line with length one, a 1x1 rect, circle with diameter 1 etc. could probably be considered points.

But perhaps having a separate type (in both bevy_geom::2d and bevy_geom::3d) for it might allow for better semantics and/or optimizations.

@alice-i-cecile
Copy link
Member

I think a separate type for points is better: the semantics are different, and the optimizations are very different.

I'm also of the opinion that it's a valid geometric primitive too: it's used in a lot of different places in modelling and you can approximate stuff quickly by treating them as points.

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
aevyrie and others added 5 commits April 24, 2021 14:52
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@aevyrie aevyrie marked this pull request as ready for review April 30, 2021 04:48
aevyrie and others added 3 commits January 23, 2022 12:32
Remove angle as a component
Merging 'bounding and collision' with 'where are the transforms'
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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'm happy with this RFC as it currently stands: I think it sets out a nice foundation to build on. There are small details to work out, such as what exactly the Meshable trait should look like or the exact optimal representation for each of the shapes, but we can figure those out as we build and benchmark.

We're already starting to reimplement similar tools within the engine: I'd like to move forward with a unified design ASAP to avoid having to deduplicate later.

@nicopap
Copy link

nicopap commented Apr 25, 2022

A few comments:

  • There exists 3d shapes that can be defined in terms of 2d shapes. Think of a pyramid with arbitrary base, or a "cylinder" with an arbitrary shape as base.
  • A 2d shape in 2d space is different from a 2d shape in 3d space. I think it's relevant to make that distinction.

The problem I've encountered trying to implement my shapes library, at least following this RFC design (with many struct rather than one massive fat enum) is finding and defining the proper conversion methods. If a Pyramid is defined in terms of a 2d shape, what do I store it as in the struct Pyramid? Which traits to define and which one to use for those specific cases? This is not just about pyramids, as @cart points out, there are many ways to mesh a sphere, how to handle that? It's actually a hard problem! This PR only hand-waves-in Bounding, Collider and Meshable, and doesn't really answer how they are to be used.

Maybe this doesn't need to be answered now! It can be subject to new RFCs. I don't think the "abstract" shapes proposed in the RFC are controversial at all, I like all the propositions everyone made here. Going from abstract shape to concrete mesh or line collection can be discussed in a new RFC. I'd even suggest that the way to handle contributions of new shapes should be discussed in some RFC.

Removing torus as a primitive shape
@aevyrie
Copy link
Member Author

aevyrie commented Apr 25, 2022

@alice-i-cecile with so much churn on this RFC, I'd like to update my implementation attempt, and we can collect concrete feedback there. I have some thoughts on further modifications to the RFC, but at this point it's been reviewed so many times I don't think further review will be helpful unless we have an implementation to look at.

Copy link

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Great addition.

@nicopap
Copy link

nicopap commented Apr 26, 2022

Here is my stupid simple shapes library https://github.com/nicopap/bevy-cool-shapes/blob/main/bevy_cool_shapes/src/lib.rs . I use it to show outline gizmos on screen like here https://github.com/nicopap/bevy-cool-shapes/tree/main/bevy_cool_shapes_render

@HackerFoo
Copy link

Here's my thoughts:

  • The initial RFC should only consider convex shapes. Concave shapes are much more difficult to work with, and will require convex decomposition for many uses.
  • I suggest focusing on 3D closed shapes, such that all normals are pointing outward. 2D shapes are an entirely different thing, even in 3D - they can't really be closed, lack volume, a point can't be inside, etc.
  • Focus on reworking existing shapes (boxes, spheres, and frustums) in the engine to be more consistent, rather than introducing shapes of questionable use. This will make it clear what operations are needed on these shapes.

Methods that would be valuable to me, and probably others:

  • Compute the support point and normal for a given direction. This is the farthest point on a shape's surface along a given vector (such that it has the maximum dot product), and the normal of the surface at that point. This is all that GJK and similar algorithms need for fast distance calculation, penetration, etc.
  • For shapes that don't need GJK (e.g. AABBs), accelerated intersection tests.
  • For simple shapes (box, sphere), finding the smallest shape enclosing another more complex shape

A lot of interesting operations on shapes can be reduced to checking for intersections.

bors bot pushed a commit to bevyengine/bevy that referenced this pull request May 5, 2022
# Objective

Bevy users often want to create circles and other simple shapes.

All the machinery is in place to accomplish this, and there are external crates that help. But when writing code for e.g. a new bevy example, it's not really possible to draw a circle without bringing in a new asset, writing a bunch of scary looking mesh code, or adding a dependency.

In particular, this PR was inspired by this interaction in another PR: #3721 (comment)

## Solution

This PR adds `shape::RegularPolygon` and `shape::Circle` (which is just a `RegularPolygon` that defaults to a large number of sides)

## Discussion

There's a lot of ongoing discussion about shapes in <bevyengine/rfcs#12> and at least one other lingering shape PR (although it seems incomplete).

That RFC currently includes `RegularPolygon` and `Circle` shapes, so I don't think that having working mesh generation code in the engine for those shapes would add much burden to an author of an implementation.

But if we'd prefer not to add additional shapes until after that's sorted out, I'm happy to close this for now.

## Alternatives for users

For any users stumbling on this issue, here are some plugins that will help if you need more shapes.

https://github.com/Nilirad/bevy_prototype_lyon
https://github.com/johanhelsing/bevy_smud
https://github.com/Weasy666/bevy_svg
https://github.com/redpandamonium/bevy_more_shapes
https://github.com/ForesightMiningSoftwareCorporation/bevy_polyline
robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
# Objective

Bevy users often want to create circles and other simple shapes.

All the machinery is in place to accomplish this, and there are external crates that help. But when writing code for e.g. a new bevy example, it's not really possible to draw a circle without bringing in a new asset, writing a bunch of scary looking mesh code, or adding a dependency.

In particular, this PR was inspired by this interaction in another PR: bevyengine#3721 (comment)

## Solution

This PR adds `shape::RegularPolygon` and `shape::Circle` (which is just a `RegularPolygon` that defaults to a large number of sides)

## Discussion

There's a lot of ongoing discussion about shapes in <bevyengine/rfcs#12> and at least one other lingering shape PR (although it seems incomplete).

That RFC currently includes `RegularPolygon` and `Circle` shapes, so I don't think that having working mesh generation code in the engine for those shapes would add much burden to an author of an implementation.

But if we'd prefer not to add additional shapes until after that's sorted out, I'm happy to close this for now.

## Alternatives for users

For any users stumbling on this issue, here are some plugins that will help if you need more shapes.

https://github.com/Nilirad/bevy_prototype_lyon
https://github.com/johanhelsing/bevy_smud
https://github.com/Weasy666/bevy_svg
https://github.com/redpandamonium/bevy_more_shapes
https://github.com/ForesightMiningSoftwareCorporation/bevy_polyline
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
# Objective

Bevy users often want to create circles and other simple shapes.

All the machinery is in place to accomplish this, and there are external crates that help. But when writing code for e.g. a new bevy example, it's not really possible to draw a circle without bringing in a new asset, writing a bunch of scary looking mesh code, or adding a dependency.

In particular, this PR was inspired by this interaction in another PR: bevyengine#3721 (comment)

## Solution

This PR adds `shape::RegularPolygon` and `shape::Circle` (which is just a `RegularPolygon` that defaults to a large number of sides)

## Discussion

There's a lot of ongoing discussion about shapes in <bevyengine/rfcs#12> and at least one other lingering shape PR (although it seems incomplete).

That RFC currently includes `RegularPolygon` and `Circle` shapes, so I don't think that having working mesh generation code in the engine for those shapes would add much burden to an author of an implementation.

But if we'd prefer not to add additional shapes until after that's sorted out, I'm happy to close this for now.

## Alternatives for users

For any users stumbling on this issue, here are some plugins that will help if you need more shapes.

https://github.com/Nilirad/bevy_prototype_lyon
https://github.com/johanhelsing/bevy_smud
https://github.com/Weasy666/bevy_svg
https://github.com/redpandamonium/bevy_more_shapes
https://github.com/ForesightMiningSoftwareCorporation/bevy_polyline
Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>
@james7132 james7132 added the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label Dec 10, 2022
@james7132
Copy link
Member

This RFC is all but merged at this point, with multiple ongoing PRs implementing parts of it already in flight and all of the open discussions have been resolved.

@cart does this still need further review?

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This is indeed in a good spot. Thanks to everyone for the reviews and iteration!

@cart cart merged commit c6e44e8 into bevyengine:main Dec 12, 2022
@tomaspecl
Copy link

Maybe the Meshable trait should also have a function that would accept some parametrization for specifying how is the mesh generated, how many triangles are used etc... But that would probably mean that each shape will have its own parameters. Perhaps the Meshable trait should have an associated type which will specify the options for generating the concrete shape.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Bevy users often want to create circles and other simple shapes.

All the machinery is in place to accomplish this, and there are external crates that help. But when writing code for e.g. a new bevy example, it's not really possible to draw a circle without bringing in a new asset, writing a bunch of scary looking mesh code, or adding a dependency.

In particular, this PR was inspired by this interaction in another PR: bevyengine#3721 (comment)

## Solution

This PR adds `shape::RegularPolygon` and `shape::Circle` (which is just a `RegularPolygon` that defaults to a large number of sides)

## Discussion

There's a lot of ongoing discussion about shapes in <bevyengine/rfcs#12> and at least one other lingering shape PR (although it seems incomplete).

That RFC currently includes `RegularPolygon` and `Circle` shapes, so I don't think that having working mesh generation code in the engine for those shapes would add much burden to an author of an implementation.

But if we'd prefer not to add additional shapes until after that's sorted out, I'm happy to close this for now.

## Alternatives for users

For any users stumbling on this issue, here are some plugins that will help if you need more shapes.

https://github.com/Nilirad/bevy_prototype_lyon
https://github.com/johanhelsing/bevy_smud
https://github.com/Weasy666/bevy_svg
https://github.com/redpandamonium/bevy_more_shapes
https://github.com/ForesightMiningSoftwareCorporation/bevy_polyline
@rlidwka
Copy link

rlidwka commented Dec 5, 2023

What is "Plane2d"?

As far as I'm aware, 2d world has only 1 plane (the world itself).

I feel like it should be either renamed, or removed completely in favor of Line2d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Ready-For-Final-Review Ready for a maintainer to consider for merging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.