Skip to content

Take the desktop picker id through a flat varying, not gl_PrimitiveID - #6774

Closed
Fedr wants to merge 2 commits into
masterfrom
fedr/flat-picker-id
Closed

Take the desktop picker id through a flat varying, not gl_PrimitiveID#6774
Fedr wants to merge 2 commits into
masterfrom
fedr/flat-picker-id

Conversation

@Fedr

@Fedr Fedr commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What was wrong

The Ubuntu 26.04 UI test picked face -1090332440 out of a 512-face mesh, which an instrumented run reports as:

[warning] pointOnObjectToPickedPoint: not a valid mesh pick: face=-1090332440, faceSize=512

On desktop, RenderMeshObject::renderPicker uses MeshDesktopPicker (MRRenderMeshObject.cpp:157), whose fragment shader took the primitive id from uint(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 at gl_PrimitiveID itself rather than at the FBO, the read-back or the decode.

The read-back is not at fault: GL_RGBA32UI texture, glClearBufferuiv to 0xffffffff, glReadPixels(..., GL_RGBA_INTEGER, GL_UNSIGNED_INT, ...) — internally consistent. Nor is the id arithmetic: every renderer that draws for picking sets primBucketSize (mesh 3 and 1, points 1, label 1; features delegate through RenderObjectCombinator), so there is no division by zero.

Upstream: this is a known llvmpipe regression

mesa#15660 (llvmpipe, regression, filed 2026-06-15, closed 2026-07-22), reported against the same renderer string we run on, llvmpipe (LLVM 21.1.8). Its fix, mesa!42967 "llvmpipe: emit FS input vertex attributes in driver location order", states the cause:

compute_vertex_info() emitted the vertex attributes by walking the FS input variables in declaration order, while the FS reads the input with driver location i from attrib i + 1 [...] the FS inputs PRIMITIVE_ID, VIEWPORT and LAYER are assigned driver locations after all other inputs while the variable list is sorted by location, so the FS read its inputs from the wrong attribs when gl_PrimitiveID was used together with user varyings without a geometry shader.

That is exactly this shader: world_pos, primitiveIdf0 and primitiveIdf1 are user varyings, gl_PrimitiveID is read alongside them, and MeshLib has no geometry shader anywhere. It also accounts for the shape of the garbage - -1090332440 is bit-identical to the float -0.5111, i.e. a user varying's value read out of the wrong attrib slot, most likely a world_pos component of the picked point.

The fix commit 77251a488e91 is on Mesa main only - no stable tag, no 26.0 branch, no backport label - and Ubuntu 26.04 ships Mesa 26.0.8, so this is not something waiting on a distro update.

Note the mesh draw shader reads gl_PrimitiveID the same way on desktop (MRMeshShader.cpp, for flat shading and the selected-faces bitset). Being a varying-layout mismatch rather than a broken system value, the bug depends on each program's input set, and that one evidently lays out compatibly - the select UI scenarios pass. Left alone deliberately: no reason to churn the rendering path that works.

What this does

The picker vertex shader already computes uint primId = uint(gl_VertexID) / primBucketSize for the corner-mode path. This passes that same value down a flat integer varying and reads it in place of gl_PrimitiveID:

  • flat out uint primitiveIdFlat; / primitiveIdFlat = primId; in the picker vertex shader, desktop only.
  • the non-corner-mode fragment path reads primitiveIdFlat.

Emscripten keeps gl_PrimitiveID untouched behind #ifndef __EMSCRIPTEN__, and the varying is declared only in the mode that reads it, so LinesPicker/LinesJointPicker keep linking against vertex shaders that do not provide it.

There is precedent for the platform split a few files over: getMeshFragmentShaderColoringBlock() already picks gl_PrimitiveID on 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:

before after
invalid pick reported face=-1090332440 none
crashes SIGSEGV in toTriPoint, then a second in findVisibleFaces 0

(The invalid-pick log line and the absence of the first crash come from #6772, which guards that conversion independently; this PR removes the cause rather than the symptom, and the two do not depend on each other.)
| scenarios | died on the first pick of create_feature_sphere | features and select suites run to completion, 5 of 6 suites pass |

The remaining failure is unrelated: the text scenario 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.vcxproj builds clean locally and MeshViewer -hidden -noEventLoop -unloadPluginsAtEnd exits 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.

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.
@Fedr

Fedr commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Closing: the flat id is only valid in corner mode, and the desktop picker is usually not in corner mode.

loadFaceIndicesBuffer_() fills the element buffer per face, but only the corner-mode branch writes {3f, 3f+1, 3f+2}; the other branch writes topology.getTriVerts( f, ... ), i.e. real shared vertex ids. And on desktop cornerMode_ is set to false whenever the mesh has no creases (MRRenderMeshObject.cpp, else if ( cornerMode_ && objMesh_->creases().none() )), which is the common case — that is the GPU-memory optimisation from #2862, the same PR that introduced MeshDesktopPicker and its gl_PrimitiveID read.

So in vertex mode uint(gl_VertexID) / primBucketSize is not the face id at all, and this PR would have returned wrong faces on every platform for any crease-free mesh. gl_PrimitiveID is precisely the value that is correct in both modes, which is why it was chosen.

That also weakens the evidence I posted above: on the ubuntu26 run the guard stopped warning and the crash disappeared, but a vertex-mode id lands in range — a valid face id, just the wrong face. Absence of an out-of-range value is not proof of a correct pick, and the feature scenarios compare final meshes with a tolerance, so a plausible-but-wrong surface point can pass them. I presented that as confirmation; it was not.

Where that leaves the llvmpipe bug (mesa#15660, fixed on mesa main only, in no release):

MeshInspectorCode#7749 tracks the user-visible crash and needs its "fixes in flight" section corrected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant