Mesh: add index-independent test for mesh equality #112794

Merged
Hans Goudey merged 46 commits from wannes.malfait/blender:mesh_isomorphism into main 2023-11-27 16:10:52 +01:00
Member

This adds a new function, compare_meshes, as a replacement for BKE_mesh_cmp.

The main benefits of the new version are the following:

  • The code is written in c++, and makes use of the new attributes API.
  • It adds an additional check, to see if the meshes only differ by their indices. This is useful to verify correctness of new algorithmic changes in mesh code, which might produce mesh elements in a different order than the original algorithm. The tests will still fail, but the error will show that the indices changed.

Some downsides:

  • The code is more complex, due to having to be index-independent.
  • The code is probably slower due to having to do comparisons "index-independently". I have not tested this, as correctness was my priority for this patch. A future update could look to improve the speed, if that is desired.
  • This is technically a breaking API change, since it changes the returned values of rna_Mesh_unit_test_compare. I don't think that there are many people (if any) using this, besides our own unit tests.

All tests that pass with BKE_mesh_cmp still pass with the new version.

NOTE:
Currently, mesh edge indices are allowed to be different in the comparison, because BKE_mesh_cmp also allowed this. There are some tests which would fail otherwise. These tests should be updated, and then the corresponding code as well.

I wrote up a more detailed explanation of the algorithm here: https://hackmd.io/@bo-JY945TOmvepQ1tAWy6w/SyuaFtay6

This adds a new function, `compare_meshes`, as a replacement for `BKE_mesh_cmp`. The main benefits of the new version are the following: - The code is written in c++, and makes use of the new attributes API. - It adds an additional check, to see if the meshes only differ by their indices. This is useful to verify correctness of new algorithmic changes in mesh code, which might produce mesh elements in a different order than the original algorithm. The tests will still fail, but the error will show that the indices changed. Some downsides: - The code is more complex, due to having to be index-independent. - The code is probably slower due to having to do comparisons "index-independently". I have not tested this, as correctness was my priority for this patch. A future update could look to improve the speed, if that is desired. - This is technically a breaking API change, since it changes the returned values of `rna_Mesh_unit_test_compare`. I don't think that there are many people (if any) using this, besides our own unit tests. All tests that pass with `BKE_mesh_cmp` still pass with the new version. **NOTE:** Currently, mesh edge indices are allowed to be different in the comparison, because `BKE_mesh_cmp` also allowed this. There are some tests which would fail otherwise. These tests should be updated, and then the corresponding code as well. I wrote up a more detailed explanation of the algorithm here: https://hackmd.io/@bo-JY945TOmvepQ1tAWy6w/SyuaFtay6
Wannes Malfait added 18 commits 2023-09-24 00:37:22 +02:00
Wannes Malfait added the
Interest
Automated Testing
label 2023-09-24 00:39:53 +02:00
Wannes Malfait added 2 commits 2023-09-24 12:01:46 +02:00
Hans Goudey requested changes 2023-10-11 12:00:28 +02:00
Hans Goudey left a comment
Member

Thanks for working on this!

I think the next step is to use this for mesh regression testing (geometry nodes and modifiers tests at least) and replace BKE_mesh_cmp. I think it would be nice to do that sooner rather than later to avoid this code being unused for too long.

I requested changes mostly for a few style comments.

Thanks for working on this! I think the next step is to use this for mesh regression testing (geometry nodes and modifiers tests at least) and replace `BKE_mesh_cmp`. I think it would be nice to do that sooner rather than later to avoid this code being unused for too long. I requested changes mostly for a few style comments.
@ -0,0 +18,4 @@
* Convert the mismatch to a human-readable string for display.
*/
const char *mismatch_to_string(const MeshMismatch &mismatch);
Member

It would be nice to add a function that returns whether a MeshMismatch is considered a test failure. That would be the best way for this code to replace BKE_mesh_cmp

It would be nice to add a function that returns whether a `MeshMismatch` is considered a test failure. That would be the best way for this code to replace `BKE_mesh_cmp`
Author
Member

