[toaster0123]

Latest follow-up and review

Current head: a485967fb37674260a53a915f0afaed951ba736a.

• 56e53704be8e4fcbdfddc786cfe9a1e95f23852d: preserve screen rendering without framebuffer-object support. After invalidation, reset or cleanup, setFrameBuffer(null) restores the cache without calling unsupported FBO functions. Real offscreen targets and main overrides still throw before allocation.
• Also avoid querying GL_FRAMEBUFFER_BINDING during initialization when Caps.FrameBuffer is absent. A strict OpenGL 2.1 recording fixture checks both unsupported queries and entry points.
• a485967fb37674260a53a915f0afaed951ba736a: integrate current upstream master (f89b4e0) into this PR branch. Resolve the conflict in invalidateState() by keeping both unknown-FBO binding and upstream vertex-divisor invalidation. This does not merge the PR into master.

Latest validation

Added 13 regression tests: seven legacy-backend cases (all failed before the fix) and six previously local lifecycle edge cases, now committed. The edge cases cover distinct texture wrappers sharing an Image, stale phantom references and native-name reuse, cleanup of 151 mixed framebuffers, reset/reinitialization, texture-to-renderbuffer replacement and explicit UNSAFE cleanup.

Validated the integrated sources with:

./gradlew :jme3-core:test :jme3-effects:test :jme3-lwjgl3:test :jme3-core:checkstyleMain :jme3-core:checkstyleTest

• Core: 747 passed, 1 existing skipped.
• Effects: 6 passed. LWJGL3: 4 passed.
• Total: 757 passed, 1 skipped, zero failures/errors. The increase also includes upstream tests brought in by the merge; only 13 tests were added in this follow-up.
• 158 targeted framebuffer, ownership, native-disposal and vertex-divisor tests also pass with Java assertions disabled.
• Core main/test Checkstyle completed with no warnings on PR-modified lines. The PR diff against current master passes git diff --check; unrelated existing upstream whitespace was left unchanged.
• Verified source hashes stayed unchanged during validation.

The follow-up review covered ownership, shared-image lifetime, deferred/GC cleanup, native-name reuse, attachment snapshots, failure recovery, legacy fallback, and the upstream conflict resolution. No additional confirmed blocking regression was found in that pass. The exclusive-target contract is unchanged; use distinct targets to share a texture.

GL checks use recording fixtures. Native GPU rendering, pixel output and FPS were not validated; the sandbox's EGL/GLX context setup remains unavailable. GitHub CI for this head is separate from the local results above. Earlier results below are historical.

---

Review follow-up — 2026-10-26

Commit: 7e86c0f — Enforce framebuffer target ownership and reduce lifecycle overhead.

Target ownership contract

• A target instance belongs exclusively to the first framebuffer to which it is added. Supplying it to another framebuffer throws IllegalArgumentException in the add/replace/depth API, before changing either framebuffer or the target's slot/face.
• The same target cannot occupy multiple slots simultaneously. Setting the existing target again in its current slot is allowed.
• Ownership remains after removal, replacement, reset and disposal, because native detachment/deletion is deferred. A removed target can be re-added to its original framebuffer, but cannot be transferred to another. Create fresh target instances when rebuilding a framebuffer.
• Textures remain independently owned: create two distinct targets referencing the same texture to share it. Tests cover shared color and depth textures surviving framebuffer disposal.
• Ownership uses an independent opaque identity token, not a strong reference to the framebuffer or its native-object phantom handle. A real Java GC regression verifies that retaining a target does not prevent owner collection and native cleanup.

Bookkeeping and cleanup

• Use the existing primitive-key IntMap for framebuffer state, an attachment-slot bitmask, and reusable renderbuffer records with primitive cached formats instead of boxed maps and per-update temporary hash sets.
• Pass state through attachment updates and use indexed mipmap iteration to avoid repeated registry lookups and iterator allocation.
• Deduplicate disposal requests by object identity and avoid rescanning the remaining queue during normal managed deletion. Preserve cancellation of queued requests on direct renderer deletion.
• Clear stale disposal requests during context reset and full cleanup, so they cannot delete a resource recreated afterward. Preserve the per-frame deletion limit.

