[no squash] Make equals method symmetric

[savilli]

Fix minetest/minetest#14132
See minetest/minetest#14132 (comment)
This fix is better as it also removes some nonsense.

[savilli]

Not a good default. Fortunately, this parameter is not used.

[sfan5]

The changes look good and I'm convinced they should work but this still fails my test code:

diff --git a/source/Irrlicht/COBJMeshFileLoader.cpp b/source/Irrlicht/COBJMeshFileLoader.cpp
--- a/source/Irrlicht/COBJMeshFileLoader.cpp
+++ b/source/Irrlicht/COBJMeshFileLoader.cpp
@@ -2,6 +2,8 @@
 // This file is part of the "Irrlicht Engine".
 // For conditions of distribution and use, see copyright notice in irrlicht.h
 
+#undef NDEBUG
+#include <cassert>
 #include "COBJMeshFileLoader.h"
 #include "IMeshManipulator.h"
 #include "IVideoDriver.h"
@@ -241,6 +243,28 @@ IAnimatedMesh* COBJMeshFileLoader::createMesh(io::IReadFile* file)
 					currMtl->RecalculateNormals=true;
 				}
 
+				const auto pv = [] (const video::S3DVertex &v) {
+					printf("P = { %a , %a , %a }\n", v.Pos.X, v.Pos.Y, v.Pos.Z);
+					printf("N = { %a , %a , %a }\n", v.Normal.X, v.Normal.Y, v.Normal.Z);
+					printf("C = %#08x\n", v.Color.color);
+					printf("T = { %a , %a }\n", v.TCoords.X, v.TCoords.Y);
+				};
+				for (auto &it : currMtl->VertMap) {
+					if (it.first == v) {
+						assert(!(it.first < v));
+						assert(!(v < it.first));
+					} if (it.first < v) {
+						assert(!(v < it.first));
+					} else {
+						if (!(v < it.first)) {
+							pv(v);
+							puts("==");
+							pv(it.first);
+							assert(false);
+						}
+					}
+				}
+
 				int vertLocation;
 				auto n = currMtl->VertMap.find(v);
 				if (n != currMtl->VertMap.end())

with

P = { 0x1p-1 , 0x1.800086p-3 , 0x1.ffffbcp-2 }
N = { 0x1p+0 , 0x0p+0 , 0x0p+0 }
C = 0xffcccccc
T = { 0x0p+0 , 0x1.4p-2 }
==
P = { 0x1p-1 , 0x1.800086p-3 , 0x1.ffffbcp-2 }
N = { 0x1p+0 , 0x0p+0 , 0x0p+0 }
C = 0xffcccccc
T = { 0x0p+0 , 0x1.4p-2 }

[sfan5]

Thinking further, isn't the problem that < is strict while == is fuzzy?

e.g. with whole numbers, tolerance of 1:
1 == 2 but at the same time 1 < 2

However if you look at the relation requirements we have a fundamental problem. It's impossible to make transitivity work with fuzzy comparisons.

e.g. 1==2 and 2==3 but !(1 == 3)
or !(1 < 2) and !(2 < 3) but 1 < 3

Basically operator== must not do fuzzy comparisons and anything depending on it needs to be reworked, right?

[numberZero]

isn't the problem that < is strict while == is fuzzy?

On numbers, both are strict (that’s why equals even exists). On vectors, both are fuzzy.

It's impossible to make transitivity work with fuzzy comparisons.

Good point.

or !(1 < 2) and !(2 < 3) but 1 < 3

That’s not transitivity. It does break transitivity of equality though (defined as !(a<b) && !(b<a)), and thus the requirements you referenced. Ironically, that breaks the only reasonable use of < on vectors (using them as map keys)...

[savilli]

Well, yes, we need to make comparison operators transitive as it's required for std::map. However, it would require removing approximate comparisons. It would be a more dangerous change because it's hard to say which code may rely on them.
I think it's better to change it in a separate PR and test that PR carefully. This PR definitely doesn't make comparison operators worse.

[savilli]

Although, I took a quick look at where comparison operators are used in the code, and looks like it needs strict comparisons everywhere and not approximate ones.
I changed comparison operators and everything seems to be working. Your test code passes too. I guess it should be safe enough to commit.
Btw, your test code has a bug. } if (it.first < v) { should be replaced with } else if (it.first < v) {.

[sfan5]

Btw, your test code has a bug. } if (it.first < v) { should be replaced with } else if (it.first < v) {.

oops lol, was that the reason it failed all the time?

[savilli]

oops lol, was that the reason it failed all the time?

No, it also failed for valid reasons like it.first == v and it.first < v at the same time.

[sfan5]

Looks good to me but I thought COBJMeshFileLoader depended on this to merge vertices properly?

[appgurueu]

I don't see why it would, for sane models at least. Wouldn't any sane write the exact same coordinates for the exact same vertex?

[savilli]

I couldn't find any .obj file where it could be a problem. There are very few files where different vertices are approximately equal in the first place. But even in those files, vertices don't need to be merged.
For example, in home_workshop_machines_3dprinter_corexy.obj there are v -0.154688 0.281094 0.199376 and v -0.154687 0.281094 0.199376 approximate equal vertices. The first is used in face f 987/889/53 989/890/53 993/891/53 991/892/53 and the second is used in f 1001/905/51 995/906/51 997/907/51 1005/908/51. It makes more sense for them to be just square faces and not "square with merged corners" faces.