Skip to content

Fix cubemap texture swizzle parameter targets - #2985

Merged
riccardobl merged 3 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/cubemap-swizzle-target
Oct 2, 2026
Merged

riccardobl merged 3 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/cubemap-swizzle-target

Conversation

@toaster0123

@toaster0123 toaster0123 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Cubemap uploads pass an individual face target into TextureUtil.uploadTexture. Formats requiring channel swizzles then reuse that face target for glTexParameteri, which requires GL_TEXTURE_CUBE_MAP. The rejected swizzle calls leave the intended channel remapping unapplied.

Normalize the target only inside swizzle setup. Image definitions still use each face target, and mip levels and data slices are unchanged.

This affects desktop core-profile alpha/luminance cubemaps and GLES3 cubemaps using reordered byte formats. The target requirement is specified by OpenGL 3.3 §3.8.8 and OpenGL ES 3.0 §3.8.7.

Validation

The final diff contains only the six-line production fix in TextureUtil; the recording-GL test has been removed as requested. Production source is byte-for-byte unchanged from 3c125b5.

Previously completed validation of this production code:

  • Fourteen recording-GL cases passed; all seven cubemap cases reproduced the failure on the unmodified baseline
  • Full jme3-core:test: 497 cases, zero failures, one skipped
  • Full jme3-effects:test: 6 cases, zero failures
  • Renderer Checkstyle tasks passed
  • Mesa llvmpipe GLES pixel validation: eight cases and 28 exact RGBA readbacks

For the test-only removal, verified the production diff is unchanged and git diff --check passes; the suites were not rerun. Hardware GPU validation remains outstanding.

@riccardobl
riccardobl marked this pull request as ready for review October 2, 2026 10:28
Comment on lines +108 to +109
// Swizzle state belongs to the cubemap, not to an individual image face.
if (target >= GL.GL_TEXTURE_CUBE_MAP_POSITIVE_X

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, tight fix — normalizing the target only in setupTextureSwizzle means uploadTexture still hands each face target to glTexImage2D, which is exactly what cube maps need.

One small robustness note: this range check silently relies on the six face enums being numerically contiguous (0x8515–0x851A). It is today, but if that assumption ever breaks the swizzle would just be skipped instead of failing loudly. A one-line comment makes that explicit for the next reader.

Suggested change
// Swizzle state belongs to the cubemap, not to an individual image face.
if (target >= GL.GL_TEXTURE_CUBE_MAP_POSITIVE_X
// glTexParameteri needs the cube map target, not an individual face target.
// The six face enums are contiguous (0x8515..0x851A).
if (target >= GL.GL_TEXTURE_CUBE_MAP_POSITIVE_X
&& target <= GL.GL_TEXTURE_CUBE_MAP_NEGATIVE_Z) {

Comment on lines +198 to +202
default:
if (method.getReturnType() == void.class) {
return null;
}
throw new AssertionError("Unexpected GL call: " + method);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice recording-GL harness, and the face-vs-texture-target split is exactly the distinction that was broken.

Small thing: GLRenderer.initialize() is a big, growing method, so any new non-void GL query added there will land in this default branch and fail with a message that reads like a swizzle bug. Wording it as "the stub needs extending" saves the next person some head-scratching.

Suggested change
default:
if (method.getReturnType() == void.class) {
return null;
}
throw new AssertionError("Unexpected GL call: " + method);
default:
if (method.getReturnType() == void.class) {
return null;
}
throw new AssertionError("Unstubbed GL call, extend RecordingGl: " + method);

@jaime-jmebot jaime-jmebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this — the diagnosis and the fix both look right to me.

  • Fix is correct and well scoped. Normalizing the target inside setupTextureSwizzle is the right seam: uploadTexture keeps passing each face target to glTexImage2D (so image definitions, mip levels and data slicing are untouched), while the four glTexParameteri swizzle calls now get GL_TEXTURE_CUBE_MAP. Every branch of that switch reads the local target, so all swizzle formats are covered, not just the first one.
  • Good coverage on the tests. Driving both a GL3 and a GLES_30 proxy through updateTexImageData is a neat way to catch this without a GPU, and the paired 2D control cases guard against the obvious regression of over-correcting every target. Verified the glTexImage2D argument indices and the face-major/level-minor ordering assumptions hold against uploadTextureLevel.
  • Two minor notes left inline — a comment on the face-enum range check, and a clearer message for unstubbed GL calls in the recording proxy. Neither blocks the PR.

One thing worth calling out since you flagged it yourself: this still needs a pass on real hardware. The stub proves the right target is passed, not that a driver accepts it and the remap looks correct.

Copy link
Copy Markdown
Contributor Author

Addressed both inline notes in 3c125b5: documented the contiguous OpenGL face-enum range and clarified that an unstubbed call requires extending RecordingGl. All 14 focused cases and both renderer Checkstyle tasks pass. This follow-up changes only a comment and a test failure message. Real-GPU validation is still outstanding.

Copy link
Copy Markdown
Contributor Author

Additional offscreen pixel validation of 3c125b5 against baseline e8cf975:

The actual pinned Java GLRenderer/TextureUtil classes ran through a test-only JNA bridge to surfaceless EGL and Mesa 25.0.7 llvmpipe (LLVM 19.1.7, OpenGL ES 3.2). A fixture shader sampled the uploaded textures into a linear RGBA8 framebuffer for readback.

  • BGR8, ARGB8, BGRA8 and ABGR8 were tested as six-face cubes and matching 2D controls
  • Baseline cube cases reproduce GL_INVALID_ENUM and incorrect channel values; the 2D controls pass
  • The fixed head passes all eight cases without GL errors: all 28 RGBA pixel readbacks match exactly, including every cube face
  • The portable reproduction repeated the same before/after outcomes

This validates GLES upload and sampled pixels on a software driver. Production LWJGL/Android integration, hardware GPUs, desktop-core luminance/alpha formats, and a forced ES3.0-only context remain outside this smoke test. No performance or CI result is claimed.

Copy link
Copy Markdown
Contributor Author

Removed TextureUtilCubemapSwizzleTest in 891e14a as requested. The PR now changes only TextureUtil.java: the six-line cubemap target fix and its comments are unchanged. The description records the earlier validation; this removal was checked with an unchanged-production diff and git diff --check, without rerunning the suites.

@riccardobl
riccardobl merged commit 951c134 into jMonkeyEngine:master Oct 2, 2026
10 checks passed
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.

3 participants