Earlier validation (historical)

Executed locally on this revision:

./gradlew :jme3-core:test :jme3-effects:test :jme3-core:checkstyleMain :jme3-core:checkstyleTest

• Core: 605 passed, 1 existing skipped. Effects: 6 passed. Total: 611 passed, 1 skipped, zero failures/errors.
• 23 new regression tests cover exclusive ownership, failure atomicity, shared-texture survival, actual Java GC reachability, slot-mask boundaries, duplicate disposal, batch limits, context reset and full cleanup.
• Before the production changes, the targeted regression run reported 15 failures out of 50 tests, covering the newly required ownership rejection and two reproduced stale-deletion-queue bugs. The fixed full suite passes.
• Core main/test Checkstyle and git diff --check completed. Existing repository warnings remain; no Checkstyle warnings on the added/modified lines.

GL behavior is checked with recording-GL fixtures. No native GPU rendering, driver-memory measurements or frame-rate benchmark is claimed. The original validation below is historical.

---

What is broken

This fixes three framebuffer API behaviors: changing a render target can leave OpenGL using the old attachment, disposing a framebuffer can leave its renderbuffers undeleted, and copying to/from the screen can use the wrong framebuffer when the platform's default ID is nonzero.

1. Replacing or removing an initialized attachment does not update OpenGL

Trigger: Render into a framebuffer, then change its targets through replaceColorTarget, removeColorTarget, or the related add/depth setters. A caller expects the next setFrameBuffer(fb) to apply the new targets.

For example, after fb has been initialized with texture A:

fb.replaceColorTarget(0, FrameBuffer.FrameBufferTarget.newTarget(textureB));
renderer.setFrameBuffer(fb); // Subsequent rendering should target B.

Before: The recording-GL reproduction still has A attached. The setters do not mark the framebuffer for an update; switching away and back does not help. Removal has a second bug: even after an explicit fb.setUpdateNeeded(), the removed color slot remains attached because the update code only visits surviving targets. Rendering can therefore go to the old texture, and the native attachments can disagree with the Java target list.

Fix: Mark target/MRT changes for update, explicitly detach removed slots, and reattach surviving renderbuffers when their slot changes. Keep the selected color target valid after removal. When switching away, generate mipmaps for the textures that were actually attached, rather than a newly replaced Java target. Regression tests assert that B becomes attached and removed slots become zero.

2. Framebuffer disposal misses owned renderbuffers

Trigger: Dispose a framebuffer with several color renderbuffers, or let the native-object manager clean it up. Callers expect every renderbuffer owned by that framebuffer to be released.

Before, measured in the GL-call recording:

• Two color renderbuffers plus depth allocate 3 renderbuffer IDs. fb.dispose(); renderer.postFrame(); deletes only 2, leaving the second color renderbuffer live.
• One color renderbuffer plus depth allocate 2 IDs. renderer.cleanup() deletes the framebuffer but 0 renderbuffers: the manager's destructible framebuffer clone contains no attachment information.

These missed deletes can retain resources while the GL context remains alive. Copying attachment IDs into the clone once is insufficient: registration occurs before initial attachment allocation, and targets may change later.

Fix: Track allocated renderbuffers in renderer-owned state keyed by framebuffer ID, without retaining the owning FrameBuffer. Direct disposal, managed cleanup, and the GC-queue path can then delete all owned IDs, including late additions, removed targets not yet rebound, and allocations made before a later failure. Unregister direct deletions and reject stale queued references so a reused GL ID does not cause deletion of a new object. Tests now record all 3/3 and 2/2 deletions, and also cover context reset, ID reuse, and simulated GC cleanup. Texture attachments remain separately owned.

3. Screen copies bind framebuffer zero instead of the discovered default

Trigger: The platform starts with framebuffer 77 bound, so initialization records 77 as the screen framebuffer. With no main-framebuffer override, a null copy endpoint should refer to that same framebuffer.

