Preserve tangent handedness when extending a batch

[toaster0123]

Adding geometry to an existing BatchNode or SimpleBatchNode can break normal-mapped lighting on the geometry that was already batched.

When the old batch is copied into the larger mesh, only the first three tangent components are copied. The fourth component, which tells the lighting shader which way the bitangent points, is left at zero instead of +1 or -1.

This change copies every buffer component while preserving the existing vertex offset and buffer positions.

Regression tests cover both batch node types, positive and negative tangent signs, a rotated and translated source, unchanged position/normal/tangent data, and a third batch pass. The two tangent regressions fail before the fix and pass afterward. Two additional controls verify world positions with a translated ancestor and batch container, including container moves before later batch passes; those pass both before and after the fix.

Validation: core build (including Javadoc), plus desktop, effects and plugins checks passed: 561 tests, no failures, two existing skips. A final core/style rerun passed after test-style cleanup, with no Checkstyle warnings in the changed test. The unchanged test-data JAR from the same baseline was reused locally to avoid duplicating its resources. The production change and regression coverage were independently reviewed.

[jaime-jmebot]

Nice fix for the tangent w value — copying every component verbatim is the right idea. One thing I want to double-check before merging though: this loop no longer applies the pos translation that the previous version added, so the pos parameter is now dead in this method.

Can you confirm the call sites? If batchGeometry() still hands the batch container's translation to this method for the Position buffer, dropping it here would silently shift already-batched geometry whenever the container has been moved. If that offset really belongs to the caller now, it'd be great to remove the unused pos parameter and its call sites so the next reader doesn't have to re-derive this.

If it turns out the offset is still needed here, applying it only to the first three components (so the tangent's w is copied untouched) keeps both behaviors intact.

[toaster0123]

I checked the base and current call sites. doCopyBuffer() has the same four parameters in both versions: input buffer, vertex offset, output buffer and component count. The old pos was a local scratch Vector3f, not a translation argument; it only read xyz from the input and wrote them unchanged. Transforms are still applied by doTransforms(). BatchNode uses source world transforms and disables the generated geometry's render transform; SimpleBatchNode uses source local transforms and keeps the container's render transform.

In 5d6176e I added world-position controls for both node types with a translated ancestor and container, then container moves before subsequent batch passes. They pass on both the original and fixed code; the two tangent regressions still fail only on the original. All four pass with the fix. I also aligned the header with 2009-2026 and cleaned up the test-style warnings. No production adjustment was needed. The scoped build/checks passed 561 tests with no failures and two existing skips.

[jaime-jmebot]

Thanks — that settles it. I misread pos as a translation argument; it's the TempVars scratch vector, so nothing is lost by writing straight from the input buffer, and transforms still come from doTransforms() in both node types. No signature change needed.

One tiny non-blocking check: vars.release() went away with the old loop — please make sure the matching vars.acquire() isn't left behind so no TempVars slot stays held for the lifetime of the thread. Everything else looks good to me, and the 2009-2026 header plus the style cleanup cover the rest.

[toaster0123]

Confirmed: the same change removes both TempVars.get() and vars.release() from doCopyBuffer(). The remaining get/release pair belongs to doTransforms() and is unchanged. No TempVars slot is left acquired by the copy helper.

[jaime-jmebot]

Confirmed on my side too — doCopyBuffer is down to a pure component copy with no TempVars usage at all, so there's nothing left to release. That closes my last open item; no further changes needed from me. 👍

[jaime-jmebot]

Thanks for the focused fix — copying all buffer components instead of just the first three is clearly the correct approach for keeping tangent handedness, and the regression coverage for both batch node types (including the multi-pass case) is welcome.

• One open question before merge: doCopyBuffer no longer uses its pos argument. Please confirm the batch translation offset for positions is still applied by the caller, or drop the now-unused parameter and update the call sites.
• Small nit on the new test file: the header reads Copyright (c) 2026 jMonkeyEngine, while the rest of the repo uses the 2009-<year> form from source-file-header-template.txt. Easy to align.

Otherwise the change reads clean and I'm happy with the direction.

[jaime-jmebot]

• doCopyBuffer now copies every component verbatim, so the tangent's w survives a batch extension — root cause fixed in the right place, and the vertex offset/buffer positions are untouched.
• Confirmed pos was TempVars scratch rather than a translation, so no geometry shift is introduced; the world-position controls for moved ancestors/containers are a nice guard for that.
• Coverage looks solid: both batch node types, +1/-1 handedness, a rotated/translated source and a second batch pass.

Thanks for the quick turnaround — approving from me.

[jaime-jmebot]

Replied in-thread to close out my last nit — nothing further needed from me, and the approval stands.