Fix stale instance divisors when buffer spans change

[toaster0123]

Problem

Consecutive instanced draws can use the previous buffer's instance span, so per-instance colors, transforms, or other attributes are read at the wrong rate.

A span of 1 advances to the next attribute value for every instance. A span of 2 shares each value between two instances. If two buffers use the same shader attribute location and their spans change from 1 to 2, the second draw currently keeps divisor 1. Changing from 2 to 1 keeps divisor 2. Changing the span on the same buffer also leaves the old divisor in place.

This happens because GLRenderer compares only whether the buffers are instanced, and skips the update block entirely when the buffer object is unchanged.

Fix

Cache the numeric divisor for each attribute slot, independently of the buffer reference, and update it whenever the requested span changes. Use that cache when clearing unused slots too, so a mutated buffer or cleared weak reference cannot leave a stale divisor behind. Unchanged divisors still avoid redundant GL calls.

Validation

• Added 25 focused cases for different and reused buffers, positive and zero spans, single attributes and four-slot matrices, clearing/reusing slots, unchanged spans, and rendering without instancing support. The unchanged upstream baseline e8cf975be583a668d4121cdce334bfb0c5e75af0 fails 15 cases; the fix passes all 25.
• Native Mesa llvmpipe OpenGL ES 3.2 tests reproduce the visible error: after 1→2, baseline still draws red/green/blue/yellow instead of red/red/green/green. Both directions work after the fix, including mutations of the same buffer. Zero-span controls also pass. Each version has 12 real instanced draws, 48 RGBA pixel readbacks, and zero GL errors.
• Clean core build plus core/desktop/effects/plugins builds pass, including 582 tests with zero failures and two pre-existing skips, Javadocs, packaging, and metadata checks. Core Checkstyle passes; the new test has no warnings. Existing renderer warnings remain.
• Whole-repository clean build was attempted but is blocked by the missing Android SDK for the Android screenshot project.

The native checks use a test-only JNA bridge and software GLES. They do not cover every GPU, production backend, or scene. General recovery from arbitrary external GL state is outside this change; the context-reset test covers reapplying a positive divisor.

Supplemental native validation: standalone runner, pinned source hashes and sanitized native logs. This is separate from the Gradle tests in the PR.

[jaime-jmebot]

Nice: keying the reset off the cached divisor instead of attribs[slot].get().isInstanced() is strictly more robust, since a mutated buffer or a cleared weak reference can no longer hide a non-zero divisor.

The one invariant worth spelling out here: this cache is now a shadow of GL state, and setVertexAttrib skips the glVertexAttribDivisorARB call when the cached value is unchanged. So every path that frees a slot (or resets GL state) has to zero the cache in the same place, otherwise a missed clear turns into a silently wrong divisor rather than an obvious GL error. Would be nice to have a short comment saying that at the reset site.

[jaime-jmebot]

Clearing the divisor shadow here keeps it in lock-step with boundAttribs on context reset, which is exactly what makes the "unchanged divisor, no GL call" shortcut safe after a context re-create. 👍

Small doc nit: since GLRenderer now writes both arrays, it'd help to note on the field declaration that boundAttribDivisors[slot] must always be kept in sync with boundAttribs[slot] (0 = not instanced), so future edits don't break the pairing.

[jaime-jmebot]

Small suggestion for the new test: this blanket default (int → 0, boolean → false) is convenient glue, but it means any int-returning call on the fake silently becomes 0. Since the whole point of the test is the divisor value sent to GL, it would strengthen the test if the stub recorded the arguments passed to glVertexAttribDivisorARB and asserted on that recorded sequence rather than on internal state — that way a regression shows up as the wrong value reaching GL.

Not blocking: the 25 cases plus the baseline failing 15 already look solid coverage.

[toaster0123]

The fake already records the actual GL arguments: its glVertexAttribDivisorARB case takes the slot from arguments[0] and divisor from arguments[1], then each draw snapshots that driver-side array. The assertions inspect those draw-time snapshots, not RenderContext's cache. See the recorder at aa8d5a6. The int/boolean fallback does not supply the divisor value; the divisor setter is a void call handled before that fallback. Wrong or omitted divisor calls are what cause the 15 baseline failures.

[jaime-jmebot]

Thanks for the correction — I misread the recorder. Reading invoke again, glVertexAttribDivisorARB writes the passed divisor into the fake's own divisors[slot] array, and each glDrawArrays/glDrawArraysInstancedARB snapshots that array into drawDivisors. The assertions read those draw-time snapshots, so the value reaching GL is what gets checked, and the int/boolean fallback never supplies a divisor (the setter is a void call). My nit is withdrawn — no changes needed.

The remaining doc suggestion from my earlier pass (noting on the boundAttribDivisors declaration that it must stay in sync with boundAttribs) still stands as a nice-to-have, since the whole correctness argument rests on that pairing.

[jaime-jmebot]

Good fix — caching the numeric divisor per slot and driving both the set and clear paths off that cache closes the stale-divisor hole cleanly, and clearing it in RenderContext.reset() keeps the cache a faithful mirror of GL state.

• The 25-case test matrix (different/reused buffers, 1↔2 and zero spans, matrix slots, cleared slots) is solid, and confirming the baseline fails 15 of them is exactly the right way to prove the tests bite.
• Two non-blocking nits left inline: a comment documenting that the divisor cache and boundAttribs must stay in sync, and a suggestion that the fake GL stub record the actual glVertexAttribDivisorARB arguments rather than only returning defaults.

Happy to approve once those are considered.

[toaster0123]

On the cache invariant: zero means per-vertex fetch, and every renderer-issued divisor change must update the numeric cache. The clear path resets the GL divisor and cached number together before releasing the slot.

The numeric cache is deliberately independent of the weak buffer reference: that reference can be cleared, or the buffer can be mutated, while the native divisor remains nonzero. The field's existing "cached separately" comment is intended to preserve that distinction. The mutated/collected-buffer cleanup and slot-reuse tests cover it.

A separate distinction matters after external GL changes: resetting the Java cache to zero does not prove the existing driver state is zero. #2989 keeps those slots explicitly unknown until used. Its composition with this head passes all 47 divisor tests and native state/pixel checks; adding #2987 passes all 59 regression tests.

[jaime-jmebot]

I verified the fake in VertexAttribDivisorTest.java: invoke writes the divisor argument passed to glVertexAttribDivisorARB into the fake's own divisors array, and each draw snapshots that array into drawDivisors, which the assertions inspect. The author was correct — the test does assert on the value that reached GL, not on RenderContext's cache, and the int/boolean fallback never supplies a divisor.

I withdrew that nit in the existing thread and noted that my only remaining suggestion (a doc comment on boundAttribDivisors about staying in sync with boundAttribs) is still optional. The PR was already approved, so no new review event was needed.