spite / ShaderEditorExtension

Google Chrome DevTools extension to live edit WebGL GLSL shaders
MIT License
619 stars 58 forks source link

Shader editor affects rendering in San Angeles demo #8

Open kenrussell opened 9 years ago

kenrussell commented 9 years ago

Chrome 44.0.2359.0 (Official Build) canary (64-bit), OS X 10.9.5, Retina Display MacBook Pro

To reproduce:

  1. Open https://www.khronos.org/registry/webgl/sdk/demos/google/san-angeles/
  2. Open DevTools, open shader editor, reload
  3. Click "Program 1"

Expect the demo to continue rendering correctly.

Instead, the lighting is disabled and the objects render black. See attached screenshot.

screen shot 2015-04-08 at 6 14 27 pm

spite commented 9 years ago

This one I think it's happening because they're deleting the shader right after attaching it to the program, so my update routine tries to recycle it and fails miserably.

Is this a common pattern, doing away with a shader as soon as is attached? I guess it is, since technically most of the time you would never have a use for it anymore.

kenrussell commented 9 years ago

Nice work tracking this down.

I think this pattern is fairly common. At least, we've seen bugs in Chrome's own maintenance of shader and program objects when users immediately delete the shaders after attaching them to programs. Chrome itself tries to lazily delete objects that are attached to other ones (like renderbuffers, textures and shaders) to work around graphics driver bugs.

The best way I can think of to work around this is to defer deletion of shaders until the program(s) that reference them have all been deleted. I thought about creating a temporary shader and trying to "restore" the program to its earlier state later by re-attaching the user's previous shader. The problem is, once deleteShader is called, you can get into a situation where the last thing keeping the shader alive is the reference from the program object, so changing the program's shaders around can cause the shader to be unusable later.

I think this solution will require adding "private" attributes to WebGLProgram objects referencing the shaders that are attached, and possibly a reference count to WebGLShader objects indicating how many programs are referring to it.

(Thinking about this more, there's probably another problem if the same shader is used by more than one program object.)

spite commented 9 years ago

Actually, I've seen that the shader -as long as there's a reference- is not deleted after .deleteShader. So as long as I have a reference, it's still there. But the issue with this demo, and with many others, is something that I don't have enough knowledge to solve. I'll try to be as precise as possible.

The reason this demo goes black when re-compiling the program is because the uniforms don't have values anymore. Adding color = vec4(1.,0.,1.,1.); at the end of the vertex shader shows that the program is still running, but the light calculations are off because the light terms are 0.

Interestingly, color = colorIn;does work, even though it's a uniform, but color = light_0_diffuse;doesn't.

So I'm keeping track of uniforms for each program, and reassigning them after the shaders are updated, and as far as i can see, there are no errors.

So the question would be: what is the reasonable sequence to follow once the shader is updated, regarding uniformLocations, attribLocations, etc.? I've been trying to find some detailed explanation, but failed to find something that explains this issue.

brendankenny commented 9 years ago

@spite can you explain exactly what issue you're referring to in the last line? You seem exactly on the right track."glUniform* operates on the program object that was made part of current state by calling glUseProgram." When you change the program object, you have to set the values again.

kenrussell commented 9 years ago

In order to completely hide the fact that the program was re-linked behind the scenes, a few things are needed:

  1. All vertex attribute locations have to be the same in the re-linked program. In order to guarantee this, it's necessary to call getActiveAttrib on the original program from 0..getProgramParameter(program, ACTIVE_ATTRIBUTES), record the locations of those attributes, and then call bindAttribLocation on the program object for each of them, to re-assign them before re-linking. Otherwise you're leaving it to chance that the OpenGL implementation will assign the vertex attributes to the same locations.
  2. Re-set the uniforms' values as @brendankenny points out above. Note that in the San Angeles demo's shaders, colorIn is a vertex attribute, not a uniform, and its state isn't associated with the program object, but the overall OpenGL context. That's probably why "color = colorIn" works and "color = light_0_diffuse" doesn't. There may be a bug in the tracking of uniforms' locations or values.
  3. Have some way to map the obsolete WebGLUniformLocation objects to the new ones for the re-linked program. You already have some mechanism for this, but there might be a bug. The first idea that came to my mind was for your extension to return its own objects for WebGLRenderingContext.getUniformLocation, and change which real WebGLUniformLocation each refers to after re-linking the program. That might reduce the chance that there are errors in the bookkeeping.
brendankenny commented 9 years ago

(3) is a consequence of uniform locations being objects I hadn't thought about before. It would be handy to have CHROMIUM_bind_uniform_location here :)