Skip to content
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

reflect NUM_FRACTION_DIGITS to SVG path data #8782

Merged
merged 12 commits into from
Mar 18, 2023

Conversation

tushuhei
Copy link
Contributor

@tushuhei tushuhei commented Mar 14, 2023

Motivation

Fix #5734. toSVG does not consider NUM_FRACTION_DIGITS in exporting path data, which makes the resulting SVG (and JSON eventually) unnecessarily large.

Description

util.path.joinPath() seriarizes SVG path data by joining commands. This change is to extend the function by additing the fractionDigits argument, which represents the number of fraction digits for the serialized SVG path data.

Changes

  • BREAKING: Added a new argument to the joinPath function. If no argument is passed toFixed will round it to an int
  • Call the joinPath function with config.NUM_FRACTION_DIGITS.
  • Add a unit test for the joinPath function.

In Action

<canvas id="fabric"></canvas>
<p id="output"></p>
<div id="reconstructed"></div>
<script src="https://unpkg.com/fabric@5.3.0/dist/fabric.js"></script>
<script>
const canvas = new fabric.Canvas('fabric');
const path = new fabric.Path('M55.23 82.29a11.13 11.13 0 0 0-10.46 0L23.55 95.12c-2.88 1.74-4.61.49-3.84-2.78l5.65-24.15a11.16 11.16 0 0 0-3.23-9.95L3.36 42c-2.54-2.2-1.88-4.23 1.47-4.51l24.71-2.09A11.11 11.11 0 0 0 38 29.27l9.63-22.85c1.3-3.1 3.44-3.1 4.74 0L62 29.27a11.11 11.11 0 0 0 8.46 6.15l24.71 2.09c3.35.29 4 2.31 1.47 4.51L77.87 58.24a11.16 11.16 0 0 0-3.23 9.95l5.65 24.15c.76 3.27-1 4.52-3.84 2.78Z');
canvas.add(path);
fabric.Object.NUM_FRACTION_DIGITS = 0;
document.getElementById("output").textContent = canvas.toSVG();
document.getElementById("reconstructed").innerHTML = canvas.toSVG();
</script>

Before:
Screenshot 2023-03-14 21 49 37

After:
Screenshot 2023-03-14 21 47 18

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Thanks
Good job
Add a changelog please

src/util/path.ts Outdated Show resolved Hide resolved
@ShaMan123
Copy link
Contributor

And run npm run prettier:write and commit the changes

Read the CONTRIBUTING.md for more details on how to finish up the PR

@tushuhei tushuhei requested a review from ShaMan123 March 16, 2023 04:01
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Passing undefined to toFixed rounds the number entirely so that doesn't support what was before.
So I guess we should make the arg non optional as you did but I am not sure.
It is used only by the _toSVG method so I think it doesn't really matter.
@asturur thoughts?

src/util/path.ts Show resolved Hide resolved
src/shapes/Path.ts Outdated Show resolved Hide resolved
ShaMan123
ShaMan123 previously approved these changes Mar 16, 2023
@asturur
Copy link
Member

asturur commented Mar 16, 2023

This is fine.
But in the changelog we need to write BREAKING.

Copy link
Member

@asturur asturur left a comment

Choose a reason for hiding this comment

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

Please add BREAKING in front of the changelog line

@tushuhei
Copy link
Contributor Author

Sure thing, done.

@tushuhei tushuhei requested a review from asturur March 16, 2023 15:21
@asturur
Copy link
Member

asturur commented Mar 16, 2023

@ShaMan123 your question was collapsed for me and i didn't see.
Sorry @tushuhei @ShaMan123 is right. While joinPath is used once, is still an external utility. Forcing the use of toFixed means making impossible to keep the original values.
(please corret me if i m mistaken)

so for joinPath toFixed was good as an optional parameter, and if is isn't passed should not truncte the numbers

src/util/path.ts Outdated Show resolved Hide resolved
@tushuhei tushuhei requested a review from asturur March 16, 2023 21:39
@asturur asturur merged commit b9ea020 into fabricjs:master Mar 18, 2023
@Exlord
Copy link

Exlord commented Apr 27, 2023

Hi, I am using the custom build version which is 5.2. Is this update available there ?

@ShaMan123
Copy link
Contributor

No.
V6 only.
You can patch if you must.

@Exlord
Copy link

Exlord commented Apr 29, 2023

Is version 6 production ready ?

@ShaMan123
Copy link
Contributor

beta
much better than v5

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

Successfully merging this pull request may close these issues.

NUM_FRACTION_DIGITS doesn't work for SVG export
4 participants