Nef_3: Fix ID_support_handler logic error

[GilesBathgate]

Summary of Changes

This fixes a logical error in ID_support_handler (probably caused by a typo)

First I should explain that in this code:

cgal/Nef_3/include/CGAL/Nef_3/ID_support_handler.h

Lines 304 to 314 in 2e165ee

	int index1 = get_hash(se1->get_index());
	int index2 = get_hash(sl2->get_index());
	if(index1 < index2) {
	se->set_index(index1);
	set_hash(se1->get_index(), index1);
	set_hash(sl2->get_index(), index1);
	} else {
	se->set_index(index2);
	set_hash(se1->get_index(), index2);
	set_hash(sl2->get_index(), index2);
	}

The assignment of se1's hashed index to se1's hashed index (line 308) is a no-op H[x₁] ← H[x₁]
likewise the assignment of sl2's hashed index to sl2's hashed index (line 313) is a no-op. These no-op's don't really matter but for the sake of correctness I've removed them. The real problem is with this code:

cgal/Nef_3/include/CGAL/Nef_3/ID_support_handler.h

Line 276 in 2e165ee

set_hash(se1->twin()->get_index(), index1);

The intention here isn't a no-op the intention is for it to merge the indexes using the union find.
set_hash(se2->twin()->get_index(), index1);

I have created a failing test that passes when the fix is applied. However in a more general sense this bug doesn't create a topological, or geometric flaw, because in the correct logic is applied in another pass:

cgal/Nef_3/include/CGAL/Nef_3/SNC_SM_overlayer.h

Lines 320 to 334 in 2e165ee

	int index1 = A.get_hash(e->get_index());
	int index2 = A.get_hash(en->get_index());
	if(index2 < index1) {
	A.set_hash(e->get_index(), index2);
	e->set_index(index2);
	} else
	A.set_hash(en->get_index(), index1);
	index1 = A.get_hash(eo->get_index());
	index2 = A.get_hash(eno->get_index());
	if(index2 < index1) {
	A.set_hash(eo->get_index(), index2);
	eo->set_index(index2);
	} else
	A.set_hash(eno->get_index(), index1);

In certain cases cycling the svertices means visiting the same halfedges via each local sphere but with the indexes reversed (index2 < index1) so it falls into the branch of code where the logic is correct.

Release Management

• Affected package(s): Nef_3
• Issue(s) solved (if any): fix bug
• License and copyright ownership: CGAL

[GilesBathgate]

A better approach would be to refactor the code duplication into something like this:

int unify_indices(int idx1, int idx2) {
    int root1 = get_hash(idx1);
    int root2 = get_hash(idx2);

    if (root1 == root2) return root2;

    // Union by ID: The numerically smaller index becomes the parent
    if (root1 < root2) {
        set_hash(idx2, root1);
        return root1;
    } else {
        set_hash(idx1, root2);
        return root2;
    }
}

Then replace the repeated code with:

int index = unify_indices(se1->get_index(), se2->get_index());
se->set_index(index);

index = unify_indices(se1->twin()->get_index(), se2->twin()->get_index());
se->twin()->set_index(index);

Should that be out of scope for a simple bug fix?

[sloriot]

Successfully tested in CGAL-6.3-Ic-31

[afabri]

Can you please also add the testcase that produces an error. Does it segfault, trigger an assertion, or "only" produce a wrong result?
And I would go for the refactoring you propose. That is much nicer than a no-op needed for its side effect.

[GilesBathgate]

@afabri The test case is already added. As mentioned in the summary, the test fails without the fix applied.