Page MenuHome

GPENCIL: Fix filling error for concave shapes
ClosedPublic

Authored by Antonio Vazquez (antoniov) on Jan 1 2016, 5:32 PM.
Tags
None
Tokens
"Love" token, awarded by razvanc87."Love" token, awarded by kakachiex2."Love" token, awarded by plasmasolutions."Love" token, awarded by bliblubli.

Details

Summary

Improve filling for concave shapes using a triangulation of the stroke. THe triangulation information is saved in an internal cache and only is recalculated if the stroke change. The triangulation is not saved in .blend file.

Diff Detail

Repository
rB Blender

Event Timeline

Antonio Vazquez (antoniov) retitled this revision from to Fix grease pencil fill.
Antonio Vazquez (antoniov) updated this object.

I'm not sure stencil buffers are the best solution here. One downside is that it increases graphics card memory usage when running Blender always, regardless if grease pencil is used or not. But mainly I worry that clearing the entire stencil buffer and drawing a quad over the entire viewport for every stroke that is drawn will give very bad performance.

Why did you use this method rather than tessellating the polygon on the CPU using BLI_polyfill_calc()? I imagine the performance of that is also not ideal, though perhaps with caching, multithreading or faster algorithms it might be ok.

Perhaps there is a quick way to check if the shape is convex and only do more expensive handling of concave polygons when it's necessary. There shouldn't have to be an option to enable correct drawing of concave polygons, the other draw method isn't really low quality, it's just wrong.

A couple of notes:

  • You don't need to draw a full screen quad, redrawing the same polygon should only draw it in affected areas with the catch that any final draw should reset the stencil
  • If algorithm is done right, ie resets stencil as above, stencil need only be cleared once, at blender startup time.
  • Most 24bit depth buffers are actually aligned at 32 bit boundaries as far as I know, so memory should not be a concern.
  • I don't see why we need an option for this kind of thing. Do it always, unless of course there is a measurable performance hit.

We can always test how performance works in practice but I don't think presense of stencil should be a memory concern. The concern will be bookkeeping and avoiding stepping on each other's toes, if other effects want to use stencil. For that reason I would also rather not use stencil for polygon tesselation, but it's probably the fastest solution - also the least memory hungry. Polyfill needs to store the buffer on CPU.

Also, for better performance you can use two sided stencil, it should reduce the algorithm to two passes only.

Check:

https://www.opengl.org/sdk/docs/man2/

glStencilFuncSeparate
glStencilOpSeparate

Ah, I see this is done with inversion...forget my last comment then.

Regarding the concerns about the option:
At least in an earlier version of this patch, the new drawing method ended up breaking some old files in various ways. I'd have to test whether this still is a problem with this latest patch, but I suspect that this, rather than performance reasons is why we may want to have this.

Very needed patch, but I would also leave it as an option as many old files did use this bug for artistic purposes. Maybe we can have this new fix be the default, but activate an inverted option "compatibility fill" for files created before 2.77. The new behavior is what most people will await.

If artists find the old behaviour still useful then I guess it's fine to keep. Added some inline comments.

source/blender/editors/gpencil/drawgpencil.c
399

Don't clear here, this should only be done at startup (at GHOST level maybe)

424

Not needed, just redraw the polygon with glStencilOp(GL_KEEP, GL_KEEP, GL_ZERO);

source/blender/gpu/intern/gpu_framebuffer.c
169 ↗(On Diff #5714)

Check for GLEW_EXT_packed_depth_stencil

source/blender/gpu/intern/gpu_texture.c
143 ↗(On Diff #5714)

only if GLEW_EXT_packed_depth_stencil is supported, else you need to use regular depth format

About the option to disable/enable is for several reasons:

a) Old files can be wrong.
b) The artis maybe prefer the old method.
c) Performance options.

The user Daniel M. Lara (@_pepeland_) has done several speed test:

2736x1824 (surface pro 4):
-2 layers (old method): 93 fps

  • 2 layers (HQ): 75-80 fps

1920x1080:

  • 2 layers(old method): 112 fps
  • 2 layers (HQ): 100 fps

One important point is when you use the HQ, you don't need to use as many points, so the size of the stroke array is smaller.

Actually, I agree with @Brecht Van Lommel (brecht), using BLI_polyfill_calc might be the way to go here.
It just needs to be called after every edit. Maybe we should test how well that works first?

Antonio Vazquez (antoniov) edited edge metadata.

Update diff file after first review comments

Antonio Vazquez (antoniov) marked 4 inline comments as done.Jan 4 2016, 11:21 AM

I'm going to create a branch in my PC and try the BLI_polyfill_calc option.

source/blender/editors/gpencil/drawgpencil.c
399

Replaced the actual glClear(GL_DEPTH_BUFFER_BIT) in view_3d_draw to glClear(GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);

Antonio Vazquez (antoniov) marked 2 inline comments as done.EditedJan 4 2016, 3:58 PM

I have done some test using BLI_polyfill_calc, but I get some glitches in the filling.

Attached the code as proof of concept (this is a new branch with only this change, no STENCIL).

