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

Decrease the default number of rings in CylinderMesh to 0 #49730

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 19, 2021

Follow-up to #49729.

Subdividing a CylinderMesh is only needed when deforming it in a shader. The new default rings value decreases the triangle count for a CylinderMesh by a factor of 5.

The property hint was also tweaked to allow setting the number of rings to 0, which is a valid value.

Preview

Before

Subdivided cylinder

After

Non-subdivided cylinder

@akien-mga
Copy link
Member

I'm not really competent in that field so just asking to have it discussed: can it cause issues with some algorithms to have such thin elongated tris in the 0 rings version?

Subdividing a CylinderMesh is only needed when deforming it in a shader.
The new default rings value decreases the triangle count for a
CylinderMesh by a factor of 5.

The property hint was also tweaked to allow setting the number of
rings to 0, which is a valid value.
@Calinou Calinou force-pushed the cylindermesh-decrease-rings-default branch from c5569f5 to f31e103 Compare August 16, 2021 04:01
@reduz
Copy link
Member

reduz commented Aug 24, 2021

I think @BastiaanOlij should give a check to this

@clayjohn
Copy link
Member

I don't think this change is an improvement.

  • The difference in vertex count is negligible (GPUs are made to render thousands at a time, and there is no speed benefit unless you are reducing vertex count by many thousand). The only platform where the reduced vertex count may be beneficial is mobile, but this is bad for mobile for other reasons (below).

  • Long thin triangles can lead to worse performance (especially on mobile, but even on desktop)

  • Long thin triangles will look horrible with vertex lighting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants