Take the desktop picker id through a flat varying, not gl_PrimitiveID - #6773
Closed
Fedr wants to merge 1 commit into
Closed
Take the desktop picker id through a flat varying, not gl_PrimitiveID#6773Fedr wants to merge 1 commit into
Fedr wants to merge 1 commit into
Conversation
The Ubuntu 26.04 UI test picked face -1090332440 out of a 512-face mesh. On desktop RenderMeshObject::renderPicker uses MeshDesktopPicker, whose fragment shader took the id from gl_PrimitiveID, and that is the one channel of the pick pixel that came back wrong - uniGeomId, a uniform in the neighbouring channel, was right. llvmpipe 21.1.8 / Mesa 26.0.8 is the only stack where it happens; 22.04 and 24.04 pass. The vertex shader already computes the id as uint(gl_VertexID) / primBucketSize for the corner-mode path, so pass that same value down a flat integer varying and read it instead. Emscripten keeps gl_PrimitiveID untouched behind #ifndef, and the varying is declared only in the mode that reads it so the lines pickers keep linking.
Contributor
Author
|
Superseded by #6774, which carries the same commit rebased onto master so it no longer depends on #6772. Force-push is blocked on this repo's branches, so the rebase had to go to a new branch; |
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
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.
Stacked on #6772 (the picked-point guard), so this PR's diff is the shader change alone. GitHub will retarget it to master once #6772 merges.
What was wrong
The Ubuntu 26.04 UI test picked face -1090332440 out of a 512-face mesh, which is what #6772's new guard reports:
On desktop,
RenderMeshObject::renderPickerusesMeshDesktopPicker(MRRenderMeshObject.cpp:157), whose fragment shader took the primitive id fromuint(gl_PrimitiveID). That is the channel that came back wrong. The decisive detail is what came back right:uniGeomId, a plain uniform written into the neighbouring channel of the very same pick pixel, resolved to a real mesh object — otherwise the guard would never have reached the mesh branch. One channel of one fragment correct, the other garbage, points atgl_PrimitiveIDitself rather than at the FBO, the read-back or the decode.The read-back is not at fault:
GL_RGBA32UItexture,glClearBufferuivto0xffffffff,glReadPixels(..., GL_RGBA_INTEGER, GL_UNSIGNED_INT, ...)— internally consistent. Nor is the id arithmetic: every renderer that draws for picking setsprimBucketSize(mesh 3 and 1, points 1, label 1; features delegate throughRenderObjectCombinator), so there is no division by zero.What this does
The picker vertex shader already computes
uint primId = uint(gl_VertexID) / primBucketSizefor the corner-mode path. This passes that same value down a flat integer varying and reads it in place ofgl_PrimitiveID:flat out uint primitiveIdFlat;/primitiveIdFlat = primId;in the picker vertex shader, desktop only.primitiveIdFlat.Emscripten keeps
gl_PrimitiveIDuntouched behind#ifndef __EMSCRIPTEN__, and the varying is declared only in the mode that reads it, soLinesPicker/LinesJointPickerkeep linking against vertex shaders that do not provide it.There is precedent for the platform split a few files over:
getMeshFragmentShaderColoringBlock()already picksgl_PrimitiveIDon desktop and the interpolated float pair on Emscripten.Evidence it works
Same MeshInspector UI suite, same ubuntu26 image, with this commit in the MeshLib gitlink:
face=-1090332440toTriPoint, then a second infindVisibleFacescreate_feature_sphereThe remaining failure is unrelated: the
textscenario misses its mesh-similarity threshold by 0.0009 (0.9941 vs 0.995), which looks like a freetype/font difference on Resolute and is a baseline question, not a pick one.MRViewer.vcxprojbuilds clean locally andMeshViewer -hidden -noEventLoop -unloadPluginsAtEndexits 0 on Windows — though that only exercises the C++, since the picker shader is compiled lazily on the first pick.Note on scope
No
disable-build-*labels: this changes shader generation shared by every desktop GL platform, so Windows, macOS and the vcpkg legs all want to run, and the Emscripten leg is what proves the untouched ES branch still compiles.