Make is_equal_approx separate and make == exact again #32477

aaronfranke · 2019-10-02T01:26:13Z

This PR fixes the concerns @reduz has about #18992, and fixes #32604

My initial reasoning is that end-user code would almost always want to have equality checks be safe, so that would make sense to have as the default behavior of == for vectors. One concern about this is with performance, but if that was the only concern, performance-critical code could have a separate method for checking this (and some code could also switch to integer vectors later on, too).

This PR restores the old (3.1) functionality of == for vectors etc, and instead introduces some is_equal_approx methods for comparing them. The main reason I think this is the best course of action is that, as @reduz mentioned, we don't want to hide this functionality from the user. It makes sense to require users to be aware of approximate equality and consciously choose it, instead of it just being the default choice for ==. Aside from that, it does also have the advantage of allowing code to run as fast as before.

The first commit adds is_equal_approx internally (exposed in C#) to the types (so, nothing breaking). There is a pre-existing exposed is_equal_approx in Basis that should be replaced in 4.0, I've placed TODO notes in there. The second commit switches internal code that depends on approximate vector equality (such as the lines in #31395) to use the is_equal_approx methods (so, nothing breaking). The third commit exposes is_equal_approx methods and reverts == to check for exact equality again (same behavior as 3.1, so 3.1 projects will still work the same).

One thing I wasn't sure about is with > and >= etc. I think it's desirable to leave these as they currently are, but I could be wrong.

As a side note, this is my 100th pull request on GitHub (and my 51st to the Godot repo). 🎉

core/math/plane.cpp

akien-mga · 2019-10-08T14:26:12Z

I would really prefer a PR that only addresses the issues raised in #18992.

It's unclear whether many of the added approximation checks are necessary, and as mentioned by @reduz, they might impair the compiler's ability to perform optimizations.

AFAICT reverting the changes to operators in Plane, Vector2 and Vector3 should be enough for now. The rest can be added in a second step, in a PR that would likely not be merged for 3.2 as it's too late.

reduz · 2019-10-08T14:32:35Z

core/math/basis.cpp

@@ -557,28 +557,9 @@ void Basis::set_euler_yxz(const Vector3 &p_euler) {
 	*this = ymat * xmat * zmat;
 }

-bool Basis::is_equal_approx(const Basis &a, const Basis &b, real_t p_epsilon) const {


The epsilon is needed afaik because it can be used with different values for different use cases, when matrix is not of unit size. Check those that use is_equal_approx_ratio and leave them alone.

When the matrix is not of unit size, is_equal_approx now handles such cases, since is_equal_approx is called on each component Vector3.

The epsilon override for the pre-existing is_equal_approx is not actually used anywhere in the code base.

I undid changes to is_equal_approx_ratio for Basis, but it's also not actually used anywhere in the code base.

reduz

leave == comparisons alone if they did not use is_equal_approx originally.

reduz · 2019-10-08T14:36:24Z

core/math/delaunay.h

@@ -80,11 +80,11 @@ class Delaunay2D {
 	}

 	static bool edge_compare(const Vector<Vector2> &p_vertices, const Edge &p_a, const Edge &p_b) {
-		if (p_vertices[p_a.edge[0]] == p_vertices[p_b.edge[0]] && p_vertices[p_a.edge[1]] == p_vertices[p_b.edge[1]]) {


In many cases there is a good reason for this to have equal signs. When dealing with geometry, vertices may be very close but not be the same one, or sometimes they may be hashed, and this will break it.

Please do not replace == comparisons by is_equal_approx if they did not originally do this.

They were not originally like this. This is a reversion of https://github.com/godotengine/godot/pull/31395/files#diff-1c96941701be95512a1964a0e4e5c5c4L83

reduz · 2019-10-08T14:36:51Z

core/math/plane.cpp

@@ -156,11 +153,6 @@ bool Plane::intersects_segment(const Vector3 &p_begin, const Vector3 &p_end, Vec

 /* misc */

-bool Plane::is_almost_like(const Plane &p_plane) const {


Please do not break compatibility, leave the old function with a deprecate warning.

This is only used internally, @akien-mga told me to remove it.

Yeah it's ok to remove this one.

reduz · 2019-10-08T14:37:24Z

modules/csg/csg.cpp

@@ -242,7 +242,7 @@ void CSGBrushOperation::BuildPoly::_clip_segment(const CSGBrush *p_brush, int p_
 	//check if edge and poly share a vertex, of so, assign it to segment_idx
 	for (int i = 0; i < points.size(); i++) {
 		for (int j = 0; j < 2; j++) {
-			if (segment[j] == points[i].point) {


again, I dont remember how this was originally, but if it did not use is_equal_approx, then dont replace it.

They were originally is_zero_approx(a.distance_to(b)) which is functionally identical to checking for approximate equality except that the old code doesn't work with large vectors. This is necessary to keep #22988 fixed.

reduz · 2019-10-08T14:37:48Z

modules/csg/csg.cpp

@@ -310,7 +310,7 @@ void CSGBrushOperation::BuildPoly::_clip_segment(const CSGBrush *p_brush, int p_
 			Vector2 edgeseg[2] = { points[edges[i].points[0]].point, points[edges[i].points[1]].point };
 			Vector2 closest = Geometry::get_closest_point_to_segment_2d(segment[j], edgeseg);

-			if (closest == segment[j]) {
+			if (closest.is_equal_approx(segment[j])) {


Same as always, check what this was originally.

reduz · 2019-10-08T14:39:55Z

ack stupid github, dont merge this, i only approved the last commit.

This commit adds exposed behavior for C#

Internal changes only

This commit changes behavior for GDScript and C#. Also did some organizing of the order to logically group related methods, mostly for Rect2 and AABB.

akien-mga · 2019-11-07T13:54:12Z

As mentioned, I would really have preferred a PR only addressing the necessary bug fixes, i.e. undoing approximate equality checks added where exact checks are meant to be used.

This PR does that, and adds more utility methods, some cleanup, some refactoring... it makes it big and quite inappropriate to merge at such a late stage in development.

Still, the bug fixes are important and I don't feel like doing a slimmed down version of the PR with only fixes myself. After the last revision and discussing it with @reduz, we agree that the extra enhancements should likely be fine, so let's merge.

akien-mga · 2019-11-07T13:54:19Z

Thanks!

aaronfranke · 2019-11-07T14:22:07Z

@akien-mga For the record, having only the changes to the == operator would break other things, and I'm not keen on doing that, or creating workarounds in the affected code, just to minimize changes. I'd rather create a comprehensive solution, so I did.

If any further issues are discovered due to merging such a large PR I'll get on it immediately.

aaronfranke requested review from BastiaanOlij and neikeq as code owners October 2, 2019 01:26

aaronfranke force-pushed the equal-approx-separate branch from 320e902 to 0a3315b Compare October 2, 2019 01:48

akien-mga reviewed Oct 2, 2019

View reviewed changes

core/math/plane.cpp Outdated Show resolved Hide resolved

akien-mga added enhancement topic:core labels Oct 2, 2019

akien-mga added this to the 3.2 milestone Oct 2, 2019

akien-mga requested a review from reduz October 2, 2019 05:34

akien-mga added the for pr meeting label Oct 2, 2019

aaronfranke force-pushed the equal-approx-separate branch from 0a3315b to 726faa2 Compare October 2, 2019 20:44

reduz requested changes Oct 8, 2019

View reviewed changes

reduz approved these changes Oct 8, 2019

View reviewed changes

akien-mga requested a review from reduz October 8, 2019 14:52

aaronfranke added 3 commits October 14, 2019 16:46

Make is_equal_approx separate for structures

86922ff

This commit adds exposed behavior for C#

Replace vector == and is_zero_approx(distance) with is_equal_approx

aeb7075

Internal changes only

Expose is_equal_approx and restore == to be exact again

218f38c

This commit changes behavior for GDScript and C#. Also did some organizing of the order to logically group related methods, mostly for Rect2 and AABB.

aaronfranke force-pushed the equal-approx-separate branch from 726faa2 to 218f38c Compare October 14, 2019 21:08

aaronfranke mentioned this pull request Nov 5, 2019

Failure of mathematical analysis in 3D scene #32604

Closed

akien-mga added bug regression labels Nov 7, 2019

akien-mga merged commit 77816fe into godotengine:master Nov 7, 2019

aaronfranke deleted the equal-approx-separate branch November 7, 2019 14:22

aaronfranke mentioned this pull request Mar 29, 2020

Change Basis is_equal_approx to use instance method #37403

Merged

aaronfranke mentioned this pull request Sep 8, 2020

Texture UV gets messed up when importing from Blender #41090

Closed

Calinou removed the for pr meeting label Nov 21, 2021

aaronfranke added the performance label Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make is_equal_approx separate and make == exact again #32477

Make is_equal_approx separate and make == exact again #32477

aaronfranke commented Oct 2, 2019 •

edited

Loading

akien-mga commented Oct 8, 2019

reduz Oct 8, 2019

aaronfranke Oct 8, 2019 •

edited

Loading

reduz left a comment

reduz Oct 8, 2019

aaronfranke Oct 8, 2019 •

edited

Loading

reduz Oct 8, 2019

aaronfranke Oct 8, 2019

akien-mga Oct 11, 2019

reduz Oct 8, 2019

aaronfranke Oct 8, 2019

reduz Oct 8, 2019

reduz commented Oct 8, 2019

akien-mga commented Nov 7, 2019

akien-mga commented Nov 7, 2019

aaronfranke commented Nov 7, 2019

		@@ -156,11 +153,6 @@ bool Plane::intersects_segment(const Vector3 &p_begin, const Vector3 &p_end, Vec

		/* misc */

		bool Plane::is_almost_like(const Plane &p_plane) const {

Make is_equal_approx separate and make == exact again #32477

Make is_equal_approx separate and make == exact again #32477

Conversation

aaronfranke commented Oct 2, 2019 • edited Loading

akien-mga commented Oct 8, 2019

Choose a reason for hiding this comment

aaronfranke Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

reduz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronfranke Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reduz commented Oct 8, 2019

akien-mga commented Nov 7, 2019

akien-mga commented Nov 7, 2019

aaronfranke commented Nov 7, 2019

aaronfranke commented Oct 2, 2019 •

edited

Loading

aaronfranke Oct 8, 2019 •

edited

Loading

aaronfranke Oct 8, 2019 •

edited

Loading