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 cone, make cylinder special case of a cone #10298

Closed
wants to merge 6 commits into from

Conversation

torsteingrindvik
Copy link
Contributor

Objective

Bevy is lacking a cone mesh type.

Solution

Copy the cylinder implementation into a Cone type and split the single radius into top- and bottom radii.
Step this radius when iterating over the cone/cylinder rings.

Make cylinders a special case of a cone where the radii are equal.

Changelog

Added

  • Cone mesh added with configurable top and bottom radii. Cylinders are now implemented in terms of cones with equal radii.

@torsteingrindvik
Copy link
Contributor Author

cones.mp4

Here are a few examples. Changed material to unlit for visual clarity.

The cones in the video are defined as follows:

        meshes.add(
            shape::Cone {
                top_radius: 0.0,
                bottom_radius: 0.4,
                resolution: 32,
                ..default()
            }
            .into(),
        ),
        meshes.add(
            shape::Cone {
                top_radius: 0.9,
                bottom_radius: 0.8,
                resolution: 16,
                ..default()
            }
            .into(),
        ),
        meshes.add(
            shape::Cone {
                top_radius: 0.9,
                bottom_radius: 0.3,
                resolution: 16,
                segments: 10,
                ..default()
            }
            .into(),
        ),
        meshes.add(
            shape::Cone {
                top_radius: 1.0,
                bottom_radius: 0.0,
                resolution: 32,
                height: 0.5,
                ..default()
            }
            .into(),
        ),

use crate::mesh::{Indices, Mesh};
use wgpu::PrimitiveTopology;

