utilities: fix issues related to conversion of lengths to pt #84
Conversation
|
Oh, I didn't realize that the problem occurred even when you were reducing the stroke below a single cell (not between cells, which is expected to fail). That is interesting. Thanks for looking into this! I can't take a look at the proper logic right now, but I'll give some initial feedback. |
dixslyf
force-pushed
the
fix-pt-conversion
branch
from
176402a
to
c27b067
Compare
|
I've rewritten the functions to use regex (yeah, quite ugly) and added some tests ( One concern I have: the |
dixslyf
force-pushed
the
fix-pt-conversion
branch
2 times, most recently
from
65aae94
to
a9f73ca
Compare
The `convert-length-to-pt` function did not properly handle converting negative `length`s to pt. They would get converted to 0pt because the line drawn for measurement would (I presume) have a width of 0pt when given a negative `length`.
`convert-length-to-pt` had a couple of issues regarding relative lengths. When the `length` component has an em part, there would be an error because Typst does not allow comparing `length`s with an em part with those that do not. It also did not account for ratios with a fraction (e.g., 1.5%). This commit fixes those issues and, at the same time, simplifies the implementation to re-use the other conversion functions for better consistency.
dixslyf
force-pushed
the
fix-pt-conversion
branch
from
a9f73ca
to
e75ce59
Compare
That seems to be the case since before Typst was released... In this case, I think a good workaround might be to keep the old algorithm (just measure the line) if there are no negative numbers in the length's parts (checking if either of the "minus" symbols is present in the repr() should be enough). That way, we only lose a bit of precision when dealing with the specific case of negative numbers. Of course, this is bound to be improved if/when we switch to 0.7.0's constructs. |
Seems like this is true for the other length types as well (and |
dixslyf
force-pushed
the
fix-pt-conversion
branch
2 times, most recently
from
967115a
to
b64ca4a
Compare
|
I've gone ahead and implemented what I came up with in #84 (comment). I've also managed to come up with a solution for the same precision issue for On the testing side:
I think we're nearly done with this PR. All that's left is another round of review and ensuring that this PR works with older versions of Typst (still trying to work out the best way to do this). I've left the commit history mostly intact because I didn't want to accidentally remove commits when force pushing. However, I can squash some of them if you prefer. EDIT: Forgot to check for regressions; will do tomorrow. |
|
Thanks, you're doing great work! I'll try to provide a final round of review soon. Regarding commit history, don't worry too much about it, I'll probably squash before merging. |
Unfortunately, `repr` rounds the `pt` component of `length`s to 2 decimal places, which results in a loss of precision. This commit rewrites the conversion of `length`s to `pt` without relying on the numerical values in the `repr`. We instead draw and measure lines.
Typst rounds all length components, except `em` components, to 2 decimal places for their `repr`, which results in a loss of precision. This commit resolves the loss of precision. Some aspects of Typst's behaviour are important: - The `repr` of `em` is lossless. - The "draw and measure a line" technique applied on `relative` lengths returns the length of the `length` component only (the ratio component is ignored due to limitations of the technique).
dixslyf
force-pushed
the
fix-pt-conversion
branch
from
d78ff3d
to
5cd75f6
Compare
|
Re-ordered the commits so that changes to I managed to hack together a bunch of scripts using Nix and diff-pdf to semi-automate regression testing for the different Typst versions. (Didn't include them here since they're out of scope and pretty makeshift, but they can be found at: dixslyf@7d078eb.) I don't see any regressions for any of the supported Typst versions. |
|
Thank you! The result looks great. I also appreciate very much that you tested much more than I could ever ask someone to! 😂 Great job 👍 |
|
I plan on having a better testing system in the future. I'll probably consider using PNG instead of PDF though as it's a pain to work with :p I'm particularly interested in Lau's efforts towards a standard testing solution for packages, although I might also try out something like CeTZ's system. We'll see... |
Indeed, PDFs are a huge pain to work with. I initially wanted to go with PNG for my tests, but PNG export was only added in Typst v0.5.0. What I tried at first was to convert the PDFs to PNGs using ImageMagick (I also tried Another thing I tried was to compare the output across different Typst versions (e.g., compare the outputs for Typst v0.2.0 and v0.9.0). The outputs were slightly different — mostly differences in spacing. However, even for pages that look the same to our eyes, if you compared them using
Same!
I took a look at CeTZ's testing when I was trying to test this PR, and it did look nice and promising. My only concern was it tests only a single version of Typst as far as I can tell. It's probably not too difficult to expand on that, however (e.g., modifying |
* utilities: fix conversion of negative `length` lengths The `convert-length-to-pt` function did not properly handle converting negative `length`s to pt. They would get converted to 0pt because the line drawn for measurement would (I presume) have a width of 0pt when given a negative `length`. * utilities: factor out pt conversions into functions * utilities: simplify and fix conversion of relative lengths to pt `convert-length-to-pt` had a couple of issues regarding relative lengths. When the `length` component has an em part, there would be an error because Typst does not allow comparing `length`s with an em part with those that do not. It also did not account for ratios with a fraction (e.g., 1.5%). This commit fixes those issues and, at the same time, simplifies the implementation to re-use the other conversion functions for better consistency. * utilities: convert `convert-length-to-pt` variables to kebab case * utilities: convert `length`s to pt without losing precision Unfortunately, `repr` rounds the `pt` component of `length`s to 2 decimal places, which results in a loss of precision. This commit rewrites the conversion of `length`s to `pt` without relying on the numerical values in the `repr`. We instead draw and measure lines. * utilities: convert `relative` lengths to pt without losing precision Typst rounds all length components, except `em` components, to 2 decimal places for their `repr`, which results in a loss of precision. This commit resolves the loss of precision. Some aspects of Typst's behaviour are important: - The `repr` of `em` is lossless. - The "draw and measure a line" technique applied on `relative` lengths returns the length of the `length` component only (the ratio component is ignored due to limitations of the technique). * utilities: assert `page-size` and `frac-total` are purely pt lengths * tablex-test: add tests for `convert-length-to-pt` * tablex-test: add tests for line expansion * tablex-test: check return type of conversion to pt * tablex-test: test precision of conversion to pt * add issue number to tablex-test.typ * pdf will be in separate commit
This PR fixes some issues regarding the conversion of lengths to pt
in hopes of progressing #74. Please see each commit's message for details.I don't see any regressions in
tablex-test.typ(compiled it to PNG before and after this PR's commits, then checked their hashes).I'm marking this as a draft for now as I haven't added any tests. I did some quick testing with negative line expansion, and surprisingly, it seems to work, so I'd like to investigate this further before readying this PR.EDIT: should resolve #74.