Repository navigation
Fix cubemap texture swizzle parameter targets - #2985
Conversation
| // Swizzle state belongs to the cubemap, not to an individual image face. | ||
| if (target >= GL.GL_TEXTURE_CUBE_MAP_POSITIVE_X |
There was a problem hiding this comment.
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.
| // 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) { |
| default: | ||
| if (method.getReturnType() == void.class) { | ||
| return null; | ||
| } | ||
| throw new AssertionError("Unexpected GL call: " + method); |
There was a problem hiding this comment.
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.
| 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
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis and the fix both look right to me.
- Fix is correct and well scoped. Normalizing the target inside
setupTextureSwizzleis the right seam:uploadTexturekeeps passing each face target toglTexImage2D(so image definitions, mip levels and data slicing are untouched), while the fourglTexParameteriswizzle calls now getGL_TEXTURE_CUBE_MAP. Every branch of that switch reads the localtarget, so all swizzle formats are covered, not just the first one. - Good coverage on the tests. Driving both a
GL3and aGLES_30proxy throughupdateTexImageDatais 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 theglTexImage2Dargument indices and the face-major/level-minor ordering assumptions hold againstuploadTextureLevel. - 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.
|
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. |
|
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.
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. |
|
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. |
Summary
Cubemap uploads pass an individual face target into
TextureUtil.uploadTexture. Formats requiring channel swizzles then reuse that face target forglTexParameteri, which requiresGL_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:
jme3-core:test: 497 cases, zero failures, one skippedjme3-effects:test: 6 cases, zero failuresFor the test-only removal, verified the production diff is unchanged and
git diff --checkpasses; the suites were not rerun. Hardware GPU validation remains outstanding.