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

Bug fix: Check for valid E57 pose rotation #1690

Merged
merged 2 commits into from
Sep 29, 2022

Conversation

nigels-com
Copy link
Contributor

CloudCompare seems to be having trouble reading E57 that have a pose rotation quaternion set to all zeros.
Likely this is malformed according to the E57 specification, but this change adds a check.
See also: libE57Format #107

@@ -1399,6 +1399,10 @@ static bool GetPoseInformation(const e57::StructureNode& node, ccGLMatrixd& pose
bool validPoseMat = false;
if (node.isDefined("pose"))
{
CCCoreLib::SquareMatrixd rotMat(3);
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary do that here since 'validPoseMat' is false? You could simply call 'validPoseMat.toIdentity()' at the beginning of the method, just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering the case that translation is defined, but not rotation. Seems reasonable to initialise to identity matrix, in the absence of rotation?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it's enough to initialize poseMat to identity directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as discussed.

@dgirardeau
Copy link
Member

Thanks!

@dgirardeau dgirardeau merged commit 6a92ead into CloudCompare:master Sep 29, 2022
@nigels-com
Copy link
Contributor Author

Cheers! Thanks!

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

2 participants