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

Fix divide zero issue in mat4.toSRT which causes node._lrot with NaN values. #16935

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

dumganhar
Copy link
Contributor

Re: #16929

If node._lrot.x is NaN, it will be serialized to scene data with null value.

企业微信截图_b5056fb5-4838-440b-9b90-774731779425

Changelog


Continuous Integration

This pull request:

  • needs automatic test cases check.

    Manual trigger with @cocos-robot run test cases afterward.

  • does not change any runtime related code or build configuration

    If any reviewer thinks the CI checks are needed, please uncheck this option, then close and reopen the issue.


Compatibility Check

This pull request:

  • changes public API, and have ensured backward compatibility with deprecated features.
  • affects platform compatibility, e.g. system version, browser version, platform sdk version, platform toolchain, language version, hardware compatibility etc.
  • affects file structure of the build package or build configuration which requires user project upgrade.
  • introduces breaking changes, please list all changes, affected features and the scope of violation.

@dumganhar dumganhar requested a review from minggo April 26, 2024 10:41
Copy link

Interface Check Report

This pull request does not change any public interfaces !

@dumganhar dumganhar marked this pull request as draft April 28, 2024 01:49
cocos/core/math/mat4.ts Outdated Show resolved Hide resolved
if (det < 0) {
s.x *= -1;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I don't understand, why inverse s.x?

Copy link
Contributor Author

@dumganhar dumganhar Apr 28, 2024

Choose a reason for hiding this comment

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

https://github.com/cocos/cocos-engine/blob/v3.8.3/cocos/core/math/mat4.ts#L904

It's from the original logic。

There is an explain from AI:

When the determinant is less than 0, it signifies two important physical meanings:

  1. Reversal of Orientation: The sign of the determinant indicates whether a transformation preserves the orientation of space. When the determinant is negative, it means that the transformation reverses the orientation of space, flipping from the positive direction to the negative direction. Geometrically, this implies that objects are oriented oppositely after the transformation compared to the original orientation.
  2. Volume Reversal: The absolute value of the determinant represents the scaling factor of the space volume defined by the transformation. When the determinant is negative, it implies that the transformation results in a reversal of the space volume, meaning that after the transformation, the volume of objects in space is reversed, becoming opposite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it mirrors the x-axis. As long as the conversion is consistent, it will be fine.

@dumganhar dumganhar marked this pull request as ready for review April 29, 2024 01:42
}

m3_1.m00 = m.m00 / sx;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code did not check whether the sx value is 0, and divide 0 will make m3_1.m00 be a NaN value.

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

Successfully merging this pull request may close these issues.

None yet

4 participants