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

Fix Line#getSignedDistance and add tests to avoid regression #1880

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

roissard
Copy link

@roissard roissard commented Dec 1, 2020

Description

The proposed calculation for getSignedDistance is wrong.

When rewriting:

Math.sqrt(vxvx+vyvy)

into:

vy * Math.sqrt(1 + (vx * vx) / (vy * vy))

The sign of vy may break the equivalence, it should be:

Math.abs(vy) * Math.sqrt(1 + (vx * vx) / (vy * vy))

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the JSHint rules (npm run jshint passes)

@lehni
Copy link
Member

lehni commented Mar 14, 2021

I am getting this error when running the tests locally, rebased on top of the develop branch:

[14:32:29] gulp-qunits (Node): ✖ Test failed: 
[14:32:29]     Path Boolean Operations: Isolated edge-cases from @iconexperience's boolean-test suite
[14:32:29]     Failed assertion: , expected: 100.00% identical, was: 50.00% identical
[14:32:29]         at Object.push (/Users/lehni/Development/Paper.js/paper.js/node_modules/qunitjs/qunit/qunit.js:2194:12)
[14:32:30] gulp-qunits (Node): ✖ Took 12600 ms to run 13848 tests. 13847 passed, 1 failed.

Strangely this only happens in the tests that run in Node, and not the browser ones… I need to look more into which test actually fails, the message is somehow missing. But we need to figure this out before being able to merge.

@lehni
Copy link
Member

lehni commented Mar 14, 2021

If you rebase on top of develop, you'll get the test message now:

[14:49:49] gulp-qunits (Node): ✖ Test failed: 
[14:49:49]     Path Boolean Operations: Isolated edge-cases from @iconexperience's boolean-test suite
[14:49:49]     Failed assertion: path1.intersect(path2); // Test 8, expected: 100.00% identical, was: 50.00% identical
[14:49:49]         at Object.push (/Users/lehni/Development/Paper.js/paper.js/node_modules/qunitjs/qunit/qunit.js:2194:12)

So in short, it's this test:

const p1 = PathItem.create([[249.53400844979302, 269.5706278725207, 0, 0, -5.202299838213264, 1.3434437726778015], [243.02912232136828, 272.45353289318496, -0.02459817048722357, 0.029126503110205704, 0, 0], [197.1892333604494, 233.7404268430289], true])
const p2 = PathItem.create([[243.02912232136822, 272.453532893185, 0, 0, 0.44566136595048533, -0.527704170852985], [242.66927041668393, 276.2408467361064, -0.16957999946740188, -3.971488536288973, 0, 0], [197.18923336044946, 233.74042684302884], true])

p1.strokeColor = 'red'
p2.strokeColor = 'green'
p1.strokeScaling = p2.strokeScaling = false;

const result = p1.intersect(p2)

result.fillColor = 'yellow'
result.selected = true

view.matrix = [1769.1029649736397, 0, 0, 1769.1029649736397, -429598.57726305583, -481524.06395399297]

If I run this against v0.12.12, I get the expected result:

image

But run against your change, the result is empty…

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

Successfully merging this pull request may close these issues.

2 participants