-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Offset the line to fit the glyph with negative side bearing #167
Offset the line to fit the glyph with negative side bearing #167
Conversation
f7a23ad
to
de6f102
Compare
de6f102
to
f69962c
Compare
src/rendering/line_iter.rs
Outdated
if self.empty { | ||
// If this is the first word on the line, offset the line by | ||
// the word's left negative boundary to make sure it is not clipped. | ||
let offset = handler.measure_left_offset(w); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we somehow avoid double-measuring here? I'd probably prefer modifying measure
to return the offset along with the width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Realised that having a more specialised function was nicer than changing the existing measure
function, to avoid touching a lot of places where it is used.
Thanks for working on this, this is an even smaller diff than I expected :) |
@bugadani I started with a much more complex solution and then realised that simply injecting unaccounted whitespace did the trick! Will update the existing measure function to prevent from double measuring. |
Thank you, this is awesome. Two more things and I think we can ship this:
|
@bugadani rendering testing turned out to be quite involved, because I needed to implement a test font. The font I am working on is not released yet, and it might not be a good idea to depend on it. |
4b7756c
to
3db4f67
Compare
@bugadani please review but don’t merge this just yet. I want to run a few more tests next week! |
Apologies, I didn't intend the test to turn this hairy, not sure what went through my head with that. However, thanks for adding it! If you'd like to prevent me from merging, just feel free to mark the PR as a draft (although... request noted even though the green button is tempting :) ), you can find the option top right, under the "Reviewers" box. |
@bugadani unfortunately I found another issue.. this time it is a panic caused by splitting long words in A pixel width of a str is not equal to a sum of widths of individual characters. In my font, the space between a pair of characters depends on the characters themselves (this is called kerning), and also sometimes consecutive characters may be collapsed into a ligature. In the test font that I added, the spacing between the characters is 1px that is not accounted into the char width. This makes the actual length of the string longer than the sum of widths of individual characters. I added a test that panics. I can replace this with a function that iterates over the character indices and measures the str width until that, but it would certainly be slower than the current implementation. |
Yes, you are bringin in a whole new set of assumptions. e-t was built assuming fonts are monospaced, so in several places At this point I can accept compromises in terms of speed, as some of the "optimisations" present were made without any instrumentation. We can tune things later if speed becomes an issue, although I'm quite sure nobody build an e-book reader with e-t :) |
@bugadani I've committed a possible fix for this issue and added tests for TestFont and MonoFont. Do you know any other place where this is assumed? |
I do not but that is mainly because I haven't read this code in a long time. |
I don't see any obvious problems left, I think this PR is now ready. |
Thank you very much for working on this! |
Implements the logic that we discussed in #166.
This is how it works for MogeeFont (with justified text):