Make CCD read only when kinematic is enabled and vice versa #13890
Conversation
Signed-off-by: amzn-ahmadkrm <ahmadkrm@amazon.com>
| AZStd::string RigidBodyConfiguration::GetCcdTooltip() const | ||
| { | ||
| if (m_kinematic) | ||
| { | ||
| return ccdDescription + | ||
| "<b>CCD cannot be enabled if the rigid body is kinematic, set the rigid body as non-kinematic to allow changes to be made.</b>"; | ||
| } | ||
| return ccdDescription; | ||
| } | ||
|
|
||
| AZStd::string RigidBodyConfiguration::GetKinematicTooltip() const | ||
| { | ||
| if (m_ccdEnabled) | ||
| { | ||
| return kinematicDescription + | ||
| "<b>The rigid body cannot be set as Kinematic if CCD is enabled, disable CCD to allow changes to be made.</b>"; | ||
| } | ||
| return kinematicDescription; | ||
| } |
There was a problem hiding this comment.
This setup forces the system to allocate a new string every time the function is called. Since the outcomes of the function are known at compile time, it may be simpler to change this so that there is no memory allocation involved:
(I'll write examples for just the kinematic part for clarity)
- Define all possible strings as
constexpr AZStd::string_viewconstants:
constexpr AZStd::string_view kinematicDescription =
"When active, the rigid body is not affected by gravity or other forces and is moved by script. ";
const AZStd::string kinematicDescriptionReadOnly =
"When active, the rigid body is not affected by gravity or other forces and is moved by script. <b>The rigid body cannot be set as Kinematic if CCD is enabled, disable CCD to allow changes to be made.</b>"";
or even better, if the compiler lets you do it:
constexpr AZStd::string_view kinematicDescription =
"When active, the rigid body is not affected by gravity or other forces and is moved by script. ";
const AZStd::string kinematicDescriptionReadOnly =
kinematicDescription + "<b>The rigid body cannot be set as Kinematic if CCD is enabled, disable CCD to allow changes to be made.</b>"";
- Change the function signature to return a
const AZStd::string_view
const AZStd::string_view RigidBodyConfiguration::GetKinematicTooltip() const
- Just return one string or the other in the function body
return (m_ccdEnabled) ? kinematicDescriptionReadOnly : kinematicDescription;
I'm writing this off the top of my mind, so the exact types may need to be adjusted a bit (maybe a string instead of string view, or it may be even simpler to just use const char*) but I think it conveys the base idea :)
There was a problem hiding this comment.
Thank you for this :) I changed it to a string view but no the compiler didn't let me concatenate string views :( 👎
There was a problem hiding this comment.
Makes sense now that I think about it again lol. One could replace it with a macro I guess, but given it's that simple I wouldn't overcomplicate it.
Signed-off-by: amzn-ahmadkrm <ahmadkrm@amazon.com>
| @@ -104,6 +105,24 @@ namespace AzPhysics | |||
| } | |||
| } | |||
|
|
|||
| constexpr AZStd::string_view kinematicDescription = | |||
There was a problem hiding this comment.
I think the coding standards have upper camel case for constant names like this https://github.com/o3de/sig-core/blob/main/governance/Coding-Standards-and-Style-Guide.md#constant-names (e.g. KinematicDescription rather than kinematicDescription).
|
|
||
| AZStd::string_view RigidBodyConfiguration::GetCcdTooltip() const | ||
| { | ||
| return (m_kinematic) ? ccdDescriptionReadOnly : ccdDescription; |
There was a problem hiding this comment.
nitpick - not a big deal but I think the brackets are redundant here and on line 222
There was a problem hiding this comment.
I usually add them as well cause they help me with readability, but yes they are not needed.
| AZStd::string_view GetCcdTooltip() const; | ||
| AZStd::string_view GetKinematicTooltip() const; |
There was a problem hiding this comment.
I saw @AMZN-daimini's earlier comments and as the strings are known at compile time (constant) then returning a string_view is safe but it's definitely something to be careful about because if this code ever changes there's the risk of a dangling reference. Unless it's performance critical return by value is often safer but in this case I totally understand how it's justified.
Signed-off-by: amzn-ahmadkrm <ahmadkrm@amazon.com>
|
Just kicked off a build @amzn-ahmadkrm 👍 |
cgalvan
deleted the
ahmadkrm/disallow-kinematic-and-ccd-enabled-at-the-same-time
branch
Signed-off-by: amzn-ahmadkrm ahmadkrm@amazon.com
What does this PR do?
This PR updates the UI to disallow the user to set a Rigid Body Component to Kinematic mode while CCD is enabled and vice versa, these are not compatible in PhysX and results in a warning. This PR makes it so it doesn't get to the point of the PhysX warning and informs the user that only one setting can be used at once.
How was this PR tested?
Manual testing.
Please describe any testing performed.