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

Reduce minimum capsule/cylinder mesh rings to 0 #82887

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Oct 6, 2023

Fixes #82850

  • Reduce the minimum amount of capsule/cylinder mesh rings to 0. I checked that both work fine with 0. The code for cylinder already supported 0, but the range hint was wrong, so the editor didn't allow less. Allowing a capsule to have 0 rings is new behavior, but I tested that it still works fine.
  • Change the ring amount setter methods to use ERR_FAIL_COND like TorusMesh::set_rings already does.

@aaronfranke aaronfranke added this to the 4.x milestone Oct 6, 2023
@aaronfranke aaronfranke requested a review from a team as a code owner October 6, 2023 01:23
@aaronfranke aaronfranke force-pushed the capsule-cylinder-rings branch from fd91aca to 70bd65d Compare October 6, 2023 01:27
@aaronfranke aaronfranke requested a review from a team as a code owner October 6, 2023 01:27
@Calinou
Copy link
Member

Calinou commented Oct 6, 2023

See #49730, which was rejected in the past. This change in particular might prove controversial (long thin triangles are slow to rasterize):

  • Change the default cylinder rings to 0.

That said, I still think changing the property hint makes sense to be reduced to 0 as not all cylinders are very long.

@aaronfranke
Copy link
Member Author

I can undo that change if it's not desired, but I don't know if I agree with the points raised in that issue. Long thin triangles can't possibly be less performant than more triangles, right?

@Calinou
Copy link
Member

Calinou commented Oct 6, 2023

Long thin triangles can't possibly be less performant than more triangles, right?

The issue is that long thin triangles (more than 8:1 aspect ratio roughly) go through a slow path in the GPU hardware. This is also the case for triangles smaller than a pixel.

This can lead to situations where rendering more triangles can actually be faster than rendering fewer triangles. This is especially common on mobile GPUs where these slow paths are really slow (due to these GPUs working with a tile-based deferred architecture, known as TBDR).

@aaronfranke aaronfranke force-pushed the capsule-cylinder-rings branch from 70bd65d to 0a577fb Compare October 6, 2023 20:53
@aaronfranke
Copy link
Member Author

@Calinou I updated the PR to not change the default value. Now it's just the allowed and range hint values.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Nov 13, 2023
@aaronfranke aaronfranke force-pushed the capsule-cylinder-rings branch from 0a577fb to 6ee9edb Compare December 6, 2023 04:29
@YuriSizov YuriSizov merged commit 9cd9831 into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@aaronfranke aaronfranke deleted the capsule-cylinder-rings branch December 8, 2023 17:58
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.

CylinderMesh does not allow for zero rings
5 participants