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

Implement Meshable for some 3D primitives #11688

Merged
merged 7 commits into from
Feb 6, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Feb 4, 2024

Objective

Split up from #11007, fixing most of the remaining work for #10569.

Implement Meshable for Cuboid, Sphere, Cylinder, Capsule, Torus, and Plane3d. This covers all shapes that Bevy has mesh structs for in bevy_render::mesh::shapes.

Cone and ConicalFrustum are new shapes, so I can add them in a follow-up, or I could just add them here directly if that's preferrable.

Solution

Implement Meshable for Cuboid, Sphere, Cylinder, Capsule, Torus, and Plane3d.

The logic is mostly just a copy of the the existing bevy_render shapes, but Plane3d has a configurable surface normal that affects the orientation. Some property names have also been changed to be more consistent.

The default values differ from the old shapes to make them a bit more logical:

  • Spheres now have a radius of 0.5 instead of 1.0. The default capsule is equivalent to the default cylinder with the sphere's halves glued on.
  • The inner and outer radius of the torus are now 0.5 and 1.0 instead of 0.5 and 1.5 (i.e. the new minor and major radii are 0.25 and 0.75). It's double the width of the default cuboid, half of its height, and the default sphere matches the size of the hole.
  • Cuboid is 1x1x1 by default unlike the dreaded Box which is 2x1x1.

Before, with "old" shapes:

old

Now, with primitive meshing:

new

I only changed the 3d_shapes example to use primitives for now. I can change them all in this PR or a follow-up though, whichever way is preferrable.

Sphere API

Spheres have had separate Icosphere and UVSphere structs, but with primitives we only have one Sphere.

We need to handle this with builders:

// Existing structs
let ico = Mesh::try_from(Icophere::default()).unwrap();
let uv = Mesh::from(UVSphere::default());

// Primitives
let ico = Sphere::default().mesh().ico(5).unwrap();
let uv = Sphere::default().mesh().uv(32, 18);

We could add methods on Sphere directly to skip calling .mesh().

I also added a SphereKind enum that can be used with the kind method:

let ico = Sphere::default()
    .mesh()
    .kind(SphereKind::Ico { subdivisions: 8 })
    .build();

The default mesh for a Sphere is an icosphere with 5 subdivisions (like the default Icosphere).


Changelog

  • Implement Meshable and Default for Cuboid, Sphere, Cylinder, Capsule, Torus, and Plane3d
  • Use primitives in 3d_shapes example

@Jondolf Jondolf added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Math Fundamental domain-agnostic mathematical operations labels Feb 4, 2024
@Jondolf Jondolf mentioned this pull request Feb 4, 2024
46 tasks
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Some thoughts:

  • I like that the default primitives are more uniformly scaled. Getting any more specific than that would be super bikeshedy.
  • I like the api in general. Good use of the builder pattern.
  • I would prefer if cuboid, plane, cylinder and torus got their own modules instead of living in dim3.

Looks pretty good all in all.

@spectria-limina
Copy link
Contributor

I did a review ass, the biggest thing is I don't understand why you're duplicating all this code instead of making use of existing code where it exists. Duplicated code is fragile; it would be much more preferable to build a render::mesh::shape and convert that to a mesh where possible.

@NthTensor
Copy link
Contributor

NthTensor commented Feb 6, 2024

The plan is to remove the render shapes in a follow up. We are temporally copying rather than replacing to make the patch smaller and more manageable.

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 content with this, but I'd like to remove the duplicated code ASAP.

@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 Feb 6, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 6, 2024
Merged via the queue into bevyengine:main with commit cf15e6b Feb 6, 2024
24 checks passed
@Jondolf Jondolf deleted the 3d-primitive-meshing branch February 7, 2024 17:29
github-merge-queue bot pushed a commit that referenced this pull request Feb 8, 2024
# Objective

#11431 and #11688 implemented meshing support for Bevy's new geometric
primitives. The next step is to deprecate the shapes in
`bevy_render::mesh::shape` and to later remove them completely for 0.14.

## Solution

Deprecate the shapes and reduce code duplication by utilizing the
primitive meshing API for the old shapes where possible.

Note that some shapes have behavior that can't be exactly reproduced
with the new primitives yet:

- `Box` is more of an AABB with min/max extents
- `Plane` supports a subdivision count
- `Quad` has a `flipped` property

These types have not been changed to utilize the new primitives yet.

---

## Changelog

- Deprecated all shapes in `bevy_render::mesh::shape`
- Changed all examples to use new primitives for meshing

## Migration Guide

Bevy has previously used rendering-specific types like `UVSphere` and
`Quad` for primitive mesh shapes. These have now been deprecated to use
the geometric primitives newly introduced in version 0.13.

Some examples:

```rust
let before = meshes.add(shape::Box::new(5.0, 0.15, 5.0));
let after = meshes.add(Cuboid::new(5.0, 0.15, 5.0));

let before = meshes.add(shape::Quad::default());
let after = meshes.add(Rectangle::default());

let before = meshes.add(shape::Plane::from_size(5.0));
// The surface normal can now also be specified when using `new`
let after = meshes.add(Plane3d::default().mesh().size(5.0, 5.0));

let before = meshes.add(
    Mesh::try_from(shape::Icosphere {
        radius: 0.5,
        subdivisions: 5,
    })
    .unwrap(),
);
let after = meshes.add(Sphere::new(0.5).mesh().ico(5).unwrap());
```
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 A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible 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.

6 participants