Skip to content

Reapply cached render state after external OpenGL changes - #2993

Open
toaster0123 wants to merge 3 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/render-state-invalidation-20261002
Open

toaster0123 wants to merge 3 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/render-state-invalidation-20261002

Conversation

@toaster0123

@toaster0123 toaster0123 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The Renderer API documents invalidateState() for use after manual or external-library GL changes. A valid example is an external renderer disabling color writes, then handing control back to jME and invalidating its cache.

Previously, invalidation replaced cached values with assumed GL defaults. If the next requested state matched those defaults, jME skipped the GL call needed to restore it. The next draw could remain invisible or use the wrong depth, stencil, cull, blend, polygon-offset, line-width or desktop polygon-mode state. External write masks could also prevent a framebuffer clear.

Fix

RenderContext now owns both cached draw-state values and their validity. invalidate() preserves those values but marks them unknown; reset() remains an explicit reset to known fresh-context defaults. Existing public field types are preserved. There is no parallel invalid-category set in GLRenderer.

Each writer restores and validates only what it actually applies. Inactive depth functions and blend factors remain unknown until needed. Partial clearBuffers calls validate only their write masks.

The mesh path also consumes unknown line width for a default-width mesh, even without an intervening material-state apply. It preserves a known material width and elides repeated GL calls.

Two related corrections:

  • Initialize destination blend-factor caches to the actual GL default, Zero
  • Use the desktop/GLES profile when deciding whether to issue desktop polygon-mode and draw/read-buffer queries; real GLES adapters expose the Java GL2 interface too

User-visible blending correction

The first Additive or Custom One-One blend on a fresh context now applies the requested factors instead of incorrectly skipping them. For a half-alpha red source over opaque blue, the native regression changes from replacement red [255,0,0,128] to the requested additive magenta [255,0,255,255]. This needs no prior invalidation. Fresh Alpha blending and non-default controls are also tested.

Validation

  • Original 62-case proof: unchanged upstream and Fix stencil testing remaining enabled after it is disabled #2987 alone each fail 34 cases; the fix passes all 62
  • Revised standalone: 71 tests pass. These add cache lifecycle/ownership and default-mesh/material-width controls. The retained-value test specifies the requested architectural change, not a second pixel defect
  • Clean core/desktop/LWJGL3 builds plus effects/plugins checks and core Javadoc pass: 632 reported tests, zero failures/errors, two existing skips. No added-line Checkstyle warnings
  • Combining the exact published heads of Fix stencil testing remaining enabled after it is disabled #2987, Fix stale instance divisors when buffer spans change #2988 and Fix stale vertex divisors after external GL rendering #2989 with this revision passes 133 tests, including explicit stencil-reference/mask retention and divisor-cache cleanup contracts
  • Native Mesa testing covers GLES, desktop core GL and the real dual-interface GLES adapter shape. Each revised standalone/composed run set passes 213 phases, 222 state assertions and 216 pixel readbacks, plus 33 real mesh draws with native-width and raster-column checks. Zero mismatches or GL errors
  • Published first revision e16d377 reproduces external width 5 surviving a default-width mesh draw. Revised 4c89905 restores width 1; material width 3 remains 3; repeated draws make no line-width calls. All 18 frozen revision runs replayed successfully

Two independent source reviews found no blockers. Ordinary enabled-stencil call elision remains #2987's separate fix.

Composition needs explicit conflict resolution: retain #2989's per-slot invalidation after context.invalidate(), place #2987's stencil defaults in the draw-state reset helper, and keep #2988's numeric divisor reset in shared other-cache cleanup. Those exact resolutions are tested.

Limits

Scope is cached RenderState values and their shared clear/mesh consumers. Viewport/scissor/resource bindings, background clear color and capability-loader refactoring are outside this patch. Existing other-cache cleanup is preserved.

Native tests use a test-only JNA bridge and Mesa llvmpipe, not every GPU or a production backend. Wide-line rasterization is supported on the tested software driver, not guaranteed across hardware. A synthetic desktop adapter tagged as GLES is already misclassified by the upstream loader and is excluded from supported passing coverage.

The whole-repository local build remains blocked by the missing Android SDK. The checks above describe completed local scope; GitHub workflow status must be checked for the current head.

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

Copy link
Copy Markdown
Member

invalidateState() should make the cached GL state unknown, not reset the cache to assumed defaults. I'd rather represent that invalid/unknown state in RenderContext itself instead of maintaining a parallel set of invalid render-state categories in GLRenderer.

Comment on lines +3860 to +3861
if (mesh.getLineWidth() != 1f) {
setLineWidth(mesh.getLineWidth());

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 cleanup of the line-width path. One gap though: the guard still bails out when the mesh uses the default width of 1, so a LineWidth entry in invalidRenderState never gets consumed for those meshes. If external code widened the line before invalidateState(), the next default-width line mesh would render with that stale width — exactly the class of bug this PR is fixing. Please let the invalid flag drive the call too (and make sure setLineWidth removes the entry once it applies the value).

Suggested change
if (mesh.getLineWidth() != 1f) {
setLineWidth(mesh.getLineWidth());
if (mesh.getLineWidth() != 1f || invalidRenderState.contains(RenderStateCategory.LineWidth)) {
setLineWidth(mesh.getLineWidth());
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verified and fixed in 4c89905. The mesh guard now also checks whether RenderContext's LineWidth category is unknown, and setLineWidth validates it after the write.

The native reproduction calls invalidateState(), setShader() and renderMesh() with no intervening applyRenderState(): e16d377 leaves external width 5 and five red raster rows; the revision restores width 1 and one row. Repeating the draw emits no line-width call. Separate controls preserve a valid material width 3 for a default-width mesh, including after invalidation followed by material reapply. This passed on the three tested Mesa profiles; it is software-driver evidence, not a guarantee of wide-line support on other hardware.

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.

Confirmed on 4c89905 — the guard now reads !context.isRenderStateValid(RenderStateCategory.LineWidth) || mesh.getLineWidth() != 1f, so an unknown line width is consumed even for a default-width mesh, and setLineWidth re-validates right after the write. That closes the stale-width gap I flagged; thanks for adding the native raster-column check on top of the state assertions.

Remaining step is just CI — the checks for this head haven't started yet, so a maintainer approval is all that's needed.

@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 work — turning invalidateState() into explicit "unknown" flags per category instead of silently assuming GL defaults is the right fix, and the write-mask/clearBuffers plus mesh line-width consumers are nicely covered. Two things to look at before merge:

  • Default-width meshes can still skip the line-width reapply. The new mesh.getLineWidth() != 1f guard means an invalidRenderState entry for LineWidth is never consumed when a mesh uses width 1. If external code changed the line width and then invalidateState() was called, the next default-width line mesh draws with the stale external width — the same bug class this PR targets. The invalid flag should also drive that call, and setLineWidth should clear the flag when it applies.
  • dfactorAlpha default change is worth a second pair of eyes. Switching from One to Zero matches the GL default (GL_ZERO), and it does fix fresh Additive/Custom One-One blending. But this is a user-visible behavior change in a widely used engine, so it deserves an explicit note in the changelog and ideally a test asserting the first blend with no prior invalidation produces the right pixels.

One unrelated but useful data point: the check status for the head commit isn't resolvable (404 from the commit-status API), so I couldn't independently confirm the test/build results in the description.

Copy link
Copy Markdown
Contributor Author

@riccardobl I agree with the ownership point. Revised in 4c89905: RenderContext now owns category validity alongside its cached values, and GLRenderer has no parallel validity set. invalidate() retains the draw-state values but marks them unknown; reset() explicitly means known fresh-context defaults. Existing public field types are preserved, and constructor initialization uses private helpers.

Writers validate only the state they apply. Disabled depth/blend parameters stay unknown, partial clears only validate their masks, and the combined tests preserve the stencil-reference/mask and vertex-divisor cleanup contracts from the other PRs. Two independent reviews found no blockers; 71 focused tests and 133 combined tests pass. The exact revised sources also passed the Mesa state/pixel tests. The PR description now makes the ownership distinction and scope explicit.

Copy link
Copy Markdown
Contributor Author

@jaime-jmebot The fresh-blend concern was independently rechecked. Both destination caches now match GL_ZERO. The native first-draw tests use a fresh context with no invalidation: Additive and Custom One-One previously produce [255,0,0,128] over opaque blue instead of the requested [255,0,255,255]. The revised sources produce the expected pixel on desktop GL, GLES and the dual-interface GLES adapter. Fresh Alpha blending is also a control: factors [SRC_ALPHA,ONE_MINUS_SRC_ALPHA,SRC_ALPHA,ONE_MINUS_SRC_ALPHA], pixel [128,0,127,191], and no repeated-state GL calls.

I added an explicit user-visible blending correction note to the PR description. I did not find a tracked changelog/release-note file in this checkout, so I have not introduced a new repository-wide changelog convention.

For CI, the original e16d377 workflows completed successfully, but that result does not apply to the new head. Current 4c89905 workflows are awaiting approval (Build run 37020980155 and Android run 37020980665, both action_required). Local validation on this exact revision passed 632 reported tests with two existing skips, relevant builds/style and core Javadoc. The local whole-repository build is still blocked by the unavailable Android SDK.

@jaime-jmebot

Copy link
Copy Markdown
Contributor

Checked 4c89905 — the line-width gap is closed on my side.

  • Mesh guard: the draw path now calls setLineWidth(mesh.getLineWidth()) when !context.isRenderStateValid(RenderStateCategory.LineWidth) or the mesh width isn't 1, and setLineWidth marks the category valid right after the write. So an external width survives invalidateState() no longer, and repeated default-width draws stay call-free. Good fix.
  • Blend defaults: dfactor/dfactorAlpha at GL_ZERO matches the real GL default, and now that RenderContext owns validity and invalidate() retains values instead of rewriting them, the correction is cleanly confined to the constructor/reset() path. Pixel evidence on desktop GL, GLES and the dual-interface adapter is convincing. I agree there's no changelog file to update here.
  • Remaining step is CI, not code: the checks for this head are still sitting at action_required (Build 37020980155, Android 37020980665), so there's no green signal on 4c89905 yet — I can't confirm the local run results independently either. A maintainer approval would unblock that.

Once CI is green on this revision I'm happy to approve. Nice work keeping the ownership clean and the tests tied to the real regression.

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

Both points from my earlier review are addressed in 4c89905.

  • Line width: the mesh guard now also fires when the LineWidth category is unknown, so external widths no longer survive an invalidateState() into a default-width mesh draw, while known material widths and repeated draws still elide the GL call.
  • Blend defaults: destination factors start at GL_ZERO (the real GL default), confined to the constructor/reset() path now that invalidate() retains values. Tests cover the fresh-context Additive/One-One pixels plus controls, and the behaviour change is called out in the PR description — no changelog file exists in the repo to update.

Architecture is clean: validity lives with the values in RenderContext, each writer validates only what it applies, and partial clearBuffers only validates its own masks. Nothing further from me — the checks for this head haven't started yet, so a maintainer run/approval would give the final signal.

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.

3 participants