Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes for several rounding mistakes to achieve better precision, accuracy, and usability #840

Merged
merged 6 commits into from
Dec 8, 2020

Conversation

michaelcbrook
Copy link
Contributor

First of all, really appreciate all your hard work on this library.

In the spirit of improving it and solving potentially many issues users could have with it (myself included), I've identified and fixed many instances of incorrect multiplication or rounding, which will resolve many common mistakes when passing decimal numbers into the API, and will actually increase the accuracy of the resulting PPTX to be perfectly consistent with the input style parameters.

The fixes:

  1. Font sizes. Virtually all font sizes are using the following calculation: Math.round(fontSize) + "00". While this doesn't cause the resulting file to be corrupt, it removes a great deal of precision. If the font size input is 12.5, the output becomes 13. Alternatively, all of these should be Math.round(fontSize * 100). This will produce exactly the right result of 12.5.

  2. lineSpacing. The above fix was applied partially to lineSpacing recently, however, lineSpacing was only multiplied by 100 but the result was not rounded. As a result, round-off errors could still occur. (i.e. 70.79 * 100 = 7079.000000000001 in JavaScript). Rounding additionally after the multiplication prevents this. This problem bit me a number of years ago. Unfortunately, if this fix isn't applied, there is no workaround for making this work.

  3. Rotations. Similar to the above, the result was not being rounded, which made it susceptible to the round-off error. It also would fail if the input rotation was much more than four decimal places long.

  4. Transparencies. Specifically, shapeFill alpha, shapeFill transparency, and chartColorsOpacity all were similar to # 1, simply being suffixed with "000" instead of properly being multiplied by 1000 and then rounded. Using decimals in these values would have simply not worked before, when they could completely be supported and produce a result with higher precision and accuracy.

Hopefully these changes will help prevent a lot of the hangups people first have when using it (not knowing which numbers accept decimals, which don't, how many decimals, etc.) and give everyone the ability to achieve near-perfect precision that was needlessly prevented before just because of the subpar multiplication logic.

Thanks again for the great library.

…d-off errors), all font sizes (resulting in greater accuracy), rotation angles (only accepted up to four decimal places before), shapeFill alpha and transparency (did not allow decimals), and chartColorsOpacity (same)
@gitbrent gitbrent self-assigned this Sep 3, 2020
@gitbrent gitbrent added this to the 3.4.0 milestone Sep 3, 2020
… a bold, italic, strike, or underline value set explicitly to false inherits the value of the parent textbox instead of removing styling.
@michaelcbrook
Copy link
Contributor Author

michaelcbrook commented Sep 7, 2020

Just wanted to make a note on this additional commit. It contains:

  1. Additional places where rounding was missed but should be performed (that I found later).

  2. An unrelated bug fix from lines 2163 - 2166 that fixes a bug where, if a parent textbox is set to bold, italics, underline, or strike, words inside of the textbox can't be un-bolded, un-italicized, un-underlined, or un-striked by explicitly setting the word-level formatting to false. This is because the code only explicitly adds the associated attribute for the style if the value is truthy, but in cases where it's explicitly false, the attribute should be added with a negatory value in order to not just inherit the style from the parent textbox.

Thank you!

Edit: Flubbed on # 2. My changes didn't work for some reason. Will repost here when fixed, or feel free to jump in and see what I missed.

@michaelcbrook
Copy link
Contributor Author

Fixed! 👍

Copy link
Owner

@gitbrent gitbrent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect

@gitbrent gitbrent merged commit 183300d into gitbrent:master Dec 8, 2020
gitbrent added a commit that referenced this pull request Dec 8, 2020
gitbrent added a commit that referenced this pull request Dec 8, 2020
@gitbrent
Copy link
Owner

gitbrent commented Dec 8, 2020

Thanks @michaelcbrook - great work!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants