OmarShehata / webgl-outlines

Implementation of a post process outline shader in ThreeJS & PlayCanvas.
MIT License
376 stars 41 forks source link

Broken surface ID buffer on GTX 1070 ? #9

Closed OmarShehata closed 1 year ago

OmarShehata commented 1 year ago

Reported on Twitter: https://twitter.com/tamat/status/1580872701035495425

image

I haven't been able to reproduce. My guess is the surfaceID buffer has invalid data drawn to it. That buffer is of type HalfFloat. I picked that because I thought that'd have more support than Float buffers, but it may be the other way around?

This buffer type is defined here:

https://github.com/OmarShehata/webgl-outlines/blob/b591487f9ae9c1fe450580be1f3b02cf58dd19d1/threejs-outlines-minimal/src/CustomOutlinePass.js#L28

zalo commented 1 year ago

I'm seeing this too on a 3080 in both Chrome and Firefox. In the console, I'm seeing this shader error:

index.cedc7d4f.js:3113 THREE.WebGLProgram: Program Info Log: C:\fakepath(53,12-96): warning X4713: Sample Bias value is limited to the range [-16.00, 15.99], using -16.000000 instead of -100.000000
C:\fakepath(53,12-96): warning X4713: Sample Bias value is limited to the range [-16.00, 15.99], using -16.000000 instead of -100.000000
C:\fakepath(53,12-96): warning X4713: Sample Bias value is limited to the range [-16.00, 15.99], using -16.000000 instead of -100.000000
C:\fakepath(53,12-96): warning X4713: Sample Bias value is limited to the range [-16.00, 15.99], using -16.000000 instead of -100.000000
C:\fakepath(53,12-96): warning X4713: Sample Bias value is limited to the range [-16.00, 15.99], using -16.000000 instead of -100.000000
C:\fakepath(53,12-96): warning X4713: Sample Bias value is limited to the range [-16.00, 15.99], using -16.000000 instead of -100.000000
C:\fakepath(53,12-96): warning X4713: Sample Bias value is limited to the range [-16.00, 15.99], using -16.000000 instead of -100.000000

On iOS Safari, it appears to work fine.

OmarShehata commented 1 year ago

Thanks @zalo ! Could I see what the "surfaceID debug buffer" looks like for you? And just to rule it out, the normal buffer and depth buffer modes as well?

zalo commented 1 year ago

image

Except for this buffer and the OutlinesV2 buffers, all the other buffer modes (including Outlines V1) look perfectly sane and correct.

None of the sliders have any effect on the corruption.

OmarShehata commented 1 year ago

Interesting, yeah so the problem is definitely in how the surfaceIDs are drawn, but I'm not sure why. This is the shader that draws the surfaceID's:

https://github.com/OmarShehata/webgl-outlines/blob/main/threejs/src/FindSurfaces.js#L164

I normalize it to 0-1 before I draw in the red channel. The "debug draw" version where it assigns a random color based on the ID shader is here:

https://github.com/OmarShehata/webgl-outlines/blob/main/threejs/src/FindSurfaces.js#L185

dtrebilco commented 1 year ago

I also get this on Brave browser on a 3070

OmarShehata commented 1 year ago

For people who can reproduce, could I get a screenshot of your WebGL report (https://webglreport.com/) ?

dtrebilco commented 1 year ago

Screenshot 2022-10-18 223141 Screenshot 2022-10-18 223209

dtrebilco commented 1 year ago

I think it might be caused by the GL_DITHER option that is enabled by default - this would mess up the index values being written to the color buffer and show edges everywhere. https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/enable

If I can get your example running locally I'll try and add and test.

dtrebilco commented 1 year ago

Nope, not the dither state (although I think it probably should be turned off anyway)

dtrebilco commented 1 year ago

OK I think I know what is causing this:

1) The object ID is read by the vertex shader (verified correct) and passed as a basic interpolator to the fragment shader. 2) The fragment shader basically just writes this object ID out to the Float16 buffer 3) Post process is applied on these id buffer values where if surfaceValueDiff != 0

As these id values are floating point, any slight variation will produce a outline. (in general, comparing floating point values for exact values causes problems)

There are 3 ways to fix it: Solution 1) Don't compare against exact values - change to surfaceValueDiff > 0.01 - that seems to work

Solution 2) Make sure no perspective correct interpolation is happening on the varying that will not need it - this ensures you get the exact value in the fragment shader - just put a "flat varying" in front of the shader interpolator when writing object ids. However, this needs WebGL2. FYI: Here is a good reference on perspective correct interpolation if you did not know about it: https://stackoverflow.com/questions/24441631/how-exactly-does-opengl-do-perspectively-correct-linear-interpolation

Solution 3) Another way to prevent perspective correct interpolation is to make sure position "w" value is 1. Simply do a gl_Position.xyz /= gl_Position.w; gl_Position.w = 1.0; in the vertex shader writing vertex ids.

Side note: I think you need to clamp the value "outline" when doing

gl_FragColor = vec4(mix(sceneColor, outlineColor, outline));

as outline could be > 1.0

Just adding a note here how I debugged this in case I ever need to revist: Using this video as a starting point https://www.youtube.com/watch?v=T7MPxvX5alA Using RenderDoc on an old version of ChromePortable ChromiumPortable.exe --no-sandbox --disable-gpu-watchdog --gpu-startup-dialog --disable-direct-composition Attached RenderDoc by injecting into process (this option is enabled via a toggle in settings)

Also tried to use Chrome canvas debugger, but it is removed from modern chrome. Some initial testing with https://spector.babylonjs.com/ plugin, but this does not support shader editing.

OmarShehata commented 1 year ago

Awesome, thank you so much @dtrebilco for digging into this and for the detailed solutions!! I should have guessed it was the interpolation, because this comes up in the original tweet thread where I first heard about this technique:

https://twitter.com/ianmaclarty/status/1499494985762480128

The solution suggested there is a simple round(surfaceId) which we should expect to work because we know the surfaceIds are always integers.

@dtrebilco could you give that a try and if that works you, would you like to open a PR to make this change? Clamping the outline value at the end also makes sense to me.

Otherwise solution 3 sounds good to me since it doesn't require WebGL 2 and doesn't require setting a threshold to compare against. We can add a comment explaining why that's necessary/linking to this discussion.

OmarShehata commented 1 year ago

This is now fixed in https://github.com/OmarShehata/webgl-outlines/pull/10

I've updated the demo with the latest code, so that should look correct for everyone now: https://threejs-outlines-postprocess.glitch.me/

Thanks again @dtrebilco and everyone for the help here!

dtrebilco commented 1 year ago

Verified live demo looks correct for me