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 rotation from one vector3 to another #13004

Merged
merged 7 commits into from
Sep 26, 2022
Merged

Add rotation from one vector3 to another #13004

merged 7 commits into from
Sep 26, 2022

Conversation

BabylonJSGuide
Copy link
Contributor

@BabylonJSGuide BabylonJSGuide commented Sep 21, 2022

First commit wherever a result was returned this is now reflected in comments. Only comments changed, if current vector was returned then this was kept and and comment not changed. Ref https://forum.babylonjs.com/t/comments-refer-to-wrong-vectors-returned-by-method-using-toref/34093/3?u=johnk

An alternative way of managing an animation along a curve by calculating the rotation needed from a set rotation to a tangent. Example PGs to follow.

@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

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

Just two comments! thanks :-)

packages/dev/core/src/Maths/math.vector.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Maths/math.vector.ts Outdated Show resolved Hide resolved
@BabylonJSGuide BabylonJSGuide changed the title Add roation from one vector3 to another Add rotation from one vector3 to another Sep 22, 2022
@RaananW RaananW requested a review from sebavan September 22, 2022 12:23
Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

changes look good to me. i'll let @sebavan give the final ok

@james-pre
Copy link
Contributor

james-pre commented Sep 22, 2022

@BabylonJSGuide I tried to make a PG with this but it does not work: https://playground.babylonjs.com/#QYA3JC#24

@BabylonJSGuide
Copy link
Contributor Author

@dr-vortex Will produce a working PG tomorrow. Will also study your PR to see if it has any advantages.

@james-pre
Copy link
Contributor

Thanks!

Might I also recommend we move the rotation between vectors related stuff to #13012?

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.

Flagging as Request changes to prevent accidental merge waiting on #13012 comments resolution.

@BabylonJSGuide
Copy link
Contributor Author

BabylonJSGuide commented Sep 23, 2022

Thanks!

Might I also recommend we move the rotation between vectors related stuff to #13012?

No for the following reasons.

  1. Each need to stand seperately on their own merits.
  2. At the moment I do not understand the way your function is used and would need to see if they were sufficient differences
  3. As I stated earlier I will study your method and get to understand it and see if it has any advantages/differences

@BabylonJSGuide
Copy link
Contributor Author

Finally undertsand @dr-vortex 's method. As a mathematician when I talk about vectors I automatically think direction, whilst for his finction he his thinking position. My function finds the rotation between to direction vectors, whilst his find the rotation needed for the mesh to travel along a straight line joining two position vectors or points. Hence the confusion.

dr-vortex examples that I now follow https://playground.babylonjs.com/#QYA3JC#28, https://playground.babylonjs.com/#QYA3JC#29

@RaananW and @sebavan I will leave it up to you whether you decide whether either, both or neither are sufficiently different to what is already possible to accept the PRs.

I have made some suggests on #13012

@BabylonJSGuide
Copy link
Contributor Author

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.

Only one tiny comment and good to go !!! thanks

packages/dev/core/src/Maths/math.vector.ts Outdated Show resolved Hide resolved
@BabylonJSGuide
Copy link
Contributor Author

@sebavan sebavan merged commit c67d38e into BabylonJS:master Sep 26, 2022
RaananW pushed a commit that referenced this pull request Dec 9, 2022
Add rotation from one vector3 to another

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

4 participants