renderer.copyFrameBuffer(offscreen, null, true, false); // Copy to the screen.
renderer.copyFrameBuffer(null, offscreen, true, false); // Copy from the screen.

Before: Normal screen binding uses 77, but at glBlitFramebufferEXT the recorded draw/read endpoint is 0. The copy therefore targets a different framebuffer; it cannot be relied on to copy to/from the presentation surface.

Fix: Use the discovered default ID for null copy endpoints, and keep initialization, restoration, bound-framebuffer deletion, and main-framebuffer overrides consistent with it. The tests now record 77 at the relevant blit endpoint and verify restoration; the default-zero control still passes.

Initial validation (historical)

• Initial pre-fix audit on the earlier Fix render-target mipmap storage reuse and GLES3 half-float filtering #2983-based branch: 10 recording-GL cases, 7 failures and 3 passing controls. The standalone validation below was run separately on this PR.
• Expanded regression suite on this independent PR: 34/34 passed (14 attachment, 14 lifecycle, 6 default-framebuffer cases).
• Core: 516 passed, 1 existing skipped. Effects: 6 passed. Total: 522 passed, 1 skipped, including the 34 regressions; zero failures/errors.
• Core main/test checkstyle and git diff --check completed. Existing checkstyle warnings remain; no new warnings in the added tests or production lines. Independent review found no blocking issues.

These tests record GL calls, attachment IDs, and resource lifetimes. They do not rasterize pixels or measure driver memory. No native GPU, iOS hardware, or visual-output validation is claimed.

Original relation to #2983 (historical)

This branch is based directly on master and excludes #2983's mipmap null-storage fix, GLES3 half-float change, and their tests. It passes independently. Combining the PRs has one expected textual overlap in the mipmap loop: retain this PR's actual-attachment snapshot iteration and #2983's image-binding/generation body.

The earlier fork branch fix/framebuffer-attachment-resource-lifecycle is retained unchanged; this independent branch supersedes it for upstream review.

[jaime-jmebot]

Nice work on this one — the three fixes map cleanly onto real API expectations, and the recording-GL tests (attachments, renderbuffer lifetimes, blit endpoints) are the right kind of proof for this: they assert the actual GL calls rather than just Java-side bookkeeping.

A few things worth a look:

• NativeObjectManager queue guard — the new if (refMap.get(ref.objClone.getUniqueId()) != ref) continue; is a good guard, but it dereferences objClone before checking it's still there. Since unregisterObject now clears the reference, a stale queued Reference could reach this branch with objClone == null. A cheap ref.objClone == null || on the same condition makes it null-safe with no behavior change otherwise.
• Track map cleanup — could you confirm the renderbuffer bookkeeping keyed by framebuffer ID is also dropped in deleteFrameBuffer, alongside the glDeleteFramebuffersEXT call? If an entry survives and a later framebuffer is handed the same GL name, it would inherit renderbuffer IDs it never allocated — the exact id-reuse hazard this PR already guards against in refMap.
• Minor nits (no action strictly required):
  • FrameBuffer.clearColorTargets(): the if (colorBufIndex >= 0) branch reads like a leftover from an older version — the field starts at 0, so the condition is always true and colorBufIndex = 0; could be a plain assignment.
  • In mipmappedReplacementGeneratesOldRenderedTextureBeforeAllocatingNewOne, the old texture's mipmaps are generated twice (once when the slot is replaced, again on unbind). Harmless, but it's a full glGenerateMipmap on a texture nothing will read again.

When this gets combined with #2983, the mipmap loop is the one spot worth merging by hand: keep this PR's actual-attachment snapshot iteration with #2983's image-binding/generation body, as you noted.

[toaster0123]

I checked these points against f1646c1 and reran the 34 attachment/lifecycle/default-framebuffer regressions; all pass.

