Fix octave-dot/ornament-text collision in the stacked annotation band - #38
Merged
Merged
Conversation
NoteTopAnnotationLayout.AnnotationLayerClearance (6f) is the gap between two stacked annotation layers' anchor Y values, not their actual rendered glyph heights -- too small a gap let a tall glyph's ink overlap the layer below it. Discovered while building a synthetic demo score to validate this session's other changes: rendering Trill/Mordent/Turn (all untouched by this session, pre-existing code) with a high-octave-dot note shows the exact same collision, so this predates every recent PR -- it just had never been rendered and looked at before. Verified this for real, not just in theory: libgdiplus (Mono's System.Drawing implementation) is installed in this sandbox, so the actual JianpuRenderer code, compiled via mcs against the real built JianpuEditor.exe, can run under mono and produce a real PNG via RenderToBitmap -- confirmed the collision, tried a fix, and confirmed the fix visually before committing to it, rather than reasoning about layout math blind the way every other rendering change this session had to. Fixed by setting the clearance to the actual Font.Height of the tallest stacked ornament font (Microsoft YaHei / Arial Italic, both 11pt) instead of a hand-picked small number, so it's a real font metric rather than a magic constant that happens to look fine in one screenshot. Locked in with a new regression test that measures those fonts' real height and asserts the clearance covers it, rather than asserting a hardcoded pixel value -- a future reduction back toward 6f would fail this test even without a visual re-check. ROADMAP.md documents this new visual-verification capability and updates the accidental-slash-convention phase's "can't visually verify at all" caveat accordingly (lowers but doesn't eliminate that phase's risk). Verified via the same sandbox pipeline as every prior change this session, plus the new rendering-based check described above: dotnet build succeeds against the real project/package graph, dotnet format --verify-no-changes passes on both projects. dotnet test itself cannot execute in this sandbox; real confirmation of the new test comes from GitHub Actions CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
The new AnnotationLayerClearance_CoversTheTallestStackedOrnamentFont regression test failed on GitHub Actions: real Windows GDI+ reports Font.Height=20 for Microsoft YaHei/Arial Italic at 11pt, not 18 as this sandbox's libgdiplus (Mono's System.Drawing implementation) reported -- expected, since Microsoft YaHei isn't installed on Linux and libgdiplus silently substitutes a fallback font with slightly different metrics. Flagged this exact risk in the PR description before pushing; it materialized as intended (a real test failure against real platform metrics), not a surprise. Bumped from 18f to 22f -- a couple of pixels of headroom above the real measured value (20) rather than sitting exactly on the boundary, so a slightly different runner image can't flake this. Re-rendered the full demo score via Mono+libgdiplus to confirm no new collisions from the extra spacing. Verified via the same sandbox pipeline as every prior change this session: dotnet build succeeds against the real project/package graph, dotnet format --verify-no-changes passes on both projects. dotnet test itself cannot execute in this sandbox; real confirmation comes from GitHub Actions CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found while building a synthetic demo score to validate this session's other changes, and shipped
with the user's go-ahead once I had real visual proof.
NoteTopAnnotationLayout.AnnotationLayerClearance(6f) is the gap between twostacked annotation layers' anchor Y values, not their actual rendered glyph heights. Too small
a gap let a tall glyph's ink overlap the octave dot below it. Confirmed pre-existing, not
caused by any change this session: rendering Trill/Mordent/Turn (all untouched by this
session's work) with a high-octave-dot note shows the exact same collision -- it just had never
been rendered and looked at before.
libgdiplus(Mono'sSystem.Drawingimplementation) isinstalled in this sandbox, so the real
JianpuRenderercode, compiled viamcsagainst theactual built
JianpuEditor.exe, can run undermonoand produce a real PNG viaRenderToBitmap. Used this to confirm the collision, try a fix, and visually confirm the fixbefore committing to it -- rather than reasoning about layout math blind, the way every other
rendering change this session had to.
ROADMAP.mddocuments this and updates theaccidental-slash-convention phase's "can't visually verify at all" caveat accordingly.
Font.Heightof the tallest stacked ornament font(Microsoft YaHei / Arial Italic, both 11pt) instead of a hand-picked small number, so it's a real
font metric rather than a magic constant that happens to look fine in one screenshot.
Test plan
AnnotationLayerClearance_CoversTheTallestStackedOrnamentFont) measuresthose fonts' real height and asserts the clearance covers it, rather than asserting a hardcoded
pixel value -- a future reduction back toward 6f would fail this test even without a visual
re-check.
AnnotationLayerClearanceread the constant dynamically, nota hardcoded literal, so they stay correct regardless of its value.
dotnet buildsucceeds against the real project/package graph,
dotnet format --verify-no-changespasses onboth projects.
Mono+libgdiplus and confirmed the collision is gone with no new collisions introduced elsewhere
(checked the full demo score covering every ornament type, not just the isolated repro case).
Not verified: real Windows GDI+ font metrics for these two fonts may differ slightly from
libgdiplus's approximation (Microsoft YaHei likely isn't installed in this sandbox, so libgdiplus
silently substitutes a fallback font) -- a real screenshot on Windows would still be the
authoritative check, though this is much stronger evidence than any prior rendering change this
session had.
dotnet testitself cannot execute here (no WindowsDesktop runtime pack); realconfirmation of the new test comes from GitHub Actions CI.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
Generated by Claude Code