[toaster0123]

Problem

Renderer.invalidateState() must not reset the Java cache to assumed defaults while leaving the corresponding native bindings unchanged. In particular, an external renderer can leave texture unit 3 active, so a subsequent jME texture operation on unit 0 affects the wrong unit. Framebuffer selection and temporary attachment/copy operations must also have a consistent binding after invalidation.

Change

This revision follows the immediate-reset approach merged in #2993 instead of introducing unknown-binding state:

• GLRenderer.invalidateState() unconditionally selects texture unit zero and, when framebuffer objects are supported, binds the platform's defaultFBO for both reading and drawing. The default can be nonzero.
• Restore the platform framebuffer's configured sRGB write state directly, without trusting the previous cache, on backends supporting sRGB write control.
• Reset RenderContext and record the same native defaults. Texture references are forgotten so textures are rebound when used; there is no native loop unbinding every texture unit.
• Remove the PR-added framebuffer-validity flag, unknown texture-unit sentinel and all four binding-knowledge methods. RenderContext now matches the base branch exactly.
• Simplify copy and attachment restoration to the known cached binding. No additional native binding queries or validity checks in ordinary rendering paths.
• Preserve early rejection of unsupported framebuffer operations and nonzero platform framebuffer handling.

The existing #2993 draw-state reset remains unchanged. The previous SDL screenshot-test teardown fix is retained.

Contract and lifecycle

Invalidation now immediately replaces external read/draw framebuffer bindings with the platform default; it does not preserve those external bindings. A configured main-framebuffer override is retained, but is not selected or allocated by invalidation. This is important because cleanup() and resetGLObjects() also call invalidation and must not recreate resources they just deleted/reset. Call setFrameBuffer(null) to select a configured override before resuming rendering, or explicitly select another target.

The public API documentation describes this contract and requires a current graphics context on the rendering thread. Viewport/scissor resets, external mutation of texture/attachment contents, and unrelated OpenGL state categories are outside this change.

Cost and compatibility

At invalidation: one active-texture call, one framebuffer bind when supported, and one sRGB enable/disable when supported. The existing two desktop draw/read-buffer-selection queries are retained and run after binding the platform framebuffer. No new binding queries are introduced, including in copies and attachment updates after invalidation. Repeated texture/framebuffer operations retain ordinary cached call elision.

Public/protected GLRenderer signatures were compared using javap and are unchanged from the base. RenderContext is source-identical to the base, and Renderer changes only documentation.

Validation

Implementation commit: db744371d0b75dcc4ae52a86f962c3d2bd707c5e. Head e436c07e8be6112966829e877d0a49eb7b2644a7 only wraps a test method signature for Checkstyle; full unit suites and test Checkstyle were rerun successfully on that head.

• All 416 renderer tests pass, including 65 focused binding/reset cases.
• Full core, desktop, effects and plugins suites: 1,140 reported tests, zero failures/errors, one existing skip. No test exclusions were applied (core 1,064; desktop 26; effects 6; plugins 44).
• Negative control: the exact same 65 focused tests run with the base GLRenderer (8abcb3ee) produce 39 failures and 26 passing controls; all 65 pass with this implementation.
• Focused coverage includes native-state modeling immediately upon return, repeated external changes despite a default-valued cache, direct clears, zero/nonzero platform FBOs, retained overrides, cleanup/context-reset without allocation, texture uploads/parameters, sRGB in both directions, unsupported capabilities, and query-free copy/attachment restoration including attachment failures.
• Java compilation succeeds for LWJGL2, LWJGL3, Android and iOS. Production core was also independently compiled with --release 8 and the focused tests run through JUnit.
• Core Javadoc and Checkstyle succeed. No warnings on added lines remain; existing repository Checkstyle warnings are unchanged.
• An independent read-only source review found no introduced blockers.

A local native TestPresentationMsaa attempt could not create the SDL window (Could not get EGL display), before renderer initialization; the devbox's standalone glxinfo also cannot find a GLX visual. This is not counted as a passing native test. Backend rendering validation belongs to the new CI runs: Build jMonkeyEngine 41053318909 and Android Screenshot Test 41053318578. Read their current status rather than relying on the previous revision's green runs.

[jaime-jmebot]

Nice work — the diagnosis is right: resetting the cache to "unit 0 / main framebuffer" without restoring the native binding is exactly the kind of thing that bites users after they embed another GL renderer. Moving "we don't know" into RenderContext is the correct shape for the fix, and the fresh-default / repeated-call controls are a good guard against regressing into redundant binds.

A few things I'd like tightened before this lands:

1. invalidateBindings() clears the binding knowledge but leaves boundFBO stale

boundTextureUnit = -1;
frameBufferBindingValid = false;
boundFB = null;          // cleared
// boundFBO keeps the old FBO name

boundFB/boundFBO are public fields, so a consumer that forgets isFrameBufferBindingValid() sees "no framebuffer object bound, but FBO 12" — a combination that can't happen natively. Since the validity flag alone is enough to force the next real bind, I'd reset it here too:

boundFBO = 0;

That means RenderContextBindingTest.invalidateBindingsLeavesFramebufferNameUnchanged should flip to asserting 0 — fine, it was only pinning the current behaviour.

2. The new public setter is a footgun of the same kind this PR fixes

setFrameBufferBindingValid(true) lets anyone declare the cache valid without an actual glBindFramebuffer, which re-creates the original bug from the other side. A single call that records all three values at once can't drift:

public void setFrameBufferBinding(FrameBuffer fb, int fbo) {
    this.boundFB = fb;
    this.boundFBO = fbo;
    this.frameBufferBindingValid = true;
}

