Page MenuHome

Wide lines + line stipple deprecated API replacement
ClosedPublic

Authored by Alexander Romanov (a.romanov) on Mar 28 2016, 2:55 PM.

Details

Summary

The patch contains an implementation of the wide lines and the line stipple that is necessary for OpenGL upgrade.

For the implementation I have chosen the geometry shader because it required minimum changes for the wrapper calls and such implementation is the best for the "basic shader" architecture.

There are few shortcomings that can be corrected in future. They all are related to the fact that the lines in one strip are not connected with each other. So the stipple pattern is not continuous on the common vertex of two lines. There is also no continuity of form (each line is an independent rectangular).
But the advantage is that even outdated glBegin, glVertex work! Though with the above restrictions.
Continuity of form and stipple can be implemented with additional attributes, and it will require more changes in calls.

At the moment, the patch replaces calls for some "gestures". It works satisfactorily for "cross" or "rectangular" and not so good for "lasso" and "circle" due to the above-mentioned shortcomings.

Don't forget to set USE_GLSL to true for testing.

Alexander Romanov (Blend4Web Team)

Diff Detail

Event Timeline

Alexander Romanov (a.romanov) retitled this revision from to Wide lines + line stipple deprecated API replacement.Mar 28 2016, 2:55 PM
Alexander Romanov (a.romanov) updated this object.
Alexander Romanov (a.romanov) updated this revision to Diff 6327.

Going forward, code like this should target the Blender 2.8 release. That's when we'll have GLSL 1.5 and guaranteed geometry shader support. Blender 2.7x will stay at OpenGL 2.1 + extensions.

Looks good at first glance!

Thanks! I'll review this in detail later, might not have time until this weekend.

Brecht Van Lommel (brecht) requested changes to this revision.

Overall approach looks good to me.

source/blender/gpu/intern/gpu_basic_shader.c
387

Style nitpick: move brace to the next line.

This should also be a static function since it's not exposed in the header file?

628–631

I'd replace this by BLI_assert((GPU_MATERIAL_STATE.bound_options & GPU_SHADER_LINE) == 0);, we shouldn't be changing window size while the shader is bound.

source/blender/gpu/shaders/gpu_shader_basic_geom.glsl
3

Do you have a link to a website? Probably we shouldn't be including people's email addresses in our code.

6

What is this passthrough?

source/blender/gpu/shaders/gpu_shader_basic_vert.glsl
14

Maybe rename to varying_vertex_color_line?

source/blender/windowmanager/intern/wm_subwindow.c
308

There's other places that call glViewport, for example in the game engine or when offscreen rendering to a texture. So maybe we need a more general solution, like glGetIntegerv(GL_VIEWPORT, ..) when binding the shader?

That may not be great for performance, I'm not sure how fast that call is generally, I'm not sure.

This revision now requires changes to proceed.Apr 3 2016, 1:31 AM
source/blender/windowmanager/intern/wm_subwindow.c
308

When we have a custom matrix stack then we can make this efficient. So the potentially inefficient glGetIntegerv per shader bind is fine for now really, as long as there is a comment that we should optimize this.

Alexander Romanov (a.romanov) marked 5 inline comments as done.
Alexander Romanov (a.romanov) updated this revision to Diff 6354.

Fixed drawbacks

Alexander Romanov (a.romanov) marked 2 inline comments as done.Apr 4 2016, 1:48 PM
Alexander Romanov (a.romanov) added inline comments.
source/blender/gpu/shaders/gpu_shader_basic_geom.glsl
6

It's for debugging. Just show original lines.

Brecht Van Lommel (brecht) accepted this revision.

Looks good to me!

This revision is now accepted and ready to land.Apr 5 2016, 1:06 AM

Ok, now I'll wait for start of Blender 2.8 development process.

I think this patch can be committed now? As long as USE_GLSL is off everything should keep working unchanged.

I think this patch can be committed now? As long as USE_GLSL is off everything should keep working unchanged.

@Mike Erwin (merwin), @Brecht Van Lommel (brecht), what policy do we have? Are there plans to enable USE_GLSL for Blender 2.7* ?

I think we'll postpone actually enabling USE_GLSL to 2.8, but we can continue doing refactoring in master and commit this now I think.

Mike Erwin (merwin) accepted this revision.

Looks good! I wanted to test it on Mac before giving the OK but have been busy. Go ahead and commit then we'll clean up any rough edges later.

I typically test on GL 2.1 (Mac), GL 3.2 (Radeon HD 2400 on Windows) and GL 4+ (also on Windows). Just to make sure code doesn't break under different supported GL versions.

Since we don't have a better place to put this, master is fine.

Alexander Romanov (a.romanov) updated this revision to Diff 6374.

Sorry, I've lost the geometry shader file in the previous update.

This revision was automatically updated to reflect the committed changes.