Did you receive the email thread titled "[Bf-viewport] OpenGL refactoring tasks"? From 11 Dec 2015. We discussed flat shading with the dFdx approach. I still think it's a bad approach but some parts of this patch are good improvements.

Benefits:

We don't have to specify normals for flat geometry, they are derived from positions.
Declarative choosing of shading model (smooth by default). This is much better than toggling modes with GL functions!

Drawbacks:

More complexity in the fragment shader! To compute the same value at each fragment.
glShadeModel(GL_FLAT) does not affect just normals; it makes all vertex attributes (except position) flat.

What happens when we want flat normals but interpolated UV & color attributes? Maybe the options should be SMOOTH, FLAT_NORMALS, FLAT_EVERYTHING.

In D1958#44738, @Mike Erwin (merwin) wrote:

Did you receive the email thread titled "[Bf-viewport] OpenGL refactoring tasks"? From 11 Dec 2015. We discussed flat shading with the dFdx approach. I still think it's a bad approach but some parts of this patch are good improvements.

Yeah, I am now re-reading.

What happens when we want flat normals but interpolated UV & color attributes? Maybe the options should be SMOOTH, FLAT_NORMALS, FLAT_EVERYTHING.

Well, thus I should rename USE_FLAT_MODEL to USE_FLAT_NORMALS. It seems to me that FLAT_EVERYTHING is useless. What is the use of this?

And what about the material shader in general. How do you see workflow of Viewport upgrade? Are we ready to remove Viewport and rewrite it from scratch? It may be better first to create an infrastructure in which at least partially have been solved tasks like Flat Model. It could be helpful for avoiding of bad designs.

I don't know if there is a drawback in practice. We are generally CPU bound, and doing this in the fragment shader might end up being faster in some cases due to simpler CPU side code, even if it's more work for the GPU? It also lets us use indexed buffers more easily, which has performance benefits too.

It's true that this does not cover glShadeModel entirely, however do we actually ever need non-interpolated vertex attributes? As far as I know we do not depend on that, the main thing is the normals?

For general GLSL materials I guess we will have to copy this code, it's not clear to me how those could be based on the basic shader. There might be a good way to code like this between GLSL shaders, but I don't really have ideas for that at the moment.

Regarding rewriting from scratch. I know this has been mentioned for 2.8, though personally I'm not particularly interested in doing that. It would be good to significantly clean up the 3D view drawing code at some point, perhaps rewriting a lot of it. But this work of removing deprecated OpenGL functions and improving the GPU module will be needed regardless.

source/blender/gpu/GPU_basic_shader.h
53–54	Maybe rename this to `GPU_SHADER_FLAT_NORMAL`.
source/blender/gpu/shaders/gpu_shader_basic_frag.glsl
31	I'm not sure what the meaning is of `ec`, could this be renamed to something without cryptic abbreviations?

This revision now requires changes to proceed.Apr 27 2016, 8:54 PM

@Brecht Van Lommel (brecht) wrote:

I don't know if there is a drawback in practice. We are generally CPU bound, and doing this in the fragment shader might end up being faster in some cases due to simpler CPU side code, even if it's more work for the GPU? It also lets us use indexed buffers more easily, which has performance benefits too.

For much drawing in 2.8 we'll gather all draw data into a batch (on CPU, only when model changes) then draw many times (GPU). Overall we will be less CPU limited. I think we should judge performance arguments based on that assumption.

Indexed drawing with positions (no normals) will benefit, good point!

I'm not violently opposed to the dFdx approach. It just seems like we're making the GPU work harder than it has to, to put the same image on screen. These little inefficiencies add up.

It's true that this does not cover glShadeModel entirely, however do we actually ever need non-interpolated vertex attributes? As far as I know we do not depend on that, the main thing is the normals?

Right, we are not restricted to mimic glShadeModel exactly. The main thing for this task is normals.
Non-interpolated & linear interpolated attributes are useful. They might or might not be needed in the simple shader interface. Solid color for example could use "flat". Probably should. They're definitely needed for certain effects in non-simple shaders though. I'm using linear attribs right now to implement nicer outlines -- see T48238.

For general GLSL materials I guess we will have to copy this code, it's not clear to me how those could be based on the basic shader. There might be a good way to code like this between GLSL shaders, but I don't really have ideas for that at the moment.

No need for simple shaders there, let's keep building a shader from the material node graph like today. Whether it's a Cycles preview or targeting a real-time game engine. Material creation should be very similar to the end user.

In D1958#44766, @Mike Erwin (merwin) wrote:

No need for simple shaders there, let's keep building a shader from the material node graph like today. Whether it's a Cycles preview or targeting a real-time game engine. Material creation should be very similar to the end user.

Well, as far as I understand, it is not assumed by design that the basic shader will cover material shader functionality, and it is not so useful for glShadeModel replacement. So this task primarily should be oriented to material shader. looking ahead, I want to ask, what drawbacks of design have you noticed in the material shader? Is there something to refactor?