Mesh: add index-independent test for mesh equality #112794
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#112794
Loading…
Reference in New Issue
No description provided.
Delete Branch "wannes.malfait/blender:mesh_isomorphism"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This adds a new function,
compare_meshes
, as a replacement forBKE_mesh_cmp
.The main benefits of the new version are the following:
Some downsides:
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
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);
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 replaceBKE_mesh_cmp
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:
Oh, I missed that, but obvious in retrospect, thanks!
@ -0,0 +251,4 @@
return value1 != value2;
}
/* The other types are based on floats. */
else if constexpr (std::is_same_v<T, float>) {
Lots of else after return in this function
@ -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);
Use functional style casts here:
float4(value1);
@ -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);
BLI_assert_unreachable
@ -0,0 +717,4 @@
return MeshMismatch::NumFaces;
}
std::cout << "domain sizes match" << std::endl;
I this printing can be removed
@ -45,0 +50,4 @@
return !(e1 == e2);
}
friend std::ostream &operator<<(std::ostream &stream, const OrderedEdge &e)
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.
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.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 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 completely replaced
BKE_mesh_cmp
now. I ended up keeping thethreshold
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.@ -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.
Is this something you are checking for?
@ -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,
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 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 tomeshes_unisomorphic
which fits better with the semantics of the return type.@ -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:
Don't use a
default
case if you want to get compiler warnings when cases are missing.@ -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> ||
We have an
is_same_any_v
utility that you might want to use.@ -0,0 +843,4 @@
return MeshMismatch::FaceTopology;
}
return {};
std::nullopt
@ -36,12 +36,13 @@ struct OrderedEdge {
return (this->v_low << 8) ^ this->v_high;
}
friend bool operator==(const OrderedEdge &e1, const OrderedEdge &e2)
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.@ -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);
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.
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.
Doesn't this
meshes_unisomorphic
test also check the same things thatBKE_mesh_cmp
checked? I had thought of this as a superset of the original comparison tests. It would be great to get rid ofBKE_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 returnstd::null_opt
if the meshes were equal, and aMeshMismatch::Indices
(or something like that) if the meshes were only isomorphic. I would then also change the name tomeshes_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 inmeshes_unisomorphic
. I don't know if vertex groups are already treated as generic attributes.Combining the two tests makes sense to me anyway.
Yes, they can be read as generic attributes, that should work.
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).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.@blender-bot build
@ -0,0 +10,4 @@
* \ingroup bke
*/
namespace blender::bke::mesh {
So far I've just used the
mesh
namespace for functions that take mesh data spans, rather than a fullMesh
. Think it's nice to keep it like that (soblender::bke::compare_meshes
) unless there's a solid reason to change it later.I got these 2 warnings after merging main into a local branch
Somehow slipped under the radar it seems. Fixed in
1e84a02