Skip to content
This repository has been archived by the owner on Oct 4, 2022. It is now read-only.

Allow arbitrarily many cylinder subdivisions #514

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yuriy0
Copy link

@yuriy0 yuriy0 commented Aug 28, 2020

PhysX does not support cylinders as primitives so they must be approximated by meshes. In order to be useable as a dynamic object, the mesh must be convex, but convex meshes are limited by number of polys and vertices.

Currently the cylinder approximation will always generate one convex mesh to represent the cylinder. For many cylinders the vertex limit still gives fairly precise approximations. This might not be the case for large cylinders where the accuracy of dynamics is important.

The changes the function which approximates cylinder geomtry in Physx: if the cylinder mesh exceeds the max number of PhysX convex mesh polys, instead split it into several shapes.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

If the cylinder mesh exceeds the max number of PhysX convex mesh polys, split it into several shapes
@AMZN-puvvadar
Copy link

Hey @yuriy0 ,

We took a look at this change are unable to accept it for the following reasons.

  • EditorShapeColliderComponent::m_subdivisionCount is still u8. Is there a reason for this?
  • If the type should be changed, a converter will be required to gracefully de-serialize existing levels.
  • Is there a reason there is no upper bound on the number of subdivisions to avoid massive number of shapes from being created?
  • Alternatively, Utils::MaxFrustumSubdivisions could be removed (but this is subject to how existing unit tests are updated).
  • No test coverage for what is a fairly big yet bespoke change that may impact other customers.
  • The change presently invalidates some existing tests (e.g. FrustumCreatePoints_CreateFromInvalidMaxSubdivisions_ReturnsEmpty)

If you could clarify or address the above, we'd be happy to take another look. Thanks again for taking the time to submit!

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

Successfully merging this pull request may close these issues.

3 participants