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.

Sign up for GitHub

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

Jump to bottom

Support all dragAxis #15844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −1
Open

Support all dragAxis #15844

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/dev/core/src/Behaviors/Meshes/pointerDragBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ export class PointerDragBehavior implements Behavior<AbstractMesh> {

// Project delta drag from the drag plane onto the drag axis
pickedPoint.subtractToRef(this.lastDragPosition, this._tmpVector);
dragLength = Vector3.Dot(this._tmpVector, this._worldDragAxis);
dragLength = Vector3.Dot(this._tmpVector, this._worldDragAxis) / (Vector3.Dot(this._worldDragAxis, this._worldDragAxis) || 1);
Copy link
Member

Choose a reason for hiding this comment

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

Seems a little unintuitive to dot a vector with itself. To me it would be a little more clear to do:

Suggested change
dragLength = Vector3.Dot(this._tmpVector, this._worldDragAxis) / (Vector3.Dot(this._worldDragAxis, this._worldDragAxis) || 1);
dragLength = Vector3.Dot(this._tmpVector, this._worldDragAxis) / (this._worldDragAxis.lengthSquared() || 1);

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks weird to me. What is the intended computation? get lengh of this._tmpVector projected on this._worldDragAxis ? in that case, is normalizing this._worldDragAxis just enough?

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 used the dot because this is the notation you can find in Vector projection formulas but I understand it may be more readable with lengthSquared instead.

Copy link
Contributor Author

@fabsharp fabsharp Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@CedricGuillemet Yes it could work too, let me know which solution you think is the best.

Copy link
Contributor Author

@fabsharp fabsharp Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@CedricGuillemet if I normalized it's in place, so I have to clone, I know it's a negligible impact, but maybe the lengthSquared is also more readable ?

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 still thinking about it...And now I think it's more understandable with a normalize() ^^

Copy link
Contributor

@AmoebaChant AmoebaChant Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the bugfix! Instead of dividing the dot product by the length of this._worldDragAxis squared, could you please normalize this._worldDragAxis before this line? Something like:

this._worldDragAxis.normalize();

Then this line can remain unchanged:
dragLength = Vector3.Dot(this._tmpVector, this._worldDragAxis);

Thanks again!

this._worldDragAxis.scaleToRef(dragLength, this._dragDelta);
} else {
dragLength = this._dragDelta.length();
Expand Down