Skip to content

Warn when a second GL context is registered without useMultipleContexts#133

Merged
Renanse merged 1 commit into
masterfrom
fix/warn-single-context-multi
Jun 20, 2026
Merged

Warn when a second GL context is registered without useMultipleContexts#133
Renanse merged 1 commit into
masterfrom
fix/warn-single-context-multi

Conversation

@Renanse

@Renanse Renanse commented Jun 20, 2026

Copy link
Copy Markdown
Owner

What

ContextManager.addContext now logs a one-time WARNING when more than one distinct GL context is registered while Constants.useMultipleContexts is false.

Why

The engine's per-context GL-resource keying is correct by design: VAOs are keyed by RenderContext's unique ref, while buffers/textures/programs are keyed by the sharable ref (matching what GL actually shares between contexts). But all of that keying is gated behind Constants.useMultipleContexts, which is read once at class-load from -Dardor3d.useMultipleContexts and defaults to off.

When it's off, ContextValueReference.get/put ignore the per-context key entirely and read/write a single value:

public U getValue(final RenderContextRef glContext) {
  if (Constants.useMultipleContexts) {
    return _valueCache.get(glContext);   // keyed by context
  } else {
    return _singleContextValue;          // ref ignored — one value for ALL contexts
  }
}

So an app that runs multiple GL contexts but forgets the flag is in a quietly-broken state:

  • Buffers / textures / programs survive it — the contexts genuinely share those at the GL level, so reusing one id is accidentally fine.
  • VAOs do not — they aren't shareable across contexts. A VAO genned in context A and then handed back for context B is invalid there → GL_INVALID_OPERATION or missing/garbage geometry on the second context.

Nothing surfaced this. addContext is the one place that knows how many contexts are live, so it's the natural spot to turn a silent misconfiguration into an actionable log line that names the fix (-Dardor3d.useMultipleContexts).

Notes

  • Not a behavior change — purely a diagnostic. No fix is needed (or attempted) for the keying itself; it's correct when the flag is set.
  • The warn-latch is volatile and fires once. addContext is otherwise unsynchronized by existing design, so a narrow check-then-set race could log twice — harmless for a one-time diagnostic; full locking just this one boolean while the backing WeakHashMap stays unsynchronized would be inconsistent.
  • Decided against making the flag dynamic: it's a deliberate perf/JIT gate, and a startup warning gets the same benefit at far less risk.

Test

TestContextManagerardor3d-core's first coverage for this class. Verifies a single context (and same-key recreation, e.g. on resize) stays quiet, and a second distinct context warns exactly once. Red→green; full ardor3d-core suite green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Implemented diagnostic warning system that automatically detects and logs when multiple rendering contexts are inadvertently registered in single-context mode, improving reliability and safety.
  • Tests

    • Added comprehensive test suite validating multi-context detection behavior, covering registration scenarios, warning notifications, and mode configuration handling.

ContextManager.addContext registered contexts silently. When an app runs more
than one GL context but does not set -Dardor3d.useMultipleContexts, the engine
is in a quietly-broken state: ContextValueReference ignores its per-context key
and keeps a single value, so non-sharable GL resources (VAOs, FBOs) created in
one context are handed back for another. Shared resources (buffers, textures,
programs) survive that because the contexts genuinely share them at the GL
level, but VAOs are not shareable - so a VAO genned in context A and bound in
context B is invalid there, corrupting rendering on the second context.

The per-context keying (VAOs by unique ref, buffers by sharable ref) is correct
by design, but it is a no-op unless useMultipleContexts is on, and nothing told
the user. addContext is the one place that knows how many contexts are live, so
warn once there when a second distinct context appears while the flag is off,
pointing at the fix. This is a one-line diagnostic, not a behavior change - it
turns a silent misconfiguration into an actionable log line.

The latch is volatile so a set on one registration thread is visible to another;
a narrow check-then-set race could still log twice, which is harmless for a
one-time diagnostic (the class is otherwise unsynchronized by existing design).

Red->green test (ardor3d-core's first ContextManager coverage): a single context
and same-key recreation stay quiet; a second distinct context warns exactly once.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

ContextManager adds a volatile warning latch, a Logger, and the warnIfUnsafeMultiContext() method that emits a single WARNING log when more than one distinct RenderContext is registered while Constants.useMultipleContexts is disabled. A new TestContextManager JUnit class verifies the exact warning counts for all relevant registration scenarios.

Changes

Multi-context detection and one-time warning

Layer / File(s) Summary
warnIfUnsafeMultiContext() implementation
ardor3d-core/src/main/java/com/ardor3d/renderer/ContextManager.java
Copyright year bumped to 2026; HashSet/Set imports and a Logger field added; _warnedSingleContextMode made protected static volatile; warnIfUnsafeMultiContext() counts distinct RenderContext values in contextStore and logs a warning exactly once when more than one distinct context exists in single-context mode.
Warning-count tests
ardor3d-core/src/test/java/com/ardor3d/renderer/TestContextManager.java
New TestContextManager class isolates static state per test, captures WARNING records via an in-memory Handler, and asserts: zero warnings for a single context, zero for same-key re-registration, exactly one warning on a second distinct context in single-context mode (no increment on a third), and zero warnings when multi-context mode is enabled.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops 'twixt GL contexts two,
But single-mode flags say "this just won't do!"
One warning is logged, and never again,
The latch holds its breath through each render domain.
🐇✨ No double-warns, the logger stays sane!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main change: adding a one-time diagnostic warning when a second GL context is registered without the useMultipleContexts flag enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/warn-single-context-multi

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ardor3d-core/src/main/java/com/ardor3d/renderer/ContextManager.java`:
- Around line 34-39: Replace the volatile boolean field
`_warnedSingleContextMode` with an `AtomicBoolean` to enforce true one-time
emission of the warning. Change the check-then-set pattern that checks the
variable and sets it to true into an atomic compareAndSet operation (or similar
atomic method) that guarantees only one thread can successfully transition the
flag and log the warning, eliminating the race condition. Apply the same atomic
pattern fix to the related warning code referenced at lines 72-83.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 569e0c96-0328-4ee8-b623-2c8d933cdbfb

📥 Commits

Reviewing files that changed from the base of the PR and between 61989ee and 7f441ea.

📒 Files selected for processing (2)
  • ardor3d-core/src/main/java/com/ardor3d/renderer/ContextManager.java
  • ardor3d-core/src/test/java/com/ardor3d/renderer/TestContextManager.java

Comment thread ardor3d-core/src/main/java/com/ardor3d/renderer/ContextManager.java
@Renanse Renanse merged commit 57c9876 into master Jun 20, 2026
2 checks passed
@Renanse Renanse deleted the fix/warn-single-context-multi branch June 20, 2026 20:07
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.

1 participant