• The queue guard retains a valid clone. objClone is a separate final field initialized by createDestructableClone(); the phantom referent passed to the superclass is obj.handleRef. The inherited Reference.clear() clears that referent, not the objClone field. The existing directDeletionUnregistersOldCloneBeforeFramebufferNameReuse test explicitly enqueues the old reference, directly deletes/unregisters its framebuffer, reuses the GL name, then drains the queue and checks that the new framebuffer survives. Reference fields and constructor, queue/reuse regression.

• deleteFrameBuffer already removes the bookkeeping entry with frameBufferStates.remove(fb.getId()) before deleting owned renderbuffers and the framebuffer name. A subsequent allocation installs a fresh FrameBufferState. Managed cleanup, simulated GC, direct deletion/name reuse, and context reset are covered. Deletion path.

• The colorBufIndex condition preserves MRT: setMultiTarget(true) sets it to -1. An unconditional assignment to 0 in clearColorTargets() would disable that mode. MRT sentinel.

• The replacement test records one generation for the old texture during replacement, then one for the replacement on unbind. Its asserted sequence is [old ID, replacement ID], not two calls for the old texture. The removed texture can also remain application-owned and be sampled later. Exact assertions.

For the eventual combination with #2983, agreed: retain this PR's actual-attachment snapshot iteration and #2983's direct image binding, mip-range restoration and generated-state update. The standalone attachment test checks texture IDs; it does not claim to cover #2983's storage-redefinition or pixel-preservation behavior. No source change was needed for these review points. Both current-head GitHub workflows also pass.

[jaime-jmebot]

• Excellent set of fixes addressing tricky framebuffer lifecycle, attachment update, and default framebuffer copy edge cases.
• Comprehensive recording-GL tests validate the exact GL calls and resource lifetimes.
• Code changes are clean, well-tested, and robust.

[jaime-jmebot]

• The code changes are extremely clean, thorough, and fully covered by robust recording-GL and ownership regression tests.
• All review items and edge cases have been verified and confirmed.
• Great work! Ready to go.

[toaster0123]

Fixed the second-review findings in f0c3d4b, pushed directly to this PR branch (no merge).

Fixes

1. Deleted-image mipmap snapshots: store the actual attached Image and native name per color slot. Image deletion invalidates every matching framebuffer snapshot and texture-unit cache before the name can be reused. Mipmap generation skips invalid images, and Texture.setImage(...) no longer makes the snapshot refer to an unallocated replacement. Snapshot records are reused; untouched slots remain accurate after a partial attachment-update failure.
2. Pending native CPU-buffer disposal: context reset and full cleanup now drain the remaining explicitly queued CPU-buffer releases when NativeObjectManager.UNSAFE is enabled, without issuing GL deletes against the lost context. Direct unregistration also preserves queued CPU cleanup. Managed deletion does not double-free, and the ordinary per-frame deletion limit and identity deduplication remain intact.
3. Public low-level attachment updates: updateFrameBufferAttachment() and updateRenderTexture() validate current target membership and synchronize through the full framebuffer-update path. This initializes/binds a new framebuffer and keeps pending removals, actual attachments, cleanup state and mipmap snapshots consistent. The full-reconciliation behavior is documented; the normal renderer uses private internal helpers.

Also corrected default-framebuffer binding/copy/override behavior after state invalidation, including a nonzero presentation framebuffer. Target exclusivity and sharing textures through separate target instances are unchanged.

Validation on the pushed sources

• Added 26 regression tests: 14 image/snapshot cases, 9 native-disposal cases and 3 default-framebuffer invalidation cases.
• Full core/effects suite: 648 passed, 1 existing skipped, zero failures/errors (core 642 passed + 1 skipped; effects 6 passed).
• 32 targeted tests also pass with Java assertions disabled.
• Core main/test Checkstyle and git diff --check completed; no Checkstyle warnings on the changed lines. Existing repository warnings remain.
• Verified that the six committed source files were unchanged throughout the final validation.

The original 12 new snapshot reproductions failed before the production fix. The fixed tests cover deletion/disposal, native-name reuse, shared images, direct updates, partial failures, CPU cleanup and stale-request safety. GL validation uses recording fixtures; no native GPU rendering, driver-memory measurement or FPS benchmark is claimed.