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

Add ramp primitive #11790

Closed
wants to merge 24 commits into from
Closed

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented Feb 9, 2024

Objective

Solution

  • Added Ramp to bevy_math and implemented traits for Ramp.

Changelog

  • Implemented for Ramp: Default, Meshable, impl_reflect!, Bounded3d and for Gizmos: impl GizmoPrimitive3d<Ramp> for Gizmos

@lynn-lumen
Copy link
Contributor Author

lynn-lumen commented Feb 9, 2024

Note: this comment is obsolete now.

One remaining issue is that Cone as described in the RFC is centred at the centre of its base circle. The gizmo cone however is centred at the middle of its vertical axis.
Screenshot 2024-02-09 at 09 48 34

I'm personally in favour of centring the mesh like the gizmo because it would not introduce breaking changes and it seems more consistent with the implementations of the other shapes, but in that case the RFC should probably be updated.

@lynn-lumen lynn-lumen marked this pull request as draft February 9, 2024 08:54
@lynn-lumen lynn-lumen marked this pull request as ready for review February 9, 2024 10:34
@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 9, 2024
@Jondolf
Copy link
Contributor

Jondolf commented Feb 9, 2024

I like the ramp, but the cone should be in a separate PR. Meshing for cones isn't quite as trivial.

From what I can see, the cone here uses duplicate vertices for the tip. This is problematic for a few reasons, but the most evident one is perhaps that it can produce visible creases. A previous PR used your approach and has some discussion on this: #10298

My original primitive meshing PR (#11007) has meshing for both cones and conical frusta. It uses the alternative approach of a single vertex for the tip with an "invalid" normal so that it doesn't contribute to the shading. From what I know, this is the only way we currently have for making a perfectly smooth cone mesh. The code for that is here: https://github.com/bevyengine/bevy/pull/11007/files#diff-e7b7f96e4d538ed02ad300fdb7a47d0c7d355ca437ee0c6c5f678e6de81001bb

@lynn-lumen
Copy link
Contributor Author

Oh wow, I didn't know normals could be used like that. I'll remove the Cone-related code from this PR.

@lynn-lumen lynn-lumen changed the title Add ramp and cone meshes Add ramp prmitive Feb 9, 2024
@lynn-lumen lynn-lumen changed the title Add ramp prmitive Add ramp primitive Feb 9, 2024
@valaphee
Copy link
Contributor

valaphee commented Feb 9, 2024

Could ramp also be represented by a more general triangular prism?

@lynn-lumen
Copy link
Contributor Author

It could be, but I don't think it should be. The issue with that is that these shapes are preferably simple. If we would implement a general prism, we would probably want it to be a prism with a regular base (as in a regular n-gon). The ramp always has one 90° corner though and can as such not be such a prism.
Alternatively, we could implement a Wedge shape, which would cover all triangular prisms and a bit more.

@valaphee
Copy link
Contributor

Was referring to https://docs.godotengine.org/en/stable/classes/class_prismmesh.html a ramp would basically be a prism with left_to_right set to 0.0, is just a thought, the problem with wedges is that they don't have to be parallel, and prism is a subclass, and ramps are a subclass of prisms

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Mar 26, 2024
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.

Thanks for adding this! I know this has changed a lot since the initial post, but bear with me here.

I do have some questions. First, why are we making a specific shape for triangular prisms instead of just extruding our 2d shapes? I feel that either we should do "general right prisms" or we should do "ramps", and this is sort of a weird middle-ground.

Second, I was confused by the example in 3d_shapes. The base seems to be an irregular triangle, which I was not expecting from the code. Are we sure that's correct?

@lynn-lumen
Copy link
Contributor Author

lynn-lumen commented Apr 9, 2024

First, why are we making a specific shape for triangular prisms instead of just extruding our 2d shapes? I feel that either we should do "general right prisms" or we should do "ramps", and this is sort of a weird middle-ground.

The current implementation allows for any kind of triangle as the base of the prism. I am not sure what the difference between a "general right prism" and a "ramp" would be though. However, assuming both are triangular prisms, both of them are possible with this implementation.

Currently, both right-angled-triangles as well as any other triangle can be used as the base shape, which brings us closer to what other engines like Godot allow, but which is not what the original RFC is describing. I would personally be in favour of adding a ramp and maybe a wedge shape as described in #11786. This would probably be good, because most users will probably want to use Ramps without the additional complexity, but for those who actually want wedges (including prisms) the Wedge shape would be available. This is probably a design decision though and I would appreciate some feedback. :)

Second, I was confused by the example in 3d_shapes. The base seems to be an irregular triangle, which I was not expecting from the code. Are we sure that's correct?

Yes, this is correct. The apex_displacement is set to 0.5 meaning that the apex edge of the prism will be above the half way point between the center of the bottom quad and its back edge, which seems to be what is displayed.

@NthTensor
Copy link
Contributor

Currently, both right-angled-triangles as well as any other triangle can be used as the base shape

I was asking about right prisms not right triangle bases. As in a prism that is not oblique.

@NthTensor
Copy link
Contributor

Ok, after a bit of discussion with the author I'd like to suggest reverting this back to just plain-old-ramps. While I would like to see an arbitrary prism primitive, I find the apex_displacement parameter rather intuitive and at odds with our general approach to triangles.

For general prisms, it seems like it would be nicer to add the ability to extrude our (compact) 2d primitives into 3d primitives. Lets leave that for possible follow up.

@lynn-lumen lynn-lumen requested a review from NthTensor April 12, 2024 10:26
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.

Looks good to me.

crates/bevy_gizmos/src/primitives/dim3.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded3d/primitive_impls.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
lynn-lumen and others added 4 commits May 3, 2024 11:48
Co-Authored-By: NiseVoid <NiseVoid@users.noreply.github.com>
Co-Authored-By: NiseVoid <NiseVoid@users.noreply.github.com>
Co-Authored-By: Joona Aalto <jondolf.dev@gmail.com>
@Jondolf Jondolf mentioned this pull request May 3, 2024
41 tasks
Copy link
Contributor

@Jondolf Jondolf 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 pretty content with this. I'm not sure on the direction the ramp should be facing, but that might just be preference.

It would be good to add the ramp gizmo to the math/render_primitives example, but I'm fine leaving that to a follow-up.

@NthTensor NthTensor 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 May 3, 2024
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 7, 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 really prefer the extrusion abstraction in #13270. Does this still need to exist with that in place? A ramp is just a triangular prism, right?

IMO we should close this out and add then add convenience methods to construct ramp-like extruded triangles, rather than extending the set of primitives. I'm not fully confident this would be adequate though: so please let me know if I've missed something.

@NthTensor
Copy link
Contributor

Agreed.

@lynn-lumen lynn-lumen closed this May 7, 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 S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants