Repository navigation
Reapply cached render state after external OpenGL changes - #2993
toaster0123 wants to merge 3 commits into
Conversation
|
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. |
| if (mesh.getLineWidth() != 1f) { | ||
| setLineWidth(mesh.getLineWidth()); |
There was a problem hiding this comment.
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).
| if (mesh.getLineWidth() != 1f) { | |
| setLineWidth(mesh.getLineWidth()); | |
| if (mesh.getLineWidth() != 1f || invalidRenderState.contains(RenderStateCategory.LineWidth)) { | |
| setLineWidth(mesh.getLineWidth()); | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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() != 1fguard means aninvalidRenderStateentry forLineWidthis never consumed when a mesh uses width 1. If external code changed the line width and theninvalidateState()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, andsetLineWidthshould clear the flag when it applies. dfactorAlphadefault change is worth a second pair of eyes. Switching fromOnetoZeromatches the GL default (GL_ZERO), and it does fix freshAdditive/Custom One-Oneblending. 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.
|
@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. |
|
@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. |
|
Checked
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
left a comment
There was a problem hiding this comment.
Both points from my earlier review are addressed in 4c89905.
- Line width: the mesh guard now also fires when the
LineWidthcategory is unknown, so external widths no longer survive aninvalidateState()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 thatinvalidate()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.
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
RenderContextnow 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 inGLRenderer.Each writer restores and validates only what it actually applies. Inactive depth functions and blend factors remain unknown until needed. Partial
clearBufferscalls 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:
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
e16d377reproduces external width 5 surviving a default-width mesh draw. Revised4c89905restores width 1; material width 3 remains 3; repeated draws make no line-width calls. All 18 frozen revision runs replayed successfullyTwo 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.