Measure text (v2) #48
Conversation
Refactor renderer to support a cleaner architecture for measuring text and rendering it. Add tests covering rendering and measuring
|
|
||
| internal const int FontToPixelDivisor = EmSquareSize * PointsPerInch; | ||
| } | ||
| } |
There was a problem hiding this comment.
The risk with a Constants class is all kinds of weird things gets dumped here. Instead, lets have constants private in classes where needed. Perhaps even local to the function! The PointsPerInch does not feel like a constant either in the same way the EmSquare size is. I liked the EmSquare class with a Size constant. :-) But it could also go inside the Typeface class I guess?
| @@ -4,6 +4,8 @@ namespace NRasterizer | |||
| { | |||
| public interface IGlyphRasterizer | |||
| { | |||
| int Resolution { get; } | |||
|
|
|||
There was a problem hiding this comment.
This also feels slightly malplaced. It would be awesome if the IGlyphRasterizer was unit-agnostic. But then how would one pass the target resolution to the scaling code... Hmm, it makes a lot of sense. Let's do it like this. 👍
| /// </value> | ||
| public float LineHeight { get; set; } = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm wondering what the need for a newline at the end of a file is for?
There was a problem hiding this comment.
Nothing important, it's just annoying when I cat files, etc.
There was a problem hiding this comment.
no thats fine... don't mind doing it was just wondering... I'll try to make sure I add them in the future
| /// <value> | ||
| /// The height of the line. | ||
| /// </value> | ||
| public float LineHeight { get; set; } = 1; |
There was a problem hiding this comment.
There is no support for this right now. Perhaps remove / comment out for now to be less confusing. Pretty annoying if you set this and nothing happens...
There was a problem hiding this comment.
its needed for calculating the bounding box of the measured text... even 1 line as a height.
|
|
||
| if (drawbox) | ||
| { | ||
| g.DrawRectangle(Pens.HotPink, new Rectangle(x, y, size.Width, size.Height)); |
| @@ -120,11 +134,17 @@ public static void Main(string[] args) | |||
| var rasterizerName = args[0]; | |||
| var fontPath = new FileInfo(args[1]); | |||
| var target = new FileInfo(args[2]); | |||
| var text = args[3]; | |||
| var text = args[3].Replace("\\n", "\n").Replace("\\t", "\t"); | |||
There was a problem hiding this comment.
👍 This could even go instatic function in Program - ExpandWhitespace or UnescapeWhitespace to make it even clearer what it does.
| var index = map.CharacterToGlyphIndex(charAsInt); | ||
|
|
||
| Assert.Equal(0, index); | ||
| } |
There was a problem hiding this comment.
Nice test! Drop some newlines to make it even more readable 👍 Like so
var idDeltas = new ushort[] { (ushort) (65536 - charAsInt) };
| var index = map.CharacterToGlyphIndex(charAsInt); | ||
|
|
||
| Assert.Equal(54, index); | ||
| } |
There was a problem hiding this comment.
Nice test! Again, reducing newlines would make it even shorter and sweeter. 👍
| } | ||
|
|
||
| } | ||
| } |
| public void BeginRead(int countourCount) | ||
| { | ||
| this.BeginReadCallCount++; | ||
| this.ContourCount = countourCount; |
There was a problem hiding this comment.
no problem i'll drop them (going to be hard jumping back and fourth between ImageSharp and this project, both have very different coding styles so I apologies now for when I constantly use the wrong style 😄 )
There was a problem hiding this comment.
It's fine to merge also, it's probably faster for me to go through smaller things like this and trailing newlines etc in one go afterwards.
|
|
||
| public void CloseFigure() | ||
| { | ||
| this.CloseFigureCallCount++; |
|
|
||
| public void Curve3(double p2x, double p2y, double x, double y) | ||
| { | ||
| Points.Add(new Point<double>(x, y)); |
| public int ContourCount { get; private set; } | ||
| public int EndReadCallCount { get; private set; } | ||
| public int FlushCallCount { get; private set; } | ||
| public List<Point<double>> Points { get; set; } = new List<Point<double>>(); |
There was a problem hiding this comment.
This should not have a public setter, right?
|
|
||
| public void EndRead() | ||
| { | ||
| this.EndReadCallCount++; |
|
|
||
| public void Flush() | ||
| { | ||
| this.FlushCallCount++; |
| Points.Add(new Point<double>(x, y)); | ||
| } | ||
| } | ||
| } |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not a unit-test, but actually reads from disk etc. Those kinds of test are better covered by the "generate-sample" tests that leverage the NRasterizer.CLI program. But let's leave those in for now.
|
|
||
| Assert.Equal(expectedWidth, size.Width); | ||
| Assert.Equal(expectedHeight, size.Height); | ||
| } |
There was a problem hiding this comment.
Wow, I literally love this test <3
| Assert.Equal(fontsize * text.Length, size.Width); | ||
| Assert.Equal(fontsize, size.Height); | ||
| } | ||
| } |
|
|
||
| public ToPixelRasterizer(float x, float y, int multiplyer, int divider, IGlyphRasterizer inner) | ||
| public ToPixelRasterizer(int x, int y, int scalingFactor, IGlyphRasterizer inner) |
There was a problem hiding this comment.
I think it's clearer if the divider is passed in as well, instead as grabbed as a constant. Le'ts keep it that way.
| /// This is a struct because its super short lived and it prevents the need for an allocation for each character rendered. | ||
| /// This is internal as it will only ever be used by the <see cref="Renderer"/> and should not be used by external systems. | ||
| /// </remarks> | ||
| internal struct ToPixelRasterizer |
There was a problem hiding this comment.
This should not be shortlived, it should typically live together with the Renderer.
👍 for Internal
There was a problem hiding this comment.
So lets just do a normal class and drop remark.
There was a problem hiding this comment.
it only lives for the life time of of the RenderGlyph(..) call you are newing up a new one each time for the new offsets.
There was a problem hiding this comment.
Yeah, that's definitely not right. This should only be created once per DrawText, or even less. You don't have to fix that problem in this pr of course, but lets keep it a class for now. You can drop a TODO remarking this class is instantiated for each glyph which is not right.
|
|
||
| public int Height; | ||
| } | ||
| } |
| public int bottom; | ||
| public int right; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not introduced here, but no newline at end of file.
| { | ||
| glyph = glyph, | ||
| bottom = lineHeight + yy, | ||
| right = glyphWidth + xx, |
There was a problem hiding this comment.
Bottom right is never used, let's drop that point.
| { | ||
| Width = ((right - left) * scalingFactor) / Constants.FontToPixelDivisor, | ||
| Height = ((bottom - top) * scalingFactor) / Constants.FontToPixelDivisor | ||
| }; |
There was a problem hiding this comment.
I would expect this to be void. Let's remove this return.
There was a problem hiding this comment.
I'm using the fact that it returns the size to enable drawing the bounding box in the CLI.
if it didn't return the size then I would need to call MeasureText and Render separately causing runs of the layout method duplicating that piece of work.
There was a problem hiding this comment.
Yes, but isn't that's a great solution? Just call both Render and MeasureText. This is akin to System.Drawing and nothing strange. The double work is miniscule to say the least.
There was a problem hiding this comment.
ok... I'll drop returning the size for now, I might throw in another PR in the future if I find I actually need it in ImageSharp but tbh I can't think of a performance critical region that would need/use the returned size so dropped 👍
| if (glyph.bottom > bottom) | ||
| { | ||
| bottom = glyph.bottom; | ||
| } |
There was a problem hiding this comment.
Let's remove all these left/right bottom left comparisions. Not needed! :-)
| int left = int.MaxValue; | ||
| int right = int.MinValue; | ||
|
|
||
| foreach (var glyph in glyphs) |
There was a problem hiding this comment.
This should be called layout rather than glyph, right?
| public int bottom; | ||
| public int right; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't like this struct. Let's remove it and instead iterate through glyphs and Points (top left) together somehow. If it turns out tricky, let's keep this class, but just have two memebers TopLeft point and the Glyph.
There was a problem hiding this comment.
The bottom right is required for finding the right most extent of the bounding box and thus the width and the bottom is the same for the height.
If I don't have this struct we would need to ask the typeface multiple times for the glyph and the advance width etc.
There was a problem hiding this comment.
I see, let's keep it like this for now then.
|
Awesome. Some comments, need ro rush now. Should be mergeable pretty soon. Thank you so much. |
|
Some smaller things I can also clean up afterwards. |
tocsoft
force-pushed
the
measure-text-clean
branch
from
36704a9 to
efb75f0
Compare
|
@vidstige think I've covered all your changes. |
|
🎉 yay |
Ported all code over to the updated project structure.
Refactored the rendering and measuring away from the 'layout' code.
This should leave a clean place where all glyph layout can happen, newlines, tabs, wrapping, justification etc independently from rendering and measuring and should make testing the layout options simple to test.
fixes #42