Skip to content

Meshable extrusions#13478

Merged
alice-i-cecile merged 23 commits intobevyengine:mainfrom
lynn-lumen:meshable-extrusions
Jun 4, 2024
Merged

Meshable extrusions#13478
alice-i-cecile merged 23 commits intobevyengine:mainfrom
lynn-lumen:meshable-extrusions

Conversation

@lynn-lumen
Copy link
Copy Markdown
Contributor

@lynn-lumen lynn-lumen commented May 22, 2024 •
edited
Loading

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-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-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
mweatherley
mweatherley suggested changes May 23, 2024
View reviewed changes
Copy link
Copy Markdown
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
@lynn-lumen @mweatherley
Fix typos and improve docs
9a79564 
Co-authored-by: Matty <weatherleymatthew@gmail.com>
@lynn-lumen
Improve docs and add more code comments
74b3d53
mweatherley
mweatherley approved these changes May 24, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mweatherley mweatherley left a comment •
edited
Loading

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.

mweatherley
mweatherley reviewed May 27, 2024
View reviewed changes
@lynn-lumen lynn-lumen mentioned this pull request Jun 3, 2024
Merged
3 tasks
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 3, 2024
alice-i-cecile
alice-i-cecile approved these changes Jun 4, 2024
View reviewed changes
Copy link
Copy Markdown
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
@lynn-lumen lynn-lumen mentioned this pull request Jun 4, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 4, 2024
@lynn-lumen
Meshable extrusions - Part 2 (#13676)
8e4e840 
# 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 alice-i-cecile mentioned this pull request Jun 5, 2024
Closed
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Jun 5, 2024

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-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

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@mweatherley mweatherley mweatherley approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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-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

Milestone

0.14

Development

Successfully merging this pull request may close these issues.

3 participants

@lynn-lumen @alice-i-cecile @mweatherley