am I missing something or doing something wrong?

Updated: I have seen the problem is in the conversion between 3d and 2d. THe conversion is not right.

I have developed a completely different solution using polyfill calc and an internal cache. The result is very good and gets high FPS rate without using GPU memory..

I'm not sure how use this page...do I upload the new diff in this task or is it better open a new one?

Antonio Vazquez (antoniov) retitled this revision from Fix grease pencil fill to GPENCIL: Fix filling error for concave shapes.Apr 6 2016, 1:06 PM

Hi @Antonio Vazquez (antoniov), just upload it here - it's completely valid to develop it further here...

Totally new stroke filling method. This method uses polyfill_calc and a cache to keep the data and get very high FPS rate. No additional GPU memory used.

Joshua Leung (aligorith) requested changes to this revision.Apr 8 2016, 2:36 PM
Joshua Leung (aligorith) edited edge metadata.

@Antonio Vazquez (antoniov): Nice work! Looking forward to giving this a good round of testing later :)

In the meantime, here are a few things I spotted while looking over the code.

source/blender/editors/gpencil/drawgpencil.c
335

Just use "float"

339

sub_v3_v3v3

345

Use sub_v3_v3v3() instead

366

sub_v3_v3v3()

387

"stri" may be a less of a mouthful :)

Picky nitpick - I generally try to have all the struct-types before the primitive data types (int, float, etc.). It's not really a formal style requirement, though it's something I try to do in all of my code.

402

It took me a moment to realise that this was checking whether the three indices are within bounds.

Two ways you could make this clearer are:

  1. Put the range checks for each vert on the same line:
if ((tmp_triangles[i][0] >= 0) && (tmp_triangles[i][0] < gps->totpoints) &&
    (tmp_triangles[i][1] >= 0) && (tmp_triangles[i][1] < gps->totpoints) &&
    ...)
{

OR

  1. Use the "IN_RANGE_INCL" macro:
const int max_index = gps->totpoints - 1;
if (IN_RANGE_INCL(tmp_triangles[i][0], 0, max_index) &&
    IN_RANGE_INCL(tmp_triangles[i][1], 0, max_index) && 
    ...)
{
458

Just use glVertex3fv after getting address of the "x" val for the point?

source/blender/editors/gpencil/gpencil_edit.c
869

In this case, it's better to just set to null - there are no triangles to draw (tot_triangles = 0), so it doesn't make sense to allocate a single triangle which will then be freed when rebuilding the cache.

source/blender/makesdna/DNA_gpencil_types.h
65

No need for extra whitespace here

86

Just "pad" will do

181

Line up the equal sign with the preceeding lines (using spaces)

source/blender/makesrna/intern/rna_gpencil.c
495

See note below for line 594

594

Is there any benefit that exposing this has for scripters? Otherwise, I don't think this is really that useful (as internal "draw-time" data that gets rebuilt)

This revision now requires changes to proceed.Apr 8 2016, 2:36 PM
Antonio Vazquez (antoniov) edited edge metadata.

Updated code after review. I have applied all suggested changes.

As noted in my comments about the GP design doc, new layers should get this turned on by default. I don't think most users should need to worry about why they can't draw concave shapes and have to turn on this option first.

source/blender/editors/gpencil/drawgpencil.c
333

This can be "static void" - Doesn't look like it's used anywhere else outside this file.

Antonio Vazquez (antoniov) updated this object.
Antonio Vazquez (antoniov) edited edge metadata.

Changed method to static and new layers with HQ fill flag enabled by default.

I've tested the patch, and by and large it seems to work for most concave shapes in general.

However, after pushing it a bit, a managed to stumble across some cases where some glitches started showing up. Check each frame of the attached file for examples of things which worked vs things which didn't:

The shapes on the green layer are the filled strokes, and the red layer is where I've gone and traced over places where weird overdrawn triangles have appeared (some of which show up between verts which aren't even close to each other). The weirdest part though, is that undoing seems to result in slightly different triangulations (so some of the marked spots may not correspond to anything, but other new spots will have appeared).

I think this is normal thinking in how polyfillcalc works. If the end/start of the polygon is like an spiral, the function creates a triangle from first to last, and this can be weird, but if you think in a rope and you put in this shape, you will see this is the right way to do it. Anyway, the triangulation is using the standard triangulation of Blender.

There are two situations when the fill is weird (this does not mean wrong):

a) The start/end are inside of the stroke
b) the polygon line has a "knot".

Both situations are related to a stoke that is like a "folded rope". If we used a routine to fill the space (as any bitmap program) we could fill before getting the edges, but this is done using polyfill standard calculation and this works fine except in this situations.

Daniel has been doing a lot of test, he can give you a better feedback.

This a test done by Daniel M. Lara using a custom Blender version compiled with new triangulation HQ fill. He gets a maximum of 104 FPS and a sustained rate of 90 FPS.

You can see complete animation here: https://vimeo.com/164067136

Where can I find a Win 64 build I want to test it further

Update to fix the bug with extra triangles that produces glitches in some situations.