Fix shader problem that broke 2-D vertex data - #715
Open
alexhuth wants to merge 2 commits into
Open
Conversation
The surface shaders sit right up against the 16 vertex attribute slots WebGL guarantees, and #679 added two more of them (flatheight and flatBumpNorms) to the vertex data shader so that bumpy_flatmap would work for Vertex dataviews. 2D vertex data needs four data attributes rather than two, which pushed surface_vertex to 17: it still compiles, but it fails to link with "Too many attributes", and three.js only reports that on the browser console, so the viewer just draws a black screen. The same overflow hits plain Vertex data as soon as equivolume sampling is on. Rather than giving these values slots of their own, pack them into the spare components of attributes that are already bound: - the white matter and pial vertex areas, which only the equivolume depth sampling reads, move into auxdat.zw (auxdat.x is the medial wall mask and .y the curvature; z and w were left at zero by brainctm) - the flatmap bump height moves into the fourth component of the bumped flatmap normals, so the two become a single vec4 flatbump attribute Every shader variant now links: the worst case, 2D vertex data on a subject with a flatmap, goes from 19 attributes to 16. Renders of the paths that did work before -- volume data with equivolume sampling, and the bumpy flatmap -- are pixel-identical to what they were. Fixes gh-714. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_013wiUP7TLgDqSfnv4n9ySZj
Nothing caught gh-714: the shader that failed to link compiled fine, the page raised no javascript error, and the screenshot was written -- it was just empty. Two checks that would have caught it: test_webgl_shaders links every combination of options the viewer builds surface, pick and depth shaders with, in a real (swiftshader) GL context, and reports how many of the available vertex attributes the variant uses when linking fails. It needs Chromium but no subject database, so it runs in a couple of seconds. test_datatype_renders additionally asserts the render is not a single flat color, which is what a viewer that failed to draw anything produces. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_013wiUP7TLgDqSfnv4n9ySZj
Contributor
|
This fixes Vertex2D in Chromium. I was hoping it would also fix it in Firefox (#375), since the root cause was the same from my investigation, but apparently not. |
Contributor
|
I also don't like that this test duplicates a chunk of the shader code (mainly definitions). I think we should test for general OpenGL errors in one of the basic rendering tests, or even |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pr fixes the recently introduced shader bug that broke 2-D vertex data displays (see issue #714). The core issue is that we are hitting the upper limit on the number of vertex attributes that we can pass to the shaders, and this causes threejs to silently choke. It also adds some tests that look for blank rendered images. This is not a real visual regression test — that would supersede this test.