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

Meshable extrusions #13478

Merged
merged 23 commits into from
Jun 4, 2024
Merged

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented May 22, 2024

Objective

  • Implement Meshable for Extrusion<T>

Solution

  • Meshable requires Meshable::Output: MeshBuilder now. This means that all some_primitive.mesh() calls now return a MeshBuilder. These were added for primitives that did not have one prior to this.
  • A new trait Extrudable: MeshBuilder has been added. This trait allows you to specify the indices of the perimeter of the mesh created by this MeshBuilder and whether they are to be shaded smooth or flat.
  • Extrusion<P: Primitive2d + Meshable> is now Meshable aswell. The associated MeshBuilder is ExtrusionMeshBuilder which is generic over P and uses the MeshBuilder of its baseshape internally.
  • ExtrusionMeshBuilder exposes the configuration functions of its base-shapes builder.
  • Updated the 3d_shapes example to include Extrusions

Migration Guide

  • Depending on the context, you may need to explicitly call .mesh().build() on primitives where you have previously called .mesh()
  • The Output type of custom Meshable implementations must now derive MeshBuilder.

Additional information

  • The extrusions UVs are done so that
    • the front face (+Z) is in the area between (0, 0) and (0.5, 0.5),
    • the back face (-Z) is in the area between (0.5, 0) and (1, 0.5)
    • the mantle is in the area between (0, 0.5) and (1, 1). Each PerimeterSegment you specified in the Extrudable implementation will be allocated an equal portion of this area.
  • The UVs of the base shape are scaled to be in the front/back area so whatever method of filling the full UV-space the base shape used is how these areas will be filled.

Here is an example of what that looks like on a capsule:

Extrusion.UVs.mp4

This is the texture used:

extrusion uvs

The 3d_shapes example now looks like this:

image_2024-05-22_235915753

@alice-i-cecile alice-i-cecile 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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward M-Needs-Release-Note Work that should be called out in the blog due to impact labels May 22, 2024
@mweatherley mweatherley self-requested a review May 23, 2024 11:05
Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

I like this quite a lot! Mostly I just want more documentation (both user-facing and internal) about how the process is expected to work.

Specifically:

  • I think the MeshBuilder constraint is reasonable, and adding it to everything should be fine (if anything, I think making these uniform is good).
  • I think that encoding the perimeter of two-dimensional meshes in a trait on the builder seems like the right approach.
  • I think that the way that flat and interpolated normals are computed seems quite reasonable.

The only things I'm kind of iffy about are the way that UVs are constructed (not a big deal anyhow) and the reimplementations of resolution on extruded builders.

lynn-lumen and others added 2 commits May 24, 2024 03:05
Co-authored-by: Matty <weatherleymatthew@gmail.com>
Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

Just a couple minor remaining things but I'm pretty happy with this now. :)

The migration guide should also probably mention the obligatory MeshBuilder bound on Meshable::Output, since Meshable is a public trait.

@lynn-lumen lynn-lumen mentioned this pull request Jun 3, 2024
3 tasks
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 3, 2024
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 API, and this functionality seems like a reasonable way to use extrusions.

Like always, docs and code quality are excellent.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 4, 2024
@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 Jun 4, 2024
Merged via the queue into bevyengine:main with commit fd82ef8 Jun 4, 2024
28 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jun 4, 2024
# Objective

- Implement `Extrudable` for all meshbuilders of shapes that have been
added after #13478 was created

## Solution

- Implemented meshing for extrusions of `CircularSector`,
`CircularSegment` and `Rhombus`

## Testing

- The correctness of these was confirmed visually.

## Additional information

Here is an image of what they look like :)

![Screenshot 2024-06-04
230633](https://github.com/bevyengine/bevy/assets/62256001/d9cca0ba-30ea-4c48-8ae2-007b469739d7)

Co-authored-by: Lynn Büttgenbach <62256001+solis-lumine-vorago@users.noreply.github.com>
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1363 if you'd like to help out.

@alice-i-cecile alice-i-cecile removed the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jun 5, 2024
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants