Repository navigation
Conversation
Add a renderer overload for base-level layer rectangles with matching uncompressed formats and color spaces. Validate buffers, bounds, mipmap policy and packed expansion sizes before binding; retain source state and reactivate cached unit-zero bindings. Updates affect GPU storage only, so callers retain and replay their data after full uploads or context recreation. Keep the default renderer method compatible with external implementations and avoid requesting generated mipmaps for non-mipmapped array construction. Validate with recording-GL tests for resident, dirty, recreated and failed uploads, source aliases and malformed inputs. Include an asset-free finite native readback probe; native execution remains a separate manual check.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| try { | ||
| if (requiresPackedExpansion(src.getFormat(), format)) { | ||
| expanded = expandPackedPixels(data, src.getFormat(), | ||
| Math.multiplyExact(src.getWidth(), src.getHeight())); |
There was a problem hiding this comment.
This direct upload assumes the current GL_UNPACK_ALIGNMENT happens to match the patch row stride. OpenGL defaults to alignment 4, so tightly packed RGB8 patches with widths whose 3-byte row size is not divisible by 4 will be read with padding between rows (and can overrun the supplied patch data); the result can also depend on whatever unpack state a previous upload left behind. Please establish the correct unpack alignment for this upload (and restore or otherwise consistently manage the prior GL state), as the existing source contract is tightly packed.
There was a problem hiding this comment.
We are settings gl_unpack_alignment 1 in the renderer initialize:
https://github.com/8Keep/jmonkeyengine/blob/308dced919d190d3bc89c4daa1311b7c86b8f059/jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java#L951-L957
We should probably change this for perf? But I think for now this should be fine
There was a problem hiding this comment.
Thanks for pointing that out—I missed the renderer’s initialization setting. Since GL_UNPACK_ALIGNMENT is set to 1 during initialization, the tightly packed patch rows are handled correctly under the renderer’s normal GL-state assumptions, so my concern about the default alignment doesn’t apply here. I’ll withdraw that finding; thanks for clarifying!
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Hi, thanks for putting together this focused API and the thorough coverage around validation, source-buffer preservation, and replay after recreation. The GPU-only semantics are clearly documented, and validating before setTexture is especially helpful because that call can trigger a full upload.
I did find one correctness issue in the upload path: glTexSubImage3D consumes rows according to the current GL_UNPACK_ALIGNMENT, which defaults to 4 and may also be left changed by other renderer operations. A tightly packed RGB8 patch with a row stride not divisible by that alignment is therefore interpreted incorrectly, and the last row may be read beyond the provided data. I left an inline comment on the upload call; please make the unpack alignment consistent with the tightly packed source contract (while respecting the renderer's GL-state management).
Once that is addressed, the implementation and the test coverage look like a strong fit for the intended base-level texture-array use case. Thanks for tackling this!
A resident array can discard its CPU pixels while retaining GPU data and layer metadata. Only a full upload consumes destination pixels, so keep that validation on allocation/reupload rather than rejecting a patch. Strengthen the existing resident-data regression and extend native readback coverage to check patched and untouched texels after all CPU layers are discarded. Clarify the metadata and replay requirements. Validation: the resident regression fails before the fix. All 41 focused texture tests and style tasks pass. The finite native probe passes 45 comparisons each on desktop OpenGL and ANGLE.
|
Fixed, thanks |
Updating a small region of a texture array currently requires marking its Image dirty and reuploading all layers. Add a Renderer.modifyTexture overload that uploads a complete 2D source image into a rectangle of one layer:
For a resident clean array, this issues one layer-region upload and leaves the other texels alone. Validation happens before binding or allocation. Source buffer position, limit, mark, byte order and contents are preserved, including packed sRGB conversion.
The scope is deliberately base-level, uncompressed color data with matching format/color space and no mipmaps or multisampling. Changes affect GPU storage only; callers retain/replay their updates after a full upload or context recreation. The default interface method keeps external Renderer implementations compatible.