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

PlaneMeshBuilder should include a configurable amount of subdivision for the mesh #13258

Open
EmiOnGit opened this issue May 6, 2024 · 3 comments
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Regression Functionality that used to work but no longer does. Add a test for this! C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon

Comments

@EmiOnGit
Copy link
Contributor

EmiOnGit commented May 6, 2024

What problem does this solve or what need does it fill?

In bevy 0.12 the

// mod shape
struct Plane {
  size: f32, 
  subdivision: u32
}

struct was used to construct plane meshes.
The subdivision parameter controlled the amount of subdivisions in the resulting mesh.

This control was removed with the introduction of Plane3d.
However, this behavior is very useful for prototyping or games that generate their own meshes in bevy instead of loading them from external sources and should be provided by bevy IMO.

What solution would you like?

Add a subdivision parameter to PlaneMeshBuilder.
The subdivisions should default to the current behavior.
I also believe the code for creating the mesh can be easily ported from the old implementation in bevy 0.12.

pub struct PlaneMeshBuilder {
    pub plane: Plane3d,
    pub half_size: Vec2,
    // The new parameter
    // should default as before
    pub subdivisions: u32, 
}

Debatable

I don't think the old design of subdivisions = 0 is great. The docs in 0.12 state:

subdivisions: u32
The number of subdivisions in the mesh.
0 - is the original plane geometry, the 4 points in the XZ plane.
1 - is split by 1 line in the middle of the plane on both the X axis and the Z axis, resulting in a plane with 4 quads / 8 triangles.
2 - is a plane split by 2 lines on both the X and Z axes, subdividing the plane into 3 equal sections along each axis, resulting in a plane with 9 quads / 18 triangles.
and so on...

Maybe this could be better represented as a enum? I'm open to suggestions here.

@EmiOnGit EmiOnGit added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels May 6, 2024
@EmiOnGit
Copy link
Contributor Author

EmiOnGit commented May 6, 2024

I also just saw that this is already included in the tracking issue #10572

@bugsweeper
Copy link
Contributor

bugsweeper commented May 8, 2024

I think subdivision should be part of Meshable trait which could generate mesh for any standard shapes with any level of detail, because bevy have no tesselation stage, if someone wants to make some vertex shader like noise there is no options for standard shapes. Why for example sphere is not a good shape for subdivision?

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use C-Regression Functionality that used to work but no longer does. Add a test for this! A-Math Fundamental domain-agnostic mathematical operations X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels May 10, 2024
@alice-i-cecile
Copy link
Member

I agree with @bugsweeper's desire for a generalized subdivision API, but IMO we should tackle the simple regression first :)

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 C-Regression Functionality that used to work but no longer does. Add a test for this! C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

No branches or pull requests

3 participants