Fix T89395: Assertion failure with zero-area faces in certain situations
Needs ReviewPublic
Actions

Authored by Pratik Borhade (PratikPB2123) on Jul 3 2021, 5:30 PM.

Details

Reviewers

Germano Cavalcante (mano-wii)
Campbell Barton (campbellbarton)

Maniphest Tasks

T89395: Select Interior Faces causes assertion failure with zero-area faces in certain situations

Summary

Fix T89395

Present logic removes face groups which has high cost value.
And to avoid the large face groups to be removed first because of high cost value, cost value is divided by area.

In some situation (as described in T89395), area value is 0.
That said, the function bm_interior_face_group_calc_cost returns NaN due to undefined case (0/0)

Patch will add a check to confirm the face area value.
To prevent undefined case, just not divide cost value from area. With 0 area, cost will be less which also marks the face as interior.

Diff Detail

Repository: rB Blender

Event Timeline

Pratik Borhade (PratikPB2123) requested review of this revision.Jul 3 2021, 5:30 PM

Pratik Borhade (PratikPB2123) created this revision.

Pratik Borhade (PratikPB2123) edited the summary of this revision. (Show Details)Jul 3 2021, 5:36 PM

Pratik Borhade (PratikPB2123) added a reviewer: Germano Cavalcante (mano-wii).

Pratik Borhade (PratikPB2123) added a project: Modeling.

Pratik Borhade (PratikPB2123) added a task: T89395: Select Interior Faces causes assertion failure with zero-area faces in certain situations.

Pratik Borhade (PratikPB2123) updated this revision to Diff 39096.Jul 4 2021, 7:17 AM

Pratik Borhade (PratikPB2123) edited the summary of this revision. (Show Details)

I'm not sure what this "cost" is.
Is it a coincidence that in the .blend file the cost value is always 0.0f when the area is also 0.0f?
What if faces with zero area be tracked before this function?

This code was introduced in rBa9dbc2ef, so maybe @Campbell Barton (campbellbarton) can understand this better.

Campbell Barton (campbellbarton) requested changes to this revision.Jul 5 2021, 4:21 PM

Campbell Barton (campbellbarton) added inline comments.

source/blender/editors/mesh/editmesh_select.c
2825–2828	Since there is a small chance the result wont be finite, I'd rather keep the current logic with an explicit check for NAN / INF. if (found) { const float result = cost / area; if (isfinite(result)) { return result; } } return FLT_MAX; Faces with zero area aren't going to give useful results, so I don't think it's correct to return their cost in this case.

This revision now requires changes to proceed.Jul 5 2021, 4:21 PM

Check updated as said by the reviewer.

setting fgroup_dirty[i] dirty since we are not considering the infinite cost value.
Also setting this flag because of assert hit BLI_assert(fgroup_table[i_a] != NULL);
This will also avoid the fgroup index from appending in fgroup_recalc_stack (I consider recalculation of cost is not required for the fgroup of infinite cost)

Pratik Borhade (PratikPB2123) marked an inline comment as done.Jul 14 2021, 7:12 AM

Pratik Borhade (PratikPB2123) added inline comments.Jul 14 2021, 7:46 AM

source/blender/editors/mesh/editmesh_select.c
3014	(Unsure if this is useful but I think there is no use of this check before appending the index in stack. Because already the same check is added at the logic of recalculation: `if (fgroup_table[i] != NULL && fgroup_dirty[i] == true) {`

Revision Contents
Changeset List

Path

Size

source/

blender/

editors/

mesh/

editmesh_select.c

4 lines

Diff 39079

View Options

source/blender/editors/mesh/editmesh_select.c

Fix T89395: Assertion failure with zero-area faces in certain situationsNeeds ReviewPublicActions