bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add ramp primitive

Open lynn-lumen opened this issue 1 year ago • 9 comments

Objective

  • Fixes the ramp related parts of #11786
    • Edit: This should probably be tracked in #10572

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 avatar Feb 09 '24 08:02 lynn-lumen

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 avatar Feb 09 '24 08:02 lynn-lumen

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

Jondolf avatar Feb 09 '24 16:02 Jondolf

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

lynn-lumen avatar Feb 09 '24 18:02 lynn-lumen

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

valaphee avatar Feb 09 '24 23:02 valaphee

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.

lynn-lumen avatar Feb 09 '24 23:02 lynn-lumen

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

valaphee avatar Feb 10 '24 00:02 valaphee

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.

lynn-lumen avatar Apr 09 '24 02:04 lynn-lumen

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 avatar Apr 09 '24 11:04 NthTensor

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.

NthTensor avatar Apr 10 '24 14:04 NthTensor

Agreed.

NthTensor avatar May 07 '24 10:05 NthTensor