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

Add function to get angles between two vectors (Vector3.GetAnglesBetweenVectorsForDirectionChange) #13012

Merged
merged 9 commits into from
Sep 26, 2022

Conversation

james-pre
Copy link
Contributor

@james-pre james-pre commented Sep 22, 2022

Also add prettier settings to workspace settings.

See this PR for more details.

Demo PGs:
https://playground.babylonjs.com/#QYA3JC#25
https://playground.babylonjs.com/#QYA3JC#26
(Press space to start)

@BabylonJSGuide
I have the opportunity to talk with a physicist almost every day of the week, so I asked him about the problem and worked out the same equations used. After researching how Math.atan2 works though, and found an issue with the equations: for the phi calculation, the diff.y should have been the second parameter of Math.atan2, not the first.

Added prettier settings to workspace settings.
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.

Is it not similar to #13004

cc @RaananW for prettier

@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.

@james-pre
Copy link
Contributor Author

james-pre commented Sep 22, 2022

Is it not similar to #13004

@sebavan,
How is this related to #13004?

Edit: Read #13004. I have some feedback though I will put in on that PR.

@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.

@sebavan
Copy link
Member

sebavan commented Sep 22, 2022

Is it not similar to #13004

@sebavan, How is this related to #13004?

RotationQuaternionFromOntoToRef seems to be doing the exact same finding the required rotation bewtween two vectors.

.vscode/settings.json Outdated Show resolved Hide resolved
@azure-pipelines
Copy link

@james-pre
Copy link
Contributor Author

RotationQuaternionFromOntoToRef seems to be doing the exact same finding the required rotation bewtween two vectors.

https://playground.babylonjs.com/#QYA3JC#24 It does not seem to work.

@sebavan
Copy link
Member

sebavan commented Sep 22, 2022

RotationQuaternionFromOntoToRef seems to be doing the exact same finding the required rotation bewtween two vectors.

https://playground.babylonjs.com/#QYA3JC#24 It does not seem to work.

Let s figure it out and only keep one implementation. We need to be careful and prevent duplicated features.

@james-pre
Copy link
Contributor Author

Agreed, could we maybe move it to this PR?

I'd like to know what @BabylonJSGuide thinks of this PR since we worked on it in #12942.

@james-pre james-pre changed the title Add Vector3.GetAnglesBetweenVectorsForDirectionChange Add function to get angles between two vectors (Vector3.GetAnglesBetweenVectorsForDirectionChange) Sep 22, 2022
@sebavan
Copy link
Member

sebavan commented Sep 23, 2022

Not sure it should be moved in this PR, lets compare the functionalities and keep the one closest from the goal. You can check with @BabylonJSGuide.

@BabylonJSGuide
Copy link
Contributor

BabylonJSGuide commented Sep 23, 2022

@dr-vortex now understand the difference - yours deals with positions mine with directions.

Suggestions

  1. A better name would be pitchYawRollToMoveBetweenPoints(point0, point1)

  2. Change comment from

  • Gets the angles between two vectors needed to change direction

to something like

  • gets the pitch, yaw, roll that enables a mesh/object(?) to move along a straight line between two points (vector3)
  1. Possibly add an optional roll parameter, ie pitchYawRollToMoveBetweenPoints(point0, point1, roll?) which would default to zero but could be updated by the user to affect the roll as it follows the curve.

Need to wait to see if @RaananW and @sebavan think there is sufficient differences between both and/or between existing methods for none, one or both to be approved.

@james-pre
Copy link
Contributor Author

Yes, I've been thinking about it with positions. For your 4th point, I had originally included an offset in #12942 but removed it. I agree we should wait to see if they should be implemented separately.

Added feedback from John K.
Changed to use ToRef
Removed info from settings.
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.

Ok both PR fills different needs and are valid. Let s just rename and document more in this one what the fonction is for and good to go.

Would be great to attach unit tests as well if you can to ensure we do not regress later on ?

.vscode/settings.json Outdated Show resolved Hide resolved
packages/dev/core/src/Maths/math.vector.ts Outdated Show resolved Hide resolved
@sebavan
Copy link
Member

sebavan commented Sep 23, 2022

Let s validate the naming and doc comment with @BabylonJSGuide and ready to ship :-)

@james-pre
Copy link
Contributor Author

Sounds good!

@azure-pipelines
Copy link

Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13012/merge/testResults/webgl1/index.html

If tests were successful afterwards, this report might not be available anymore.

@BabylonJSGuide
Copy link
Contributor

BabylonJSGuide commented Sep 24, 2022

After a succesful build and before approval at least the following snapshot PG should be tested

https://playground.babylonjs.com/?snapshot=refs/pull/13012/merge#S7HDGQ#5

@sebavan
Copy link
Member

sebavan commented Sep 26, 2022

@dr-vortex I ll merge as soon as @BabylonJSGuide comments have been addressed.

Thanks a lot you both !!!

@sebavan sebavan merged commit 1a734e6 into BabylonJS:master Sep 26, 2022
RaananW pushed a commit that referenced this pull request Dec 9, 2022
Add function to get angles between two vectors (Vector3.GetAnglesBetweenVectorsForDirectionChange)

Former-commit-id: e954562b579ddbb1c7a6c75589d09621c9a49551
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