/// A cone which stands on the XZ plane
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to comment on the tip and face geometry (especially the UVs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming this is resolved via your commit, let me know if not.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, I'd like much more detail, ala what HackerFoo was requesting below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking a bit about how I would go about making some diagrams or similar, but I feel making it in an ASCII editor will be messy.

I see there is some precedent for having images in docs: https://github.com/bevyengine/bevy/blob/main/assets/docs/Mesh.png
But that is only linked to and not shown directly via rustdoc.

Should I just create an image?

@torsteingrindvik
Copy link
Contributor Author

Also I noted other programs (Godot, Blender) have other features and optimizations such as choosing if caps should be generated, making top/bottom a single point (when zero radius) and more, but I thought this might be a good start.

@alice-i-cecile
Copy link
Member

Also I noted other programs (Godot, Blender) have other features and optimizations such as choosing if caps should be generated, making top/bottom a single point (when zero radius) and more, but I thought this might be a good start.

Yep, we can merge it without those, but making good-first-issues for follow-up work like that would be much appreciated.

@alice-i-cecile alice-i-cecile added A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use and removed A-Math Fundamental domain-agnostic mathematical operations labels Oct 28, 2023
@HackerFoo
Copy link
Contributor

Could you please add an image of the cone tip with smooth shading and a 0.0 tip radius, and no intermediate rings? I've only found one way to get this to work properly: #5891 (comment)

(TL;DR use a single vertex for the tip with the normal set to zero, and normalize interpolated vertex normals in the fragment shader.)

Also, a diagram would be useful, indicating the top & bottom radius, height, resolution, and segments.

Note that, with a radius of 0 at the tip, half of the triangles surrounding the tip are degenerate if using quads, since two of the vertices will be at the tip, forming a line to the other vertex. If you omit the degenerate triangles, the normal resolution is cut in half at the tip, resulting in visible creases from the base to the tip.

@HackerFoo
Copy link
Contributor

@torsteingrindvik
Copy link
Contributor Author

torsteingrindvik commented Oct 29, 2023

Yep, we can merge it without those, but making good-first-issues for follow-up work like that would be much appreciated.

Happy to do so when this gets merged. I have a few follow-up tasks I can think of.

Could you please add an image of the cone tip with smooth shading and a 0.0 tip radius, and no intermediate rings?

Video: https://github.com/bevyengine/bevy/assets/52322338/c3a96842-5113-4fcd-8fa4-8fc77309f266
Smooth-shaded default cylinder alongside a default cone.

I guess these are the creases you mentioned @HackerFoo?

At first I tried doing a single-vertex optimization when a radius is zero, but the barrel body UV mapping got very ugly so I assumed I had to keep all the vertices in order to preserve the mapping.
I didn't realize keeping the vertices had the downside of these creases.

Should this be resolved in this PR or later?

EDIT: After reading #5891 and #5666 a bit more it seems there might not be a solution right now that isn't at odds with:

Morten Mikkelsen clarified that the world normal and tangent must be normalized in the vertex stage and the interpolated values must not be normalized in the fragment stage.

Perhaps @superdump could provide some insight?

EDIT 2: Hm, I think my PR is lacking adding a Y component to the normal vector based on the inclination between the radiuses. I'm hoping the creases are fixed if that is added. Trying to figure out the math now.

EDIT 3: I think the normal Y should get a (c.bottom_radius - c.top_radius) / c.height value, but the creases are still visible.

@HackerFoo
Copy link
Contributor

I guess these are the creases you mentioned @HackerFoo?

Yes, that's what I expected. I don't think there is a solution other than normalizing in the fragment shader, or something similar e.g. weighted vertices.

@torsteingrindvik torsteingrindvik marked this pull request as draft November 4, 2023 09:24
Torstein Grindvik and others added 5 commits November 4, 2023 10:33
@torsteingrindvik
Copy link
Contributor Author

I was also curious as to how Godot did theirs and I found this:

image

Both have similar heights and radii, but:

Default (behind) cone:

  • 64 radial segments
  • 4 rings

Less geometry (front) cone:

  • 16 radial segments
  • 1 ring

So it seems the effect is present there but masked by having more geometry in the defaults.

I adopted those defaults now, looks like so:

cone_3d_shapes.mp4

Putting into draft since I'm unsure how to diagram it properly yet.

Signed-off-by: Torstein Grindvik <[email protected]>
@torsteingrindvik torsteingrindvik marked this pull request as ready for review November 4, 2023 10:22
@torsteingrindvik
Copy link
Contributor Author

Added an illustration done in draw.io as well as the .xml source for it such that it can be further edited in the future.

Also added some more docs.

@HackerFoo
Copy link
Contributor

So it seems the effect is present there but masked by having more geometry in the defaults.

The effect should still be visible when zoomed in on the tip of the cone. A cone with a zero normal tip vertex doesn't have this problem.

In my case 20x-1 vertices (4x radial segments, 4x rings, another ring instead of a single vertex at the tip) would affect performance, but I'm not using Bevy's shapes.

@alice-i-cecile
Copy link
Member

I would definitely like to fix this problem directly, rather than mask it with more geometry :) Defaults should be both correct and free of odd performance footguns.

@torsteingrindvik torsteingrindvik added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Nov 25, 2023
@torsteingrindvik
Copy link
Contributor Author

I'd need to delve a bit deeper to understand the implications of

A cone with a zero normal tip vertex doesn't have this problem.

and I want to work on other things right now, so putting this PR up for adoption.

@Jondolf
Copy link
Contributor

Jondolf commented Dec 7, 2023

FYI, I'm implementing this as a part of adding mesh creation support for the new primitive shapes (see #10569).

I have a working implementation of the cone with a single vertex for the tip and a zero normal:

smooth cone

Note: Unlike this PR, my implementation's default UVs map to polar coordinates (which Blender also uses). For example, a traffic cone with a white line going through it would be similar to Target's logo, which feels relatively intuitive to me.

However, this requires #10905, because the vertex normals need to be normalized in the fragment shader and not in the vertex shader. Otherwise, the NaN values will produce a black mesh.

Also, if we're going to be mathematically correct, the cone in this PR isn't actually a real cone because it doesn't always taper to a single point:

A cone is a three-dimensional geometric shape that tapers smoothly from a flat base (frequently, though not necessarily, circular) to a point called the apex or vertex.
- Wikipedia

