Skip to content

Fix BitmapText bounding box alignment and baseline positioning#1355

Merged
obiot merged 4 commits intomasterfrom
fix/text-bounds
Apr 6, 2026
Merged

Fix BitmapText bounding box alignment and baseline positioning#1355
obiot merged 4 commits intomasterfrom
fix/text-bounds

Conversation

@obiot
Copy link
Copy Markdown
Member

@obiot obiot commented Apr 6, 2026

Summary

  • Width: use max(xadvance, xoffset + width) for last glyph so bounding box encompasses the full visual extent
  • Height: use actual glyph extents (maxBottom - minTop) instead of capHeight which was too short
  • Baseline: use real glyph metrics (glyphYOffset, glyphMaxBottom) for middle/bottom/alphabetic/ideographic shifts in both draw() and updateBounds(), so text is correctly centered and aligned on baseline reference points
  • Y offset: bounding box starts at the first visible pixel (glyphMinTop * scaleY) instead of the draw origin
  • Optimize: precompute glyphMinTop/glyphMaxBottom once in BitmapTextData.parse() instead of per-line character iteration
  • Optimize: cache measureText results — only recompute in setText/resize, not on every updateBounds call
  • Tests: 36 new tests covering all textAlign/textBaseline combinations, scaling, multiline, word wrap, edge cases, and BitmapTextData precomputed values

Test plan

  • All 2197 tests pass (npx vitest run)
  • Visual verification with text example (bounding boxes tightly enclose all text)
  • Verify BitmapText baseline labels align correctly with reference lines
  • Check platformer score counter still renders correctly

🤖 Generated with Claude Code

obiot and others added 2 commits April 6, 2026 09:39
- ExampleText: use new Application() + Stage pattern
- TextScreen: all text objects added as world children
- BaselineOverlay: custom renderable for baseline reference lines
- Remove deprecated video.init/device.onReady/game.world usage
- Deferred #1345 multiline baseline tests (needs TextMetrics refactor)

Note: bounding boxes for text with non-default textAlign/textBaseline
are a pre-existing issue (#1345) — not a regression from this change.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Width: use max(xadvance, xoffset + width) for last glyph visual extent
- Height: use actual glyph extents (maxBottom - minTop) instead of capHeight
- Baseline: use real glyph metrics for middle/bottom/alphabetic/ideographic
  shifts in both draw and updateBounds, so text is correctly centered/aligned
- Y offset: bounding box starts at first visible pixel (glyphMinTop)
- Optimize: precompute glyphMinTop/glyphMaxBottom in BitmapTextData.parse()
- Optimize: cache measureText results, only recompute in setText/resize
- Update text example: increase multiline font size, adjust layout
- Add comprehensive Text and BitmapText bounds test coverage (36 new tests)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 6, 2026 08:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves Text and BitmapText bounding box calculation and baseline/alignment handling so reported bounds better match the rendered visual extents (especially for last-glyph width and glyph-based vertical extents). It also adds a dedicated bitmap font fixture and substantially expands font/bounds test coverage.

Changes:

  • Update BitmapText/Text metric calculations (last glyph visual extent, glyph vertical extents) and add glyph extent precomputation in BitmapTextData.
  • Add Text.updateBounds() override to account for textAlign/textBaseline.
  • Add a new bitmap font test fixture plus extensive bounds/baseline/alignment tests, and refresh the text example to visually verify baselines.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/melonjs/tests/public/data/fnt/xolo12.png Adds bitmap font atlas used by new BitmapText tests
packages/melonjs/tests/public/data/fnt/xolo12.fnt Adds bitmap font metrics used by new BitmapText tests
packages/melonjs/tests/font.spec.js Adds many new Text/BitmapText bounds, baseline, wrap, and edge-case tests
packages/melonjs/src/renderable/text/textmetrics.js Updates BitmapText measurement logic (last-glyph width; glyph extents for height/baseline data)
packages/melonjs/src/renderable/text/text.js Adds Text.updateBounds() that incorporates textAlign/textBaseline
packages/melonjs/src/renderable/text/bitmaptextdata.ts Precomputes glyphMinTop/glyphMaxBottom while parsing bitmap font data
packages/melonjs/src/renderable/text/bitmaptext.js Uses cached metrics for bounds; changes baseline/bounds computation and draw baseline logic
packages/melonjs/CHANGELOG.md Documents the Text/BitmapText bounds/baseline fixes
packages/examples/src/examples/text/text.ts Refactors text example into a Stage and adds baseline reference overlay
packages/examples/src/examples/text/ExampleText.tsx Updates example bootstrapping to use Application + state
Comments suppressed due to low confidence (1)

packages/melonjs/src/renderable/text/bitmaptext.js:350

  • BitmapText.draw() applies the baseline adjustment inside the per-line loop and uses glyphMaxBottom (single-line extent). For multiline text this causes the baseline shift to be applied repeatedly (each line subtracts the shift again) and also cannot align the full block for "middle"/"bottom". The baseline shift should be applied once before iterating lines, based on the overall measured block height (metrics.height) and glyphYOffset, then only increment y by lineHeight per line.
			}

			// adjust y pos based on baseline using actual glyph extents
			const gy = this.metrics.glyphYOffset || 0;
			const gmb = this.metrics.glyphMaxBottom || stringHeight;
			switch (this.textBaseline) {
				case "middle":
					y -= (gy + gmb) * 0.5;
					break;

				case "ideographic":
				case "alphabetic":
				case "bottom":
					y -= gmb;
					break;

				default:
					break;
			}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

