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

getPathBBox () in versions #47

Closed
Xyu-xyu opened this issue Jan 6, 2025 · 10 comments
Closed

getPathBBox () in versions #47

Xyu-xyu opened this issue Jan 6, 2025 · 10 comments

Comments

@Xyu-xyu
Copy link

Xyu-xyu commented Jan 6, 2025

Hi! In different versions of the library getPathBBox () returns different results.

for path ='M77.7553 122.1843A15.6631 5.5 45 0 1 92.7199 129.3707L100.7729 137.4237A15.6631 5.5 45 0 1 92.9947 145.2019L84.9417 137.1489A15.6631 5.5 45 0 1 77.7553 122.1843'

2.1.0:
{"width":23.017600000026377,"height":23.0176,"x":77.75529999997363,"y":122.1843,"x2":100.7729,"y2":145.2019,"cx":89.26409999998683,"cy":133.6931,"cz":34.52640000002638}

2.1.7 :
{"width":31.529978953390938,"height":28.952970644914927,"x":77.09231545522431,"y":121.52131545522431,"x2":108.62229440861525,"y2":150.47428610013924,"cx":92.85730493191977,"cy":135.9978007776818,"cz":46.0064642758484}

2.0.0 - 2.0.9:{"width":31.529900130152313,"height":31.529900130152072,"x":77.09235490123895,"y":121.52135490123916,"x2":108.62225503139126,"y2":153.05125503139124,"cx":92.8573049663151,"cy":137.28630496631519,"cz":47.29485019522835}

Version 2.0.0 gives the correct result than later versions

@thednp
Copy link
Owner

thednp commented Jan 6, 2025

I have to disagree there, you should compare with the browser native SVGPath methods, the latest version not only performs faster, but also closer to the native methods, which is the desired outcome.

If indeed we have a problem, please provide some steps to replicate or a codepen or something.

@Xyu-xyu
Copy link
Author

Xyu-xyu commented Jan 8, 2025

I don't understand why. You can change the versions and the data output is:

v.2.0.9: width:31.529900130152313 height:31.529900130152072
v.2.1.7: width:31.529978953390938 height:28.952970644914927
v.2.1.0: width:23.017600000026377 height:23.0176

@thednp
Copy link
Owner

thednp commented Jan 8, 2025

Because we've experimented with more algorithms to compute CubicBezier and Arc segments, and lately largely improved performance overall.

@Xyu-xyu
Copy link
Author

Xyu-xyu commented Jan 8, 2025

It's wonderfull that the work speed has been improved. It is important to maintain the correctness of the data. When comparing version 2.0.9, the results getBBox () and getPathBbox () are identical with minimal discrepancies. However, in the latest version, the height difference is 2.5.

@thednp
Copy link
Owner

thednp commented Jan 8, 2025

If you convert those shapes to CubicBezier does the discrepancy go away? I see your shapes are using Arc segments, and our current implementation works with some approximation formula which is fast but not exactly super accurate or probably don't handle edge cases properly.

We might have over-optimized there a bit. Right now I cannot get into it very deep, so feel free to suggest a fix.

@herrstrietzel
Copy link

2.1.7 :
{"width":31.529978953390938,"height":28.952970644914927,"x":77.09231545522431,"y":121.52131545522431,"x2":108.62229440861525,"y2":150.47428610013924,"cx":92.85730493191977,"cy":135.9978007776818,"cz":46.0064642758484}

height":28.952970644914927 indicates the bottom extreme point wasn't calculated (See codepen ).

2.1.0:
{"width":23.017600000026377,"height":23.0176,"x":77.75529999997363,"y":122.1843,"x2":100.7729,"y2":145.2019,"cx":89.26409999998683,"cy":133.6931,"cz":34.52640000002638}

would be the bbox if no extremes were included.

Image

Not sure if you're still using parts of my script svg-pathdata-getbbox but it should actually return the correct values.
You may check for comparision this test codepen

Worth noting the bottom arc segment would need 2 extremes to be calculated for a correct bbox - maybe the implementation was simplified to search only for 1 extreme per segment?

@thednp
Copy link
Owner

thednp commented Jan 27, 2025

Thanks guys for the report, will have a look at this asap.

@thednp thednp closed this as completed in 7a2e494 Jan 27, 2025
@thednp
Copy link
Owner

thednp commented Jan 27, 2025

@Xyu-xyu and @herrstrietzel please test the latest from github

npm install github:thednp/svg-path-commander

...and confirm the issue is fixed. I added both your path samples to the test suite.

Thanks

@Xyu-xyu
Copy link
Author

Xyu-xyu commented Jan 28, 2025

The problem is solved. Thank you for fixing everything promptly.

Image

Waiting for a fresh update on npm.))

@Xyu-xyu Xyu-xyu closed this as completed Jan 28, 2025
@thednp
Copy link
Owner

thednp commented Jan 28, 2025

@Xyu-xyu @herrstrietzel

Please re-test the latest master and confirm.
I had to fix some test related issues.

v2.1.8 published.

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

No branches or pull requests

3 participants