Instead, it's a conical frustum, a truncated cone where the truncation plane is parallel to the base. Coincidentally, there is a ConicalFrustum primitive already, so I managed to easily copy your code over to use that:

conical frustum

TLDR: I'm working on this, and the code here is very useful for the conical frustum implementation. PR(s) should be ready soon after #10905 hopefully gets merged!

@torsteingrindvik
Copy link
Contributor Author

@Jondolf Very cool! Glad to hear you found something that will work well in all cases.
Since this PR was up for adoption anyway I'll close this.

github-merge-queue bot pushed a commit that referenced this pull request Dec 10, 2023
# Objective

Fixes #5891.

For mikktspace normal maps, normals must be renormalized in vertex
shaders to match the way mikktspace bakes vertex tangents and normal
maps so that the exact inverse process is applied when shading.

However, for invalid normals like `vec3<f32>(0.0, 0.0, 0.0)`, this
normalization causes NaN values, and because it's in the vertex shader,
it affects the entire triangle and causes it to be shaded as black:

![incorrectly shaded
cone](https://github.com/bevyengine/bevy/assets/57632562/3334b3a9-f72a-4a08-853e-8077a346f5c9)

*A cone with a tip that has a vertex normal of [0, 0, 0], causing the
mesh to be shaded as black.*

In some cases, normals of zero are actually *useful*. For example, a
smoothly shaded cone without creases requires the apex vertex normal to
be zero, because there is no singular normal that works correctly, so
the apex shouldn't contribute to the overall shading. Duplicate vertices
for the apex fix some shading issues, but it causes visible creases and
is more expensive. See #5891 and #10298 for more details.

For correctly shaded cones and other similar low-density shapes with
sharp tips, vertex normals of zero can not be normalized in the vertex
shader.

## Solution

Only normalize the vertex normals and tangents in the vertex shader if
the normal isn't [0, 0, 0]. This way, mikktspace normal maps should
still work for everything except the zero normals, and the zero normals
will only be normalized in the fragment shader.

This allows us to render cones correctly:

![smooth cone with some
banding](https://github.com/bevyengine/bevy/assets/57632562/6b36e264-22c6-453b-a6de-c404b314ca1a)

Notice how there is still a weird shadow banding effect in one area. I
noticed that it can be fixed by normalizing
[here](https://github.com/bevyengine/bevy/blob/d2614f2d802d0fb8000821a81553b600cc85f734/crates/bevy_pbr/src/render/pbr_functions.wgsl#L51),
which produces a perfectly smooth cone without duplicate vertices:

![smooth
cone](https://github.com/bevyengine/bevy/assets/57632562/64f9ad5d-b249-4eae-880b-a1e61e07ae73)

I didn't add this change yet, because it seems a bit arbitrary. I can
add it here if that'd be useful or make another PR though.
This was referenced Feb 9, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 13, 2024
# Objective

The `Cone` primitive should support meshing.

## Solution

Implement meshing for the `Cone` primitive. The default cone has a
height of 1 and a base radius of 0.5, and is centered at the origin.

An issue with cone meshes is that the tip does not really have a normal
that works, even with duplicated vertices. This PR uses only a single
vertex for the tip, with a normal of zero; this results in an "invalid"
normal that gets ignored by the fragment shader. This seems to be the
only approach we have for perfectly smooth cones. For discussion on the
topic, see #10298 and #5891.

Another thing to note is that the cone uses polar coordinates for the
UVs:

<img
src="https://github.com/bevyengine/bevy/assets/57632562/e101ded9-110a-4ac4-a98d-f1e4d740a24a"
alt="cone" width="400" />

This way, textures are applied as if looking at the cone from above:

<img
src="https://github.com/bevyengine/bevy/assets/57632562/8dea00f1-a283-4bc4-9676-91e8d4adb07a"
alt="texture" width="200" />

<img
src="https://github.com/bevyengine/bevy/assets/57632562/d9d1b5e6-a8ba-4690-b599-904dd85777a1"
alt="cone" width="200" />
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants