Ensure glyphs are not overwritten by fallbacks. #471
Merged
Conversation
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
There was a problem hiding this comment.
Pull request overview
This pull request fixes issue #469 where glyphs were being incorrectly overwritten by fallback glyphs during font rendering. The fix addresses multiple rendering issues discovered during debugging and updates the codebase to support ImageSharp.Drawing v3.0.0-alpha.0.6.
Key changes:
- Fixed incorrect indexing in
HorizontalMetricsTableto use the last record instead of the first when glyph index exceeds available records - Added bounds checking in
GPosTableto prevent out-of-bounds access during glyph positioning - Enhanced glyph positioning logic to properly handle vertical glyphs and prevent fallback glyphs from overwriting valid glyphs
Reviewed changes
Copilot reviewed 11 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SixLabors.Fonts/Tables/General/HorizontalMetricsTable.cs | Fixed to return last record instead of first for out-of-bounds glyph indices, per OpenType spec |
| src/SixLabors.Fonts/Tables/AdvancedTypographic/GPosTable.cs | Added bounds check before accessing collection to prevent potential exceptions |
| src/SixLabors.Fonts/GlyphPositioningCollection.cs | Enhanced vertical glyph detection, fixed fallback glyph handling, modernized collection initialization |
| tests/SixLabors.Fonts.Tests/Issues/Issues_469.cs | New comprehensive test covering multiple writing systems with fallback fonts |
| tests/SixLabors.Fonts.Tests/TestFonts.cs | Added helper properties for additional test fonts and made GetFullPath public |
| tests/SixLabors.Fonts.Tests/TextLayoutTestUtilities.cs | Updated to use new ImageSharp.Drawing v3 API (GenerateGlyphs) |
| tests/SixLabors.Fonts.Tests/SixLabors.Fonts.Tests.csproj | Enabled SUPPORTS_DRAWING tests and updated to ImageSharp.Drawing v3.0.0-alpha.0.6 |
| samples/DrawWithImageSharp/Program.cs | Updated API call from GenerateGlyphs to GeneratePaths for ImageSharp.Drawing v3 |
| samples/DrawWithImageSharp/DrawWithImageSharp.csproj | Updated to ImageSharp.Drawing v3.0.0-alpha.0.6, removed obsolete System.ValueTuple reference |
| SixLabors.Fonts.sln | Added DrawWithImageSharp sample project to solution |
| tests/SixLabors.Fonts.Tests/Fonts/Sarabun-Regular.ttf | Binary font file added for test coverage |
| tests/Images/ReferenceOutput/Test_Issue_469-.png | Reference image for visual regression test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Prerequisites
Description
Fixes #469
I also fixed additional rendering issues spotted during debugging.