Draw UI image element quads as indexed triangles, not a triangle strip

[willeastcott]

Description

UI image elements built their quad as a non-indexed 4 vertex GL_TRIANGLE_STRIP. On Exynos 2500 / Xclipse (RDNA) devices in Chrome for Android, the interpolated uvs come out corrupted across exactly the strip's second triangle, so half of every UI image is smeared along the quad diagonal.

The vertex order is BR, TR, BL, TL, so the strip emits:

• triangle 0 = (BR, TR, BL) — the bottom-right half
• triangle 1 = (TR, BL, TL) — the top-left half, and the odd triangle, whose winding the driver has to flip to keep facing consistent

In the reporter's screenshots the bottom-right half of every affected sprite is correct and the top-left half is smeared along the BL→TR diagonal. That is triangle 1, every time.

The control case is in the same screenshots: all the text renders correctly. Text elements use the same shaders, the same atlas textures and the same materials — they differ only in that TextElement already emits indexed triangles. The strip's implicit winding flip for odd triangles is the one thing the two paths do not share.

Rather than detect the device, this drops the strip. It buys nothing here (both are a single draw of two triangles), it is a legacy topology that little else on the web still exercises, and the batcher already had to convert these meshes to indexed triangles anyway. The index order matches the batcher's, so batched and non-batched images rasterize identically.

Changes:

• image-element.js — 4 vertices plus a 6 index buffer, PRIMITIVE_TRIANGLES. The index buffer is shared by every image element via a DeviceCache, and ImageElement#destroy detaches it before the renderable tears the mesh down, since Mesh#destroy would otherwise destroy a buffer the remaining elements still use.
• immediate.js — same for the app.drawTexture quad, the only other content-path strip in the engine
• batch-manager.js — comment no longer claims the non-indexed fan / strip case is the UI path
• component.test.mjs — regression tests for the topology and for the shared buffer surviving a single element's destruction

Correctness

Built the engine before and after and diffed real GPU output on WebGL 2. For plain, stencil-masked and batched UI images, and for app.drawTexture, the rendered pixels are identical, while the draw call changes:

before: drawArrays(TRIANGLE_STRIP, 4)
after:  drawElements(TRIANGLES, 6)

Batching still merges 3 quads into a single drawElements(TRIANGLES, 18), and a stencil-masked child still clips to exactly 100x100 / 10000 px at the same pixel coordinates as before.

Test suite: 2044 passing, with the same 11 pre-existing failures as main (asset fetches that need network access).

Performance

Benchmarked because an indexed draw is not obviously free. 2000 UI image elements at 8x8px (small and non-overlapping, so this is draw bound rather than fill bound), 90 frames x 8 rounds with the variant order alternating each round, gl.finish() per frame. Intel Arc Pro 140T, ANGLE/D3D11:

variant	draw call	median ms/frame	vs strip
strip (before)	drawArrays(TRIANGLE_STRIP, 4)	3.40	+0.0%
indexed, index buffer per element	drawElements(TRIANGLES, 6)	4.20	+23.5%
indexed, one shared index buffer (this PR)	drawElements(TRIANGLES, 6)	3.40	+0.0%
6 non-indexed vertices	drawArrays(TRIANGLES, 6)	3.40	+0.0%

The cost is the per-element buffer, not the indexed draw — holding everything else constant and swapping 2000 distinct index buffers for one shared one recovers all of it. That matches the code: WebglGraphicsDevice#setBuffers rebinds ELEMENT_ARRAY_BUFFER on every draw regardless, since it cannot cache the state (the VAO captures the last bind). A distinct buffer per element turns each of those into a real rebind, on top of a driver side buffer object per element. With one shared buffer it is the same handle every time.

Worth noting the corollary: going indexed adds no extra WebGL call, because non-indexed draws already pay that bindBuffer(…, null).

6 non-indexed vertices measured at parity too, so it was not worth the larger diff — it would need _updateMesh rewritten to write duplicated corners, and batch-manager.js taught about non-indexed PRIMITIVE_TRIANGLES (today the non-indexed branch only handles TRIFAN/TRISTRIP, and anything else falls through leaving numIndices / indexData from the previous loop iteration — a latent bug that change would start hitting).

This is desktop ANGLE/D3D11, not a mobile tiler, so the ranking there may differ — though "one buffer object instead of N" should hold at least as well.

Note on the 3D case

I do not have an Exynos device, so this is not confirmed on hardware — but the artifact lines up with the strip's odd triangle too precisely for that to be coincidence.

The 3D table in the first screenshot of #9051 is a separate question: indexed triangle meshes do not go through the strip path, so this change will not affect them. The one route by which a 3D mesh would hit the same driver bug is a glTF with mode: 5 primitives, which gltf-accessor.js maps straight to PRIMITIVE_TRISTRIP. Worth asking the reporter to check meshInstance.mesh.primitive[0].type on the affected mesh — if it is 5, the same reasoning extends there; if it is 4, that artifact has a different cause.

Refs #9051

Checklist

• I have read the contributing guidelines
• My code follows the project's coding standards
• This PR focuses on a single change

[github-actions]

Build size report

This PR changes the size of the minified bundles.

Bundle	Minified	Gzip	Brotli
playcanvas.min.js	2358.0 KB (+0.2 KB, +0.01%)	605.3 KB (+0.1 KB, +0.01%)	470.1 KB (+0.3 KB, +0.06%)
playcanvas.min.mjs	2355.4 KB (+0.2 KB, +0.01%)	604.4 KB (+0.1 KB, +0.02%)	469.7 KB (−0.1 KB, −0.02%)

[mvaligursky]

Automated PR review by Codex (GPT-5).

Reviewed topology and winding correctness, WebGL/WebGPU behavior, batching consistency, shared index-buffer ownership across element destruction and device loss/restoration, immediate-mode rendering, API compatibility, and performance impact. No blocking issues found.

The indexed triangle order is consistent with the batcher, the shared DeviceCache allocation avoids per-element index buffers, and the explicit detach prevents Mesh destruction from invalidating other image elements. The focused ElementComponent suite passes (8/8), and all repository CI checks are green.

[Copilot]

Pull request overview

Replaces legacy triangle-strip quads with indexed triangles to avoid UI texture corruption on affected graphics drivers.

Changes:

• Adds a device-shared index buffer for image elements with safe destruction.
• Converts immediate-mode texture quads to indexed triangles.
• Adds topology and shared-buffer lifecycle regression tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/framework/components/element/image-element.js	Uses shared indexed-triangle geometry for UI images.
src/scene/immediate/immediate.js	Converts immediate quads to indexed triangles.
src/scene/batching/batch-manager.js	Updates the non-indexed topology comment.
test/framework/components/element/component.test.mjs	Tests topology and shared-buffer lifetime.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[willeastcott]

Noooooo! I meant to close this.

[mvaligursky]

Noooooo! I meant to close this.

Hah! Need a revert PR now :)