I think any of these mismatches should be considered test failures. If any of these occur, then there is no way to re-index the meshes such that they become equal. So, to test for failures it would be like this:

const optional<MeshMismatch> mismatch = meshes_isomorphic(mesh1, mesh2);
if (mismatch) {
    std::cout << "Test failed: "<<mismatch_to_string(mismatch) << std::endl;
}
I think any of these mismatches should be considered test failures. If any of these occur, then there is no way to re-index the meshes such that they become equal. So, to test for failures it would be like this: ```cpp const optional<MeshMismatch> mismatch = meshes_isomorphic(mesh1, mesh2); if (mismatch) { std::cout << "Test failed: "<<mismatch_to_string(mismatch) << std::endl; } ```
Member

Oh, I missed that, but obvious in retrospect, thanks!

Oh, I missed that, but obvious in retrospect, thanks!
wannes.malfait marked this conversation as resolved
@ -0,0 +251,4 @@
return value1 != value2;
}
/* The other types are based on floats. */
else if constexpr (std::is_same_v<T, float>) {
Member

Lots of else after return in this function

Lots of else after return in this function
wannes.malfait marked this conversation as resolved
@ -0,0 +271,4 @@
return false;
}
else if constexpr (std::is_same_v<T, math::Quaternion> || std::is_same_v<T, ColorGeometry4f>) {
const float4 value1_f = static_cast<float4>(value1);
Member

Use functional style casts here: float4(value1);

Use functional style casts here: `float4(value1);`
wannes.malfait marked this conversation as resolved
@ -0,0 +674,4 @@
* the mesh is of good enough quality that this doesn't happen. Otherwise, the logic becomes a
* lot more difficult. */
if (matching_verts.size() != 1) {
BLI_assert(false);
Member

BLI_assert_unreachable

`BLI_assert_unreachable`
wannes.malfait marked this conversation as resolved
@ -0,0 +717,4 @@
return MeshMismatch::NumFaces;
}
std::cout << "domain sizes match" << std::endl;
Member

I this printing can be removed

I this printing can be removed
wannes.malfait marked this conversation as resolved
@ -45,0 +50,4 @@
return !(e1 == e2);
}
friend std::ostream &operator<<(std::ostream &stream, const OrderedEdge &e)
Member

Typically we'd define these in a .cc file to avoid the need to include everywhere. Just parsing that was significantly slowing down compile times.

Typically we'd define these in a .cc file to avoid the need to include <ostream> everywhere. Just parsing that was significantly slowing down compile times.
Author
Member

I moved the implementation of these operators to a new ordered_edge.cc file. Not 100% sure if that is what you meant. I had to declare them in the header anyway, because otherwise I got linker errors and "missing declaration" warnings.

I moved the implementation of these operators to a new `ordered_edge.cc` file. Not 100% sure if that is what you meant. I had to declare them in the header anyway, because otherwise I got linker errors and "missing declaration" warnings.
wannes.malfait marked this conversation as resolved
Author
Member

I think the next step is to use this for mesh regression testing (geometry nodes and modifiers tests at least) and replace BKE_mesh_cmp. I think it would be nice to do that sooner rather than later to avoid this code being unused for too long.

Do you want me to replace the rna version of BKE_mesh_cmp with this, or add a separate one, and then change the python code? You mean that I should also do this in this patch right?

I think I should probably add the treshold as a parameter to the function (like there is in BKE_mesh_cmp), but as far as I could tell, the treshold is never explicitly set in the python testing code (always left as default), so I don't know if that is really necessary.

I saw that Jacques wrote some "mesh randomizer" code, so I will try to use those to test this algorithm a bit more.

Will probably have some time this weekend to work on this.

> I think the next step is to use this for mesh regression testing (geometry nodes and modifiers tests at least) and replace `BKE_mesh_cmp`. I think it would be nice to do that sooner rather than later to avoid this code being unused for too long. Do you want me to replace the rna version of `BKE_mesh_cmp` with this, or add a separate one, and then change the python code? You mean that I should also do this in this patch right? I think I should probably add the treshold as a parameter to the function (like there is in `BKE_mesh_cmp`), but as far as I could tell, the treshold is never explicitly set in the python testing code (always left as default), so I don't know if that is really necessary. I saw that Jacques wrote some "mesh randomizer" code, so I will try to use those to test this algorithm a bit more. Will probably have some time this weekend to work on this.
Member

