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

[WIP] Fix bug in Plane transform with non-uniform scaling [4.x] #50551

Closed
wants to merge 1 commit into from
Closed

[WIP] Fix bug in Plane transform with non-uniform scaling [4.x] #50551

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
25 changes: 16 additions & 9 deletions core/math/transform_3d.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ _FORCE_INLINE_ Vector3 Transform3D::xform(const Vector3 &p_vector) const {
basis[2].dot(p_vector) + origin.z);
}

// This cheap approach does not work with non-uniform scaling
_FORCE_INLINE_ Vector3 Transform3D::xform_inv(const Vector3 &p_vector) const {
Vector3 v = p_vector - origin;

Expand All @@ -130,29 +131,35 @@ _FORCE_INLINE_ Vector3 Transform3D::xform_inv(const Vector3 &p_vector) const {
(basis.elements[0][2] * v.x) + (basis.elements[1][2] * v.y) + (basis.elements[2][2] * v.z));
}

// Note that neither the plane xform or xform_inv are particularly efficient,
// especially the xform, as it does a basis inverse. For xforming a large number
// of planes it is better to manually calculate the inverse transpose basis once
// and reuse it for each plane.
_FORCE_INLINE_ Plane Transform3D::xform(const Plane &p_plane) const {
// transform a single point on the plane
Vector3 point = p_plane.normal * p_plane.d;
Vector3 point_dir = point + p_plane.normal;
point = xform(point);
point_dir = xform(point_dir);

Vector3 normal = point_dir - point;
// use inverse transpose for correct normals with non-uniform scaling
Basis b = basis.inverse();
b.transpose();
Vector3 normal = b.xform(p_plane.normal);
Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked the math, but maybe xform_inv is actually a faster shortcut to avoid computing the full inverse transpose matrix?

Copy link
Member Author

@lawnjelly lawnjelly Jul 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is, but I believe it isn't as robust as doing proper inverse transpose. On a similar note we need to check whether using the xform_inv function on the Vector3 in Transform3D::xform_inv is up to the job.

Copy link
Member Author

@lawnjelly lawnjelly Jul 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Just confirmed, xform_inv doesn't work for Vector3 with non-uniform scales, so I'll change that.
DONE

normal.normalize();
real_t d = normal.dot(point);

real_t d = normal.dot(point);
return Plane(normal, d);
}

_FORCE_INLINE_ Plane Transform3D::xform_inv(const Plane &p_plane) const {
// transform a single point on the plane
Vector3 point = p_plane.normal * p_plane.d;
Vector3 point_dir = point + p_plane.normal;
point = xform_inv(point);
point_dir = xform_inv(point_dir);
point = basis.inverse().xform(point);

Vector3 normal = point_dir - point;
// use transpose for correct normals with non-uniform scaling
Vector3 normal = basis.transposed().xform(p_plane.normal);
normal.normalize();
real_t d = normal.dot(point);

real_t d = normal.dot(point);
return Plane(normal, d);
}

Expand Down