[no squash] Make equals method symmetric

[savilli]

Fix minetest/minetest#14132 See https://github.com/minetest/minetest/issues/14132#issuecomment-1865302265 This fix is better as it also removes some nonsense.

[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.

[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.