BitmapFont: complete array-based assembly and color-tag traversal cleanup - #2982
toaster0123 wants to merge 2 commits into
Conversation
…anup Supersede PR jMonkeyEngine#2963 and address issue jMonkeyEngine#2221 and maintainer review 5255645948. Reconstruct the missing changes and regression tests from current master, retaining the valid BitmapTextPage work from the original PR. Co-authored-by: Jaime Bot <jaime@rblb.it>
| // which makes this loop quadratic in the number of quads. | ||
| int i = 0; | ||
| for (LetterQuad fq : pageQuads) { | ||
| fq.storeToArrays(pos, tc, idx, color, i); |
There was a problem hiding this comment.
Nice — the for-each loop with a manual quad index is exactly the right fix here, and the comment explains why.
One loose end: the five LetterQuad helpers this branch used to call — appendPositions(FloatBuffer), appendPositions(ShortBuffer), appendTexCoords(FloatBuffer), appendColors(ByteBuffer) and appendIndices(ShortBuffer, int) — are now unreferenced. LetterQuad is package-private, so nothing outside com.jme3.font can reach them, and this was the only call site. Since the goal of the change is to delete the unreachable path, I'd drop these in the same PR. It's not just tidiness: appendColors writes the four channels with putInt, so it comes out in the wrong order — leaving it around just invites someone to "restore" a broken path later.
There was a problem hiding this comment.
Removed all five unused LetterQuad append helpers and their ByteBuffer, FloatBuffer and ShortBuffer imports. A repository-wide Java search confirmed that no call sites remained before removal, so the obsolete putInt-based color path is gone too.
Validation passed: full core tests (492 passed, 1 skipped, zero failures), the 10 bitmap-text regression tests, and strict core Javadoc. The optional collection/API deprecation changes are left for a separate follow-up to keep this commit focused on the dead-code cleanup.
There was a problem hiding this comment.
Confirmed on my side — LetterQuad is down to just storeToArrays, and the ByteBuffer/FloatBuffer/ShortBuffer imports went with it. That closes out the only thing I was holding the PR for, so I'm approving.
Agree on keeping the ArrayList switch and the @Deprecated on the three-arg constructor for a follow-up; the BitmapText(font, rightToLeft, arrayBased) Javadoc now says arrayBased is ignored, so the intent is already clear to anyone reading it.
One note for the flip out of draft: there are still no check runs registered on 837095e — status is pending with an empty check-run list, so CI hasn't picked up this commit yet rather than having failed. Worth giving it a moment and confirming it goes green before merging.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Really solid groundwork here — the array-only path, the shared applyColorTags() traversal, and especially the regression suite. I hand-traced the wrapping/alignment, multi-page index and colour-tag expectations against the layout code and they all check out, and going through storeToArrays again confirms the vertex/UV/index/colour ordering is unchanged from master.
A few notes:
- The one thing I'd like changed is in my inline comment: delete the now-dead
LetterQuad.append*helpers so this PR fully owns the removal of the unreachable branch. Small and mechanical. - Two optional extras, your call:
ColorTags.colorscould be anArrayListnow that nothing depends on linked semantics (the #2221 concern about retaining large backing arrays doesn't really apply to a handful of tag ranges), and the publicBitmapText(font, rightToLeft, arrayBased)constructor could carry@Deprecatedto match its two siblings now that the flag does nothing — that's item 1 of #2221. - CI hasn't reported on this exact commit yet (the status endpoint is currently 404), so it's worth confirming it comes back green before you flip this out of draft.
- Heads-up for a future pass, not for this PR:
applyColorTags()andsetBaseAlpha()still rescan every letter per range, so the end-to-end cost is stillranges × letters. You're upfront about that in the description; just flagging it's the remaining follow-up rather than something this change regressed.
Remove the five obsolete append helpers and their unused nio imports after the unreachable non-array BitmapTextPage assembly path was deleted. Repository-wide Java search confirms no remaining call sites. Addresses the review by @jaime-jmebot: jMonkeyEngine#2982 (comment) The original Jaime Bot contribution remains credited in the parent commit.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
The dead-code removal landed — LetterQuad is now just storeToArrays, and the stale buffer imports are gone with it. That was the one blocking item, so this is approved.
- Verified
LetterQuad.javaandBitmapTextPage.java: no leftoverappend*helpers, no unused imports, and the array-only assembly path inassemble()is unchanged from what I traced before. - The
ArrayListand@Deprecatedideas are fine as a follow-up; thearrayBasedparameter is already documented as ignored. - Still no check runs registered on
837095e— worth confirming CI comes back green before flipping out of draft.
2 participants
Summary
Supersedes #2963 and addresses #2221 and the maintainer review. This replacement starts from current master because #2963's published head contains placeholder-only BitmapText.java and ColorTags.java, and lacks the promised Letters changes and regression test. The original PR is left unchanged.
The valid BitmapTextPage implementation is retained from Jaime Bot's original work, with original source line endings preserved. Its boolean overloads delegate to the new constructor and remain deprecated. BitmapText uses the new constructor while preserving its public three-argument signature and documenting arrayBased as ignored.
Regression coverage
Ten headless JUnit Jupiter tests use synthetic font/material/texture data, without GPU or asset-manager dependencies. Coverage includes 100 distinguishable quads and exact vertex/UV/index order, RGB/RGBA tags with an untagged prefix and adjacent/trailing tags, base-alpha override/reset, empty and reused buffers, alternating multiple-page fonts and page-local indices, legacy constructor parity in both text directions, and multiline/wrapped horizontal/vertical alignment.
Validation
:jme3-core:test: 493 tests, zero failures/errors, 1 skipped (492 successful).:jme3-core:checkstyleMain :jme3-core:checkstyleTest: completed successfully with existing renderer warnings. These repository tasks currently include renderer packages, not font packages.:jme3-core:javadoc -PenableJavadocError=true: passed.The full Gradle checks used JDK 25 and an external, uncommitted test-runner init script to pass the environment's existing proxy/truststore settings into tests and load Mockito as a startup agent. This avoids the sandbox's self-attach limitation; no tests were excluded and no repository build configuration changed. The initial unconfigured attempt had 14 environment-related failures, all resolved by that runner setup. Font-package doclint additionally included private/package-level members.
Remote CI has not yet run for this draft's exact commit and must be checked after publication.
Based on work by Jaime Bot jaime@rblb.it in #2963.