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 meshing for Cone #11820

Merged
merged 9 commits into from
May 13, 2024
Merged

Add meshing for Cone #11820

merged 9 commits into from
May 13, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Feb 11, 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:

cone

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

texture cone

@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 11, 2024
Co-Authored By: Andrzej Głuszak <andrzej.gluszak@jetbrains.com>
self
}

/// Builds a [`Mesh`] based on the configuration in `self`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Diagrams for positions, normals, and UVs would make the math easier to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I just added better comments explaining the math and logic. I can maybe try making some simple diagrams too if I can make clean ones, but personally I think text is probably enough here.

crates/bevy_render/src/mesh/primitives/dim3/cone.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/mesh/primitives/dim3/cone.rs Outdated Show resolved Hide resolved
@rparrett
Copy link
Contributor

rparrett commented Feb 14, 2024

I'm not sure what properties would contribute to this self-shadowing-like effect specifically on the cone shape. Is some shadow code just not jiving well with the tip normal setup? Normals look fine.

image

Not a blocking comment, but we might want to spin that out into an issue.

@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 14, 2024

Yeah, the cone has this weird shadow effect:

kuva

I noticed that when originally implementing this too. The weird thing is that normalizing here seems to fix it (in my previous testing at least):

kuva

I'm not sure what's causing this. I could add the normalization there to fix it, but it feels like a pretty arbitrary fix, and it'd be good to find the root cause.

@NthTensor
Copy link
Contributor

NthTensor commented Mar 5, 2024

Someone who understands mikktspace should take a look at this. I have tried and failed to determine the source of the artifacts. If we can't solve it any other way, I suspect adding more loops (or a small loop near the tip) will mitigate the issue.

Would be really nice if we could fix the visuals before merge, because I doubt we will fix them after.

Cones are the worst.

@HackerFoo
Copy link
Contributor

For what it's worth, I don't have this problem in Noumenal, either the released version (based on Bevy 0.8) or the development version (based on somewhere around 0.12).

image

@HackerFoo
Copy link
Contributor

I reverted #5666 in my branch, though.

@NthTensor
Copy link
Contributor

I can't tell if it's just compression but that still seems to have some form of visual artifact to me. I will take your word for it, it's definitely an issue with mikktspace normalization then.

@rparrett
Copy link
Contributor

rparrett commented Mar 26, 2024

Apart from the self-shadowing or whatever, something seems off.

Cone from this PR positioned just above a cylinder:
image
Same thing exported from blender primitives rendered in scene_viewer.
image

@HackerFoo
Copy link
Contributor

HackerFoo commented Mar 26, 2024

I noticed that the shadows don’t line up in Noumenal either, so I performed an experiment in real life.

image

It seems that the lighting is more correct in the first (non-Blender) image. Now I’m tempted to print some shapes to check lighting. Did you look at the scene using Blender’s Cycles renderer?

@rparrett
Copy link
Contributor

rparrett commented Mar 27, 2024 via email

@ManevilleF ManevilleF mentioned this pull request May 1, 2024
@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Contentious There are nontrivial implications that should be thought through labels May 2, 2024
Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

None of my feedback is blocking, and IMO the visual artifacts don't seem related to the code in this PR.

@NthTensor
Copy link
Contributor

NthTensor commented May 2, 2024

That's a good point. Let's merge this and open a graphics issue. I think it was a mistake on my part to suggest blocking.

@Jondolf Jondolf 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 this pull request to the merge queue May 13, 2024
Merged via the queue into bevyengine:main with commit ac1f135 May 13, 2024
28 checks passed
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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! 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
Development

Successfully merging this pull request may close these issues.

6 participants