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

Reinstate original version of projectOnPlaneToRef with small amendment #12827

Closed
wants to merge 8 commits into from
Closed

Conversation

BabylonJSGuide
Copy link
Contributor

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@BabylonJSGuide
Copy link
Contributor Author

@azure-pipelines
Copy link

@RaananW
Copy link
Member

RaananW commented Aug 3, 2022

O #12663?

@RaananW
Copy link
Member

RaananW commented Aug 3, 2022

Is that a fix for #12663 ?

result.set(origin.x + x * hitDistance, origin.y + y * hitDistance, origin.z + z * hitDistance);
//When the ray is close to parallel to the plane return infinity vector
if (Math.abs(denom) < Math.pow(10, -10)) {
origin.addToRef(new Vector3(Infinity, Infinity, Infinity), result);
Copy link
Member

Choose a reason for hiding this comment

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

could it be result.set(Infinity, Infinity, Infinity) instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, I will check but will not be until tomorrow.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

@BabylonJSGuide the unit tests shoudl probably be changed as well to adapt to the new "old" code :-)

@BabylonJSGuide
Copy link
Contributor Author

Is that a fix for #12663 ?

It is.

@BabylonJSGuide
Copy link
Contributor Author

@BabylonJSGuide the unit tests shoudl probably be changed as well to adapt to the new "old" code :-)

I am a bit confused about new system. Need some instructions on how to deal with the two failed checks.

@sebavan
Copy link
Member

sebavan commented Aug 3, 2022

You can go in this file https://github.com/BabylonJS/Babylon.js/pull/12663/files#diff-7ad717d6f28a942a7f6dbc8c90e8071a0994061fd11be3a1b4bb4aeeff2740d0 and change the values to be covered by a unit test ?

@sebavan
Copy link
Member

sebavan commented Aug 3, 2022

@BabylonJSGuide I ll do it for you :-)

@BabylonJSGuide
Copy link
Contributor Author

@BabylonJSGuide I ll do it for you :-)

Thank you

@sebavan
Copy link
Member

sebavan commented Aug 3, 2022

Closing in favor of #12831 to have it in before the npm build, Thanks a lot @BabylonJSGuide

@sebavan sebavan closed this Aug 3, 2022
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

3 participants