Symmetry point: boundTextureUnit = -1 is an undocumented sentinel with no isTextureUnitBindingValid() counterpart, so nothing stops a future GL_TEXTURE0 + context.boundTextureUnit from producing an invalid enum. A small accessor (or at least a comment on the field) would close that.

3. Quick check: does reset() clear the flag too?

Only init() appears to set frameBufferBindingValid = true in this diff. copyContext starts by resetting the destination context, so a stale true alongside boundFB == null would be exactly the broken state — worth confirming (or adding the reset there).

4. sRGB follow-up, understood as out of scope

With an unknown binding, both toggles in setMainFrameBufferSrgb are now skipped, which is the safe direction. The open question is the other end: when jME later re-binds the main framebuffer, does anything re-apply the cached mainFrameBufferSrgb, or does the native state stay stale until the app toggles sRGB again? No blocker here — just worth a tracked issue if the answer is "nothing re-applies it".

5. Small test nit that would have real value

The recording driver already guards against bogus framebuffer IDs:

assertTrue(framebuffer >= 0, "Unknown framebuffer ID passed to GL");

Adding the same check for the active unit closes the symmetric hole and turns a leaked -1 into a failing test instead of a silently recorded activeUnit = -1:

if (name.equals("glActiveTexture")) {
    int unit = (Integer) args[0] - GL.GL_TEXTURE0;
    assertTrue(unit >= 0, "Unknown texture unit passed to GL");
    activeUnit = unit;
}

Overall the approach looks right and the control set (fresh defaults, repeated invalidation, explicit targets, nonzero presentation target, copies with separate read/draw bindings) is solid.

[jaime-jmebot]

• The root cause and the direction of the fix look right: carrying "unknown" binding knowledge in RenderContext and only eliding a bind when the cache is genuinely valid is the clean way to solve it.
• Fresh-context defaults and repeated-call elision are preserved, and the control set around them (explicit targets, nonzero presentation target, copies with separate read/draw bindings) gives good confidence there's no extra-bind regression.
• Small hygiene items before merge: clear boundFBO alongside boundFB in invalidateBindings(), consider replacing setFrameBufferBindingValid(boolean) with a single call that records framebuffer + name + validity together, and confirm reset() clears the validity flag too. Details and a suggested test guard are in my comment above.
• sRGB after an unknown binding is intentionally left out of scope — just want the "is the cached mainFrameBufferSrgb re-applied on the next main-target bind?" question tracked.

[toaster0123]

@jaime-jmebot Addressed the recording-helper and test suggestions in 809b988.

• Known bindings now use one method to record the Java association, native name and validity together. A separate framebuffer-only invalidator preserves texture knowledge during copy restoration. Tests also cover known deletion followed by null selection with a nonzero presentation target; the shortcut now checks the expected native name as well as object identity.
• I kept boundFBO as an explicitly untrusted value while invalid. Null boundFB means no Java association, not necessarily native name zero: a nonzero presentation or externally managed FBO can legitimately have that shape. Resetting the number to zero would not make it safe for a consumer to ignore validity. The field/method docs and tests now make this distinction explicit.
• reset() calls init() and deliberately restores known fresh-context defaults, including valid=true. External invalidation then marks binding knowledge unknown. The reset contract is tested; there is no copyContext path in this checkout.
• The active-unit field already documents -1 as unknown. The recording driver now immediately rejects a negative unit passed to GL, as suggested.
• An explicit later setFrameBuffer(null) reaches toggleFramebufferSrgb and applies the existing main-target policy. Main-target metadata is no longer applied merely because a Java association is null. General external sRGB-state invalidation remains outside this patch.

Validation on this exact revision: 49 focused tests, 610 scoped module tests (two existing skips), 662 composed core tests (one skip), style/Javadoc, renewed independent review, and native Mesa state/pixel controls pass. Unchanged selections still elide calls. Standalone nonzero-default initialization can make one conservative initial bind; the #2984 composition already aligns that cache.

Current-head CI still awaits maintainer approval: Build 37032810713 and Android 37032810846. I am not treating earlier workflow status as validation of this revision.

[jaime-jmebot]

Checked 809b988 against the points from my earlier comment — all substantive ones are addressed, and I agree with the reasoning on the rest:

• init() (reached via reset()) now sets frameBufferBindingValid = true next to the boundFB/boundFBO defaults, so a reset context is a genuinely known one and no stale "valid but no Java association" state can survive a reset.
• The two-value boolean setter is gone in favour of a single method that records object, native name and validity together, plus a framebuffer-only invalidator so copy restore keeps texture knowledge. That removes the "declare it valid without a bind" footgun.
• I accept your call on boundFBO staying untrusted while invalid. A null boundFB genuinely can coexist with a nonzero native name (nonzero presentation target, externally managed FBO), so zeroing the int wouldn't make ignoring the validity flag safe — it would just be a different lie. The documented "untrusted" contract plus tests is the honest version of that.
• The recording driver now rejects a negative active unit, which is exactly the symmetric guard I wanted.

No further code comments from me. Two non-blocking follow-ups worth tracking when there's time: whether the cached mainFrameBufferSrgb gets re-applied when the main target is bound again, and the one conservative initial bind on standalone nonzero-default init that only disappears once composed with #2984. Nice work on the control set — fresh defaults and unchanged-selection elision are both still pinned.

[jaime-jmebot]

The revised implementation restores native texture and framebuffer defaults immediately, while keeping the documented override and cleanup behavior intact.

• The binding-state machinery from the intermediate revision is removed; invalidation and cached state now align with the stated contract.
• Focused coverage exercises invalidation, nonzero default framebuffers, texture operations, and copy/attachment restoration.
• I found no additional blocking issues in this pass.