I think replacing BKE_mesh_cmp without changing the API would be nice (though removing the threshold parameter sounds reasonable). The function exposed to Python is basically just meant for regression tests anyway. Doing the change now would make the nice case for why we have this new code :)

I think replacing `BKE_mesh_cmp` without changing the API would be nice (though removing the threshold parameter sounds reasonable). The function exposed to Python is basically just meant for regression tests anyway. Doing the change now would make the nice case for why we have this new code :)
Wannes Malfait added 10 commits 2023-10-14 23:08:44 +02:00
Author
Member

I completely replaced BKE_mesh_cmp now. I ended up keeping the threshold parameter, since there are some tests that set it explicitly. All the current tests still pass with the new comparison function, which makes me more confident about the code.

In some of my own tests with the debug_randomize functions, I did see some small problems for a few edge cases. I think I know how to resolve those, but will leave that for tomorrow. (I think the problem lies with the sorting of float2/float3/float4 not being very stable.

I completely replaced `BKE_mesh_cmp` now. I ended up keeping the `threshold` parameter, since there are some tests that set it explicitly. All the current tests still pass with the new comparison function, which makes me more confident about the code. In some of my own tests with the `debug_randomize` functions, I did see some small problems for a few edge cases. I think I know how to resolve those, but will leave that for tomorrow. (I think the problem lies with the sorting of float2/float3/float4 not being very stable.
Wannes Malfait added 1 commit 2023-10-15 16:21:08 +02:00
This is needed, because sorting them at once is not stable enough. By
sorting per component, and then computing the set ids each time, we
get a much more stable result.
Wannes Malfait added 1 commit 2023-10-15 16:25:28 +02:00
Jacques Lucke requested review from Jacques Lucke 2023-10-15 20:43:54 +02:00
Jacques Lucke requested changes 2023-10-15 21:27:37 +02:00
@ -0,0 +29,4 @@
* time algorithm is known). Because we have more information than just connectivity (attributes),
* we can compute it in a more reasonable time in most cases.
*
* \warning This assumes that the mesh is of decent quality: no zero-size edges or faces.
Member

Is this something you are checking for?

Is this something you are checking for?
wannes.malfait marked this conversation as resolved
@ -0,0 +31,4 @@
*
* \warning This assumes that the mesh is of decent quality: no zero-size edges or faces.
*/
std::optional<MeshMismatch> meshes_isomorphic(const Mesh &mesh1,
Member

I find the return value a bit backwards, because the function name sounds like it returns something truthy if the meshes are isomorphic, which is not the case here.

I find the return value a bit backwards, because the function name sounds like it returns something truthy if the meshes are isomorphic, which is not the case here.
Author
Member

I really wanted to use something like Result<(), MeshMismatch> in rust, but ended up with this instead. I completely agree with you, and will change the name to meshes_unisomorphic which fits better with the semantics of the return type.

I really wanted to use something like `Result<(), MeshMismatch>` in rust, but ended up with this instead. I completely agree with you, and will change the name to `meshes_unisomorphic` which fits better with the semantics of the return type.
wannes.malfait marked this conversation as resolved
@ -0,0 +58,4 @@
return "The sets of attribute ids are different";
case MeshMismatch::AttributeTypes:
return "Some attributes with the same name have different types";
default:
Member

Don't use a default case if you want to get compiler warnings when cases are missing.

Don't use a `default` case if you want to get compiler warnings when cases are missing.
wannes.malfait marked this conversation as resolved
@ -0,0 +233,4 @@
const float threshold,
const int component_i)
{
if constexpr (std::is_same_v<T, int> || std::is_same_v<T, int2> || std::is_same_v<T, bool> ||
Member

We have an is_same_any_v utility that you might want to use.

We have an `is_same_any_v` utility that you might want to use.
wannes.malfait marked this conversation as resolved
@ -0,0 +843,4 @@
return MeshMismatch::FaceTopology;
}
return {};
Member

std::nullopt

`std::nullopt`
wannes.malfait marked this conversation as resolved
@ -36,12 +36,13 @@ struct OrderedEdge {
return (this->v_low << 8) ^ this->v_high;
}
friend bool operator==(const OrderedEdge &e1, const OrderedEdge &e2)
Member

The comparison operators should not be moved out of the header because they need to be inlined.
The operator<< should be moved to the implementation file though.

The comparison operators should not be moved out of the header because they need to be inlined. The `operator<<` should be moved to the implementation file though.
wannes.malfait marked this conversation as resolved
@ -39,2 +40,3 @@
{
const char *ret = BKE_mesh_cmp(mesh, mesh2, threshold);
using namespace blender::bke::mesh;
const std::optional<MeshMismatch> mismatch = meshes_isomorphic(*mesh, *mesh2, threshold);
Member

Not quite sure I fully understand. I think it's still very important that we are table to check two meshes are exactly the same, not just isomorphic. Regression tests should fail when they are not exactly the same. Checking for an isomorphism is mostly just a utility that helps us track down the source of the issue more easily.

Changing indices should never be done accidentally.

Not quite sure I fully understand. I think it's still very important that we are table to check two meshes are exactly the same, not just isomorphic. Regression tests should fail when they are not exactly the same. Checking for an isomorphism is mostly just a utility that helps us track down the source of the issue more easily. Changing indices should never be done accidentally.
Author
Member

I think you are right. I will change it so that the normal comparison is run, but if the test fails, then a check is done to see if they are isomorphic.

I think you are right. I will change it so that the normal comparison is run, but if the test fails, then a check is done to see if they are isomorphic.
wannes.malfait marked this conversation as resolved
Wannes Malfait added 8 commits 2023-10-17 10:36:51 +02:00
Member

Doesn't this meshes_unisomorphic test also check the same things that BKE_mesh_cmp checked? I had thought of this as a superset of the original comparison tests. It would be great to get rid of BKE_mesh_cmp is possible.

The other changes make sense.

Doesn't this `meshes_unisomorphic` test also check the same things that `BKE_mesh_cmp` checked? I had thought of this as a superset of the original comparison tests. It would be great to get rid of `BKE_mesh_cmp` is possible. The other changes make sense.
Author
Member

Doesn't this meshes_unisomorphic test also check the same things that BKE_mesh_cmp checked? I had thought of this as a superset of the original comparison tests. It would be great to get rid of BKE_mesh_cmp is possible.

The other changes make sense.

At the moment the meshes_unisomorphic function only tests if there is some re-ordering of the indices such that the meshes become equal, but it doesn't really test if the meshes were equal already without re-ordering the indices. Given the way the code is currently written, I should be able to determine if a re-ordering was really necessary, or if the meshes are equal. With that I could make it return std::null_opt if the meshes were equal, and a MeshMismatch::Indices (or something like that) if the meshes were only isomorphic. I would then also change the name to meshes_different.

@HooglyBoogly @JacquesLucke what do you think?

One small thing I'm concerned about is if the checking between Vertex Groups that happens in BKE_mesh_cmp is also handled by just looking at the mesh attributes in meshes_unisomorphic. I don't know if vertex groups are already treated as generic attributes.

> Doesn't this `meshes_unisomorphic` test also check the same things that `BKE_mesh_cmp` checked? I had thought of this as a superset of the original comparison tests. It would be great to get rid of `BKE_mesh_cmp` is possible. > > The other changes make sense. At the moment the `meshes_unisomorphic` function only tests if there is some re-ordering of the indices such that the meshes become equal, but it doesn't really test if the meshes were equal already without re-ordering the indices. Given the way the code is currently written, I should be able to determine if a re-ordering was really necessary, or if the meshes are equal. With that I could make it return `std::null_opt` if the meshes were equal, and a `MeshMismatch::Indices` (or something like that) if the meshes were only isomorphic. I would then also change the name to `meshes_different`. @HooglyBoogly @JacquesLucke what do you think? One small thing I'm concerned about is if the checking between Vertex Groups that happens in `BKE_mesh_cmp` is also handled by just looking at the mesh attributes in `meshes_unisomorphic`. I don't know if vertex groups are already treated as generic attributes.
Member

Combining the two tests makes sense to me anyway.

I don't know if vertex groups are already treated as generic attributes.

Yes, they can be read as generic attributes, that should work.

Combining the two tests makes sense to me anyway. >I don't know if vertex groups are already treated as generic attributes. Yes, they can be read as generic attributes, that should work.
Wannes Malfait added 2 commits 2023-10-19 16:45:59 +02:00
Author
Member

I added the changes I talked about. The function can now detect if the meshes are just isomorphic or exactly equal.

Still have to remove the BKE_mesh_cmp function. (Wanted your approval for this first).

NOTE At the moment I removed the check for edge index equality, because apparently a lot of the current tests have different edge indices. I think this is because BKE_mesh_cmp just places all edges in a set, and compares the sets (which obviously doesn't detect changes in indices).

I added the changes I talked about. The function can now detect if the meshes are just isomorphic or exactly equal. Still have to remove the `BKE_mesh_cmp` function. (Wanted your approval for this first). **NOTE** At the moment I removed the check for edge index equality, because apparently a lot of the current tests have different edge indices. I think this is because `BKE_mesh_cmp` just places all edges in a set, and compares the sets (which obviously doesn't detect changes in indices).
Member

Still have to remove the BKE_mesh_cmp function. (Wanted your approval for this first).

Sorry for the delay! That sounds great to me. And yeah, I guess we'll have to update test results for edge indices.

>Still have to remove the BKE_mesh_cmp function. (Wanted your approval for this first). Sorry for the delay! That sounds great to me. And yeah, I guess we'll have to update test results for edge indices.
Wannes Malfait added 2 commits 2023-11-20 14:59:58 +01:00
Remove BKE_mesh_cmp
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
e0970b3ce1
Author
Member

Sorry for the delay! That sounds great to me. And yeah, I guess we'll have to update test results for edge indices.

No problem, have been busy with other stuff as well.

I merged main deleted BKE_mesh_cmp, and updated the patch description to be more accurate to the current version of the patch. Tests still pass.

> Sorry for the delay! That sounds great to me. And yeah, I guess we'll have to update test results for edge indices. No problem, have been busy with other stuff as well. I merged main deleted `BKE_mesh_cmp`, and updated the patch description to be more accurate to the current version of the patch. Tests still pass.
Member

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2023-11-20 20:22:16 +01:00
@ -0,0 +10,4 @@
* \ingroup bke
*/
namespace blender::bke::mesh {
Member

So far I've just used the mesh namespace for functions that take mesh data spans, rather than a full Mesh. Think it's nice to keep it like that (so blender::bke::compare_meshes) unless there's a solid reason to change it later.

So far I've just used the `mesh` namespace for functions that take mesh data spans, rather than a full `Mesh`. Think it's nice to keep it like that (so `blender::bke::compare_meshes`) unless there's a solid reason to change it later.
wannes.malfait marked this conversation as resolved
Wannes Malfait added 1 commit 2023-11-21 11:10:47 +01:00
Jacques Lucke approved these changes 2023-11-27 15:43:46 +01:00
Hans Goudey added 1 commit 2023-11-27 16:05:34 +01:00
Hans Goudey merged commit b162281caf into main 2023-11-27 16:10:52 +01:00
Wannes Malfait deleted branch mesh_isomorphism 2023-11-27 18:10:39 +01:00
Contributor

I got these 2 warnings after merging main into a local branch

image

I got these 2 warnings after merging main into a local branch ![image](/attachments/92d2060b-92bf-43dc-a2bf-0cda3ec1f26f)
142 KiB
Author
Member

I got these 2 warnings after merging main into a local branch

image

Somehow slipped under the radar it seems. Fixed in 1e84a02

> I got these 2 warnings after merging main into a local branch > > > ![image](/attachments/92d2060b-92bf-43dc-a2bf-0cda3ec1f26f) Somehow slipped under the radar it seems. Fixed in 1e84a02
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#112794
No description provided.