obiot and others added 2 commits April 6, 2026 16:28
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move baseline y-shift out of the per-line loop and apply it once
based on the full text block height. Previously, the per-line shift
caused multiline text with bottom/middle baselines to accumulate
offsets incorrectly.

- bottom/alphabetic/ideographic: shift by glyphYOffset + totalHeight
- middle: shift by glyphYOffset + totalHeight/2
- updateBounds uses the same formula for consistent bounds

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 6, 2026 08:32
@obiot obiot merged commit 6db3d9b into master Apr 6, 2026
8 checks passed
@obiot obiot deleted the fix/text-bounds branch April 6, 2026 08:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +325 to +365
updateBounds(absolute = true) {
if (typeof this.metrics !== "undefined" && this._text.length > 0) {
const bounds = this.getBounds();
bounds.clear();

const w = this.metrics.width;
const h = this.metrics.height;

// compute x offset based on textAlign
let ax = 0;
switch (this.textAlign) {
case "right":
ax = w;
break;
case "center":
ax = w / 2;
break;
}

// compute y offset based on textBaseline
let ay = 0;
switch (this.textBaseline) {
case "middle":
ay = h / 2;
break;
case "ideographic":
case "alphabetic":
case "bottom":
ay = h;
break;
}

bounds.addFrame(-ax, -ay, w - ax, h - ay);

if (absolute === true) {
const absPos = this.getAbsolutePosition();
bounds.centerOn(
absPos.x + bounds.x + bounds.width / 2,
absPos.y + bounds.y + bounds.height / 2,
);
}
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

Text.updateBounds() now positions the bounds using metrics.width/height with baseline offsets based on the total text height, but Text.draw() still renders at this.metrics.x/y, and TextMetrics.measureText() still computes metrics.y for middle/bottom baselines using only lineHeight() (not total height). For multi-line text with textBaseline = "middle"/"bottom"/"alphabetic"/"ideographic", this makes the visual render position inconsistent with the new bounds (and can desync hit-testing/culling). Consider aligning the rendering path with the new baseline semantics (e.g., compute draw position from current pos + align/baseline offsets, or update metrics.x/y consistently when bounds are updated / when measuring).

Copilot uses AI. Check for mistakes.
Comment on lines +167 to +178
for (const bl of baselines) {
const b = new BitmapText(xPos, 375, {
font: "arialfancy",
size: 1.275,
textBaseline: bl,
textAlign: "left",
text: bl,
});
app.world.addChild(b, 3);
tmpBFont.textBaseline = bl;
tmpBFont.setText(`${bl}@`);
xPos += tmpBFont.measureText().width;
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

TextScreen.onResetEvent() creates temporary Text/BitmapText instances (tmpFont, tmpBFont) purely for measurement but never destroys them. Text allocates a CanvasRenderTarget and pooled Color objects, and BitmapText allocates pooled vectors/fontData; leaving these undisposed can leak pooled resources for the lifetime of the example. Consider calling destroy() on these temp instances (or reusing a shared instance stored on the Stage and destroyed on reset/deactivate).

Suggested change
for (const bl of baselines) {
const b = new BitmapText(xPos, 375, {
font: "arialfancy",
size: 1.275,
textBaseline: bl,
textAlign: "left",
text: bl,
});
app.world.addChild(b, 3);
tmpBFont.textBaseline = bl;
tmpBFont.setText(`${bl}@`);
xPos += tmpBFont.measureText().width;
try {
for (const bl of baselines) {
const b = new BitmapText(xPos, 375, {
font: "arialfancy",
size: 1.275,
textBaseline: bl,
textAlign: "left",
text: bl,
});
app.world.addChild(b, 3);
tmpBFont.textBaseline = bl;
tmpBFont.setText(`${bl}@`);
xPos += tmpBFont.measureText().width;
}
} finally {
tmpBFont.destroy();

Copilot uses AI. Check for mistakes.
Comment on lines +605 to +624
describe("Font : BitmapText bounds", () => {
beforeAll(async () => {
await new Promise((resolve) => {
loader.preload(
[
{
name: "xolo12",
type: "image",
src: "/data/fnt/xolo12.png",
},
{
name: "xolo12",
type: "binary",
src: "/data/fnt/xolo12.fnt",
},
],
resolve,
);
});
});
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

The Font : BitmapText bounds suite’s beforeAll assumes the engine is already booted and a renderer initialized (done in the earlier Font : Text suite). This makes the test file order-dependent and can fail when running only the BitmapText tests via Vitest filters/patterns. Consider making this suite self-contained by calling boot()/video.init(...) here as well, or extracting a shared top-level setup used by both suites.

Copilot uses AI. Check for mistakes.
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.

2 participants