Skip to content

fix(text): raise a clear error instead of IndexError on glyph/char mismatch#4868

Open
SupernovaIa wants to merge 1 commit into
ManimCommunity:mainfrom
SupernovaIa:fix/text-gen-chars-ligature-mismatch
Open

fix(text): raise a clear error instead of IndexError on glyph/char mismatch#4868
SupernovaIa wants to merge 1 commit into
ManimCommunity:mainfrom
SupernovaIa:fix/text-gen-chars-ligature-mismatch

Conversation

@SupernovaIa

Copy link
Copy Markdown

Summary

Text._gen_chars() assumes disable_ligatures=True guarantees exactly one rendered glyph per non-space character. That assumption breaks for fonts that implement some of their ligatures through OpenType features other than liga/dlig/clig/hlig (the ones ManimPango's disable_liga disables) — for example, Fira Code implements its programming ligatures (<=, ->, etc.) exclusively through calt (contextual alternates), which isn't covered. I confirmed this by inspecting Fira Code's own GSUB table with fontTools: it defines calt but none of liga/dlig/clig/hlig.

When that happens, Pango still merges e.g. < and = into a single glyph, and _gen_chars() walks off the end of self.submobjects, raising an opaque IndexError: list index out of range with no indication of what went wrong. This is reproducible today via, for instance:

Code(
    code_string="print(1<=2)",
    language="python",
    paragraph_config={"font": "Fira Code"},
)

This PR does not fix the underlying issue — the actual root cause is in a separate repository, ManimCommunity/ManimPango (its disable_liga feature string would need calt=0 added). That's outside the scope of this PR (would require setting up the Cython/C build for a separate package).

What this PR does do: turn the opaque crash into a clear, actionable error, so that anyone hitting this (there have been a few confused reports in #3237 over time) immediately understands why, instead of debugging a bare IndexError. This is a pure improvement to the failure path — no behavior changes for the cases that already work.

Refs #3237

Test plan

  • Added test_gen_chars_raises_clear_error_on_glyph_mismatch, which simulates the glyph/char mismatch directly (no dependency on any specific font being installed) and asserts the new ValueError is raised
  • uv run pytest tests/module/mobject/text/test_text_mobject.py tests/test_code_mobject.py — all pass
  • Manually reproduced the original crash with Fira Code installed locally, confirmed the new error message is accurate and points at the real cause
  • uv run pre-commit run --files manim/mobject/text/text_mobject.py tests/module/mobject/text/test_text_mobject.py — ruff, mypy, codespell all pass

…smatch

Text._gen_chars() assumed disable_ligatures=True guarantees one glyph per
non-space character, but that only holds if the font's ligatures are all
implemented via the 'liga'/'dlig'/'clig'/'hlig' OpenType features that
ManimPango disables. Fonts like Fira Code implement programming ligatures
(e.g. '<=', '->') via 'calt' instead, which isn't covered, so Pango still
merges characters into a single glyph and _gen_chars() walks off the end
of self.submobjects with an opaque IndexError (e.g. via Code(...,
paragraph_config={"font": "Fira Code"}), see issue ManimCommunity#3237).

This adds an upfront check that raises a clear ValueError explaining the
likely cause and a workaround, instead of the bare IndexError. Note this
does not fix the underlying issue -- the actual root cause (ManimPango
not disabling 'calt') lives in a separate repo (ManimPango) and would
need to be addressed there for Code/Text to render correctly with fonts
like Fira Code.

Refs ManimCommunity#3237
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.

1 participant