Page MenuHome

Fix Grab Active Vertex preview drawing repeated edges
AcceptedPublic

Authored by Pablo Dobarro (pablodp606) on Aug 19 2020, 8:56 PM.

Details

Summary

When using Grab Active Vertex with modifiers active, the code that
generates the wireframe preview relying on a hack which was estimating
the amount of maxium iterations the floodfill operation was going to do
and then adding all connections possible.
Now it uses an edgeset to keep track of the edges that were added in
order to not draw them twice and know beforehand the maximun
amount of edges that are going to be drawn.

Diff Detail

Repository
rB Blender
Branch
fix-wireframe-active-vertex (branched from master)
Build Status
Buildable 9658
Build 9658: arc lint + arc unit

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Aug 19 2020, 8:56 PM

How often this code is run? On every redraw? Once on redarw after edits?

On every redraw if the active vertex changed

Is not too bad. Although still feels a bit too expensive.
Also not sure why bitmap wasn't detecting that the edge was drawn twice. Can you elaborate on this? Because from reading, estimation of number of edges is one thing, but then there is a bitmap which was supposed to guard double-drawing.
And last but not least, please also provide information how to reproduce the issue.

@Clément Foucault (fclem), hereby i declare you my second pair of eyes and thought here ;)

The bitmap is used to control the floodfill operation and guarantee that a vertex is not added to check its neighbors twice and avoid entering an infinite loop, but an edge can still be visited from both vertices and added twice to the preview list if there is not a edgeset to keep track of which edges were used.

@Pablo Dobarro (pablodp606), I would suggest to add such information to the commit message.

If the code is only run once the active vertex changed (and does not happen on every redraw all the time) it's fine with me.
But maybe still get second thought from Clement.

This revision is now accepted and ready to land.Aug 24 2020, 11:02 AM

This looks clearer than the previous implementation. I'm not sure about the performance of the EdgeSet but I can't think of a better way here.

But the EdgeSet is exactly what we are searching. Is there a way to steal or copy the entries from a EdgeSetIterator or just perform a copy of the used items?

This would remove the need to allocate a full buffer of all the edges like here:
ss->preview_vert_index_list = MEM_calloc_arrayN(mesh->totedge, 2 * sizeof(int), "preview lines");

However, we still need to alloc/realloc each time the active vert changes.

This could be better yes but this new implementation is fine for me.

source/blender/editors/sculpt_paint/sculpt.c
8338

Is this the original object? Is this always ensured to be up to date?