Skip to content

Fix stale vertex divisors after external GL rendering - #2989

Open
toaster0123 wants to merge 2 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/vertex-divisor-invalidation-20261002
Open

toaster0123 wants to merge 2 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/vertex-divisor-invalidation-20261002

Conversation

@toaster0123

@toaster0123 toaster0123 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

After another library changes OpenGL state, jME can keep using an old instance divisor even when the next vertex buffer requests ordinary per-vertex data.

A concrete sequence is:

  1. External GL code sets an attribute's divisor to 2.
  2. The application calls renderer.invalidateState(), as the API documentation requires after external GL changes.
  3. jME draws with that attribute's instance span set to 0.

The native divisor remains 2, so the draw reads the wrong attribute values. This also happens for a slot jME has never used before.

invalidateState() clears the Java cache to its assumed defaults. It does not reset the native divisor, but the next zero-span buffer is treated as if zero were already active.

Fix

Mark every attribute slot as unknown during invalidation. Restore divisor 0 lazily when a non-instanced buffer next uses that slot, only on renderers with instancing support. Positive spans keep their existing reapply path. Fresh renderers retain their known zero defaults, and unchanged draws avoid repeated calls.

No new GL calls happen inside invalidateState(), preserving its use during context transitions.

Validation

  • Added 22 focused cases: baseline e8cf975be583a668d4121cdce334bfb0c5e75af0 fails 15 and passes 7 controls; the fix passes all 22. Tests cover external and renderer-established state, used/unused slots, same/different buffers, matrix attributes, repeated invalidation, fresh defaults, caching, and unavailable instancing.
  • Native Mesa llvmpipe OpenGL ES 3.2 reproduces three failing sequences. Baseline keeps divisor 2 and draws red/red/green/green; the fix restores 0 and draws red/red/red/red. Each version covers 6 cases, 10 real draws, 40 RGBA readbacks, and zero GL errors.
  • Based directly on master and verified together with Fix stale instance divisors when buffer spans change #2988 at aa8d5a6ace78d4872b7f8a06fe6f931b159b32f9: the patch applies cleanly, all 47 combined divisor tests pass, and combined native pixels are correct.
  • Clean core/desktop builds and LWJGL3/effects/plugins checks pass: 583 tests, zero failures, two pre-existing skips. Renderer Checkstyle passes with no warnings in the new test. Existing source warnings remain.
  • The whole-repository build remains blocked locally by the missing Android SDK for the Android screenshot project.

Native validation uses a test-only JNA bridge and a software driver. It does not cover every GPU, production backend, or complete scene. This change is limited to vertex attribute divisors after invalidation.

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

@jaime-jmebot jaime-jmebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and tight change — the lazy, per-slot restore is the right shape for this bug.

What I like:

  • Keeping invalidateState() free of GL calls is the correct constraint. It means the method stays safe to call around context loss/restore, and the recovery cost is only paid by slots that actually get used non-instanced afterwards.
  • Marking every slot as unknown (not just slots jME has already touched) is the part that fixes the harder case: a slot the app or another library used before jME ever bound a buffer to it. A per-used-slot flag would have left that bug open.
  • Good test spread: fresh-renderer defaults, the no-instancing-support path, and the "no repeated calls" caching check are exactly the three spots this kind of fix usually regresses.

Two small notes, mostly for the merge with #2988 rather than as blockers:

  • If #2988 lands a cached "currently applied divisor" per slot, those cached values must also be treated as dirty when invalidation marks a slot unknown — otherwise the positive-span path could think it already applied the divisor after an external change.
  • Adding a short comment on the new field (something like "true = native divisor for this slot is unknown") would save the next reader from having to reverse the meaning of false vs true.

No blocking issues from my side.

@riccardobl
riccardobl marked this pull request as ready for review October 2, 2026 13:06

Copy link
Copy Markdown
Contributor Author

Verified the interaction with #2988 at aa8d5a6ace78d4872b7f8a06fe6f931b159b32f9:

The new boolean means that the slot's native divisor is unknown after invalidation. Fresh renderer instances start with known GL zero defaults. The combined 47 divisor tests pass, including external positive changes, zero/positive requests, matrix slots, and repeated invalidation. Combining #2987 as well passes all 59 regression cases. Native combined state queries and pixels also pass with zero GL errors.

One detail on context safety: this change adds no state-mutating GL calls to invalidateState(). Its existing desktop draw/read-buffer queries remain.

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.

2 participants