Fix stale framebuffer attachments, leaked renderbuffers, and nonzero-default copies

[toaster0123]

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.

Validation

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

Relation to #2983

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.