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

Path.area() doesn't calculate area correctly #994

Closed
Grepsy opened this issue Feb 24, 2016 · 24 comments
Closed

Path.area() doesn't calculate area correctly #994

Grepsy opened this issue Feb 24, 2016 · 24 comments

Comments

@Grepsy
Copy link

Grepsy commented Feb 24, 2016

// square 10x10
var path = new paper.Path([
    new paper.Point(0, 0),
    new paper.Point(10, 0),
    new paper.Point(10, 10),
    new paper.Point(0, 10)
]);

console.log(path.area); // outputs 120, expected 100

For a full demo see: http://jsfiddle.net/4hLqhmey/

@lehni
Copy link
Member

lehni commented Feb 24, 2016

Oh dear, indeed. @iconexperience could this be linked to our recent optimizations? Or has the calculation been wrong all along?

@lehni
Copy link
Member

lehni commented Feb 24, 2016

Here the sketch

@lehni
Copy link
Member

lehni commented Feb 24, 2016

And here's the mentioned change: b5af47a

@lehni
Copy link
Member

lehni commented Feb 24, 2016

Indeed, reverting it back, and we get the correct 100... @iconexperience could you look into this again?

@lehni
Copy link
Member

lehni commented Feb 24, 2016

Oh, it looks like it was my fault : ) #788 (comment)

@Grepsy
Copy link
Author

Grepsy commented Feb 24, 2016

Also strange behaviour is when using closed = true it returns half of the area. See sketch

@lehni
Copy link
Member

lehni commented Feb 24, 2016

You,re right, but it returns that regardless of the closed state, treating it as a triangle. It looks like the connecting curve is being ignored here...

@lehni
Copy link
Member

lehni commented Feb 24, 2016

Wait, that example only has three segments, so it is a triangle. For paths with straight lines, the closed state doesn't make a difference.

@Grepsy
Copy link
Author

Grepsy commented Feb 24, 2016

Ok, that's surprising to me because the documentation from closed states:

Specifies whether the path is closed. If it is closed, Paper.js connects the first and last segments.

So I assumed it would 'close' the polygon effectively making it a square. Am I misunderstanding what closed means?

@lehni
Copy link
Member

lehni commented Feb 24, 2016

Yes of course, but what's the area of a path that isn't closed? It could only return 0 if it was to take it into account. If you don't close a path, but fill it with a color (fillColor = ...), the first and the last segment are connected with a straight line. If you close the path, then the handleOut of the last segment and the handleIn of the first are forming the connecting curve instead. area behaves the same way, by always returning the area of the shape enclosed by the path.

@iconexperience
Copy link
Contributor

@Grepsy: I think there may be a little misunderstanding with your sketch. Are you aware that the path in your sketch actually IS a triangle when being closed?

Here is the modified sketch drawing the stroke

I think you simple forgot the fourth point :)

But that does not change the fact that the calculated area is 20% too large.

@Grepsy
Copy link
Author

Grepsy commented Feb 24, 2016

I'm so sorry! You guys are totally right, please excuse me and forget those
last comments.

On Wednesday, 24 February 2016, Jan notifications@github.com wrote:

@Grepsy https://github.com/Grepsy: I think there may be a little
misunderstanding with your sketch. Are you aware that the path in your
sketch actually IS a triangle when being closed?

Here is the modified sketch drawing the stroke
http://sketch.paperjs.org/#S/ZY69DoIwEMdfpekECYGyOECcfAETR9qhlgsY6l3TFjUxvruH0cmb7v+R391Tor2C7ORpgexmWUlH46ZvNopg8yz2AuHOa4BYH9koBo2CZ1CVUKb6ivZPtcpoNGWvccPUzlOCkWk5rvAzU460wIE8RU60nCIAasmxRkeYyEPtaSo+ZRvBlr1oGkFrDmtOYsdn4BHAZSa3SvH3Zy4tgS6Yk+wG83oD

I think you simple forgot the fourth point :)


Reply to this email directly or view it on GitHub
#994 (comment).

@lehni
Copy link
Member

lehni commented Feb 25, 2016

No worries, these things happen all the time : )

@lehni lehni closed this as completed in 69c3470 Feb 26, 2016
@lehni
Copy link
Member

lehni commented Feb 26, 2016

@iconexperience do you think we should bring back getEdgeSum() to determine the orientation of a path? The calculation of the area is a bit more complex, but I quite like the idea that we could use the same code for both values, also since area already supports caching of the result.

@lehni lehni reopened this Feb 26, 2016
@iconexperience
Copy link
Contributor

Not sure, but I think the calculation of the area can be simplified a bit. At least grouping all those 1.5 *, 3.0 * and 0.5 * should be done, but I think a little more simplification is possible. At the end there will probably be not much difference to calculating the edge sum. I will look at it over the weekend.

@lehni
Copy link
Member

lehni commented Feb 26, 2016

Yeah I have that simplification in a stash locally, but wanted to wait for your opinion.

@iconexperience
Copy link
Contributor

On the other hand, using the edge sum is quite elegant and the calculation of the orienation will always be a bit simpler when using the edge sum. But you will have a few extra lines of code. Difficult, but both solutions are acceptable to me. You decide :)

lehni added a commit that referenced this issue Feb 26, 2016
@lehni
Copy link
Member

lehni commented Feb 26, 2016

Haha, yeah... Can we be certain that both change sign in the same situations?

@iconexperience
Copy link
Contributor

Alright, I tried to simplify the calculation of the area, and this is all I can come up with:

var getArea2 = function(v) {
   var p1x = v[0], p1y = v[1],      
        c1x = v[2], c1y = v[3],     
        c2x = v[4], c2y = v[5],     
        p2x = v[6], p2y = v[7];     
    return 1.5 * ((p2y - p1y) * (c1x + c2x)
        + c1x * (c2y - p1y)
        + (p1x - p2x) * (c1y + c2y)
        + c1y * (p1x - c2x)
        + p2y * (c2x + p1x/3)
        - p2x * (c2y + p1y/3)) / 10;
};

There are 21 mathematical operations instead of the 27 operations in the current implementation. This is an improvement, but I am not really happy with the result, because it is all but elegant. Actually, it is the opposite of cool.

@lehni It' really up to you to decide if we should use this simplified version. I would stick with the current implementation, or at least put the original calculation into the code comment, so others can later try again to find a better simplification.

Here is a sketch that compares the results of the original with the new implementation.

@lehni
Copy link
Member

lehni commented Feb 28, 2016

Well I think that's great, and pretty cool actually! Just not very symmetrical, if that's what you mean.

We could shorten it a bit more and use 3 * ... / 20 : ) Still works!

@iconexperience
Copy link
Contributor

I found it incredibly difficult to work on that simplification, but I have a cold, so maybe I just couldn't concentrate very well. If someone else wants to try finding a better solution it may be easier to first replace the variables with a, b, c, d, e..., because all those 1/2, x/y really make your head spin.

Anyway, I am happy that you like it :)

bmacnaughton added a commit to bmacnaughton/paper.js that referenced this issue Mar 22, 2016
Commits not documented listed here, starting with 45595b2. They are not included in the changelog because the appeared to be internal or already documented or they were a fix for a bug that was introduced in "develop" branch. There were commits before 45595b2 but that's when I started recording what I didn't put in the changelog.

Not sure whether the curveAt/CurveAtTime functions are deprecated or removed.

Format: commit message, commit ID. In chronological order of commits:

- Fix unit test error on Node.js 45595b2
- Implement unit tests for SVG Importing, based on visual comparison. … a12e99e
- Fix wrongly copied attributes in Item#reduce() … 8e25327
- Events: paper namespace may not be initialized when key evens are emi… … b2f3b58
- SVG: Some renaming omitted in previous commit. 1c4ff31
- SVG: Rename 'SVG' prefix to 'Svg' … af59847
- Implement tests for paperjs#69721dce1a
- Move Path_Bounds tests to Item_Bounds. … a02d724
- Fix accidentally leaked global variable. 74d1889
- Simplify getWindings(), The first curve of a loop always has the 'las… … 0716ebb
- Merge pull request paperjs#938 from iconexperience/getWinding-simplification … 53269ab
- Optimize Emitter._installEvents() … 0f084ea
- JSON: Prevent `name: undefined` exports. 7888d1d
- Change the way we determine the winding in getWinding(). Now the wind… … aed9d05  (paperjs#936)
- Clean-up changes from paperjs#939 41aca10
- Define unit test for #internalBounds regression. 336460b
- Fix internalBounds regression caused by 1ac8e46 0152439
- Revert "Change the way we determine the winding in getWinding()." f7b1aca
- Use correct SVG namespace again. … fc4bdf4
- Replace the "straight curves with zero-winding" test with a more comp… … e03c8cd
- Change the implementation of getWinding() again, so we pass all tests… … 5b31aee
- Merge pull request paperjs#944 from iconexperience/fix-getWinding … ec75985
- Merge pull request paperjs#943 from iconexperience/replace-path-contains-test … 23045bb
- Do not snap curve points to t = 0 / 1 with epsilon … 0371f66 (in boolean ops)
- Clean-up unit test for paperjs#943 and add edge case from paperjs#944a59a535
- Some comment cleanup. ffe42a0
- Merge branch 'winding-fix' into develop … 5a46620
- Some code cleanup for winding-fix. 55909b8
- Include NPM and Bower badges. 80e6246
- Improve handling of view updates and detection of invisible documents. … da216aa
- Fix new exception in unit tests. de9653a
- Rearrange method sequence in Path. d1b11c6
- Clean-up PathFitter code. e5d139c
- No need to pass normalized tangents to PathFitter#fitCubic() … c793538
- Fix JSDoc warning on Style class. a48d138
- Implement PathItem#getNearestLocation() / #getNearestPoint() … 717bc4b
- Implement Item#_hitTestChildren() … 740c94e
- Some CurveLocation cleanup. … 00d2e2a
- Remove duplicate unit tests. ed43477
- SVGExport: Remove unnecessary calls to Point#transform() in exportGra… … adc5b86
- Remove unnecessary double-spaces. 98fc513
- Improve fix for paperjs#650c1b7366
- SVGImport: Further improve handling of gradients … d9e09b9
- Gulp: Add test:browser task, to solve CORS issues on Chrome. 8542eb6
- SVGImport: Improve consistency of style handling. df57c4a
- SVGImport: Inherit default styles on Node.js too. e38a33f
- SvgImport: Always create a clip-item when viewBox is specified. 68c4541
- Tests: Implement additional tests for SvgImport. c0b39c4
- Travis CI: Use Arial in all tests. cb79232
- Shortcut Curve.evaluate() for t === 1 to avoid imprecision. aa1f219 (paperjs#960)
- Part 1 of large refactoring of bounds handling. 55c5f42
- Update straps.js 892e567
- Part 2 of large refactoring of bounds handling. 12f829c
- Travis CI: Switch to g++ 4.8 to see if this solves strange new buildi… … 5ec5c26
- Introduce Base.filter(), to copy and filter object properties. 6d5d1ce
- Implement unit tests for Item#getItems() with overlapping / inside pr… … 80c8aae
- Merge pull request paperjs#962 from iconexperience/fix-issue-960 … 7c24fc9
- Add test for paperjs#960 and improve fix a bit. … e2bc83a
- Remove unnecessary edge-case handling in CurveLocation#isCrossing() … 84a75e3 (paperjs#951, paperjs#959 - but falls into boolean fixes in general).
- Implement consistent checks for fill / stroke / shadow styles in test… … c6bcf43
- Clean-up previous commit. 0a196da
- Boolean: Implement proper handling of fully overlapping (identical) p… … 3348fb7 (paperjs#923, paperjs#958 - but falls into boolean fixes in general).
- Boolean: Only compare segments when determining if paths are identitcal. 009761d (boolean fixes in general).
- Switch from new Base() to Base.set({}) where possible. c3fff9f
- Docs: Fix warning about isFlatEnough() 27197bd
- Better detect code that requires a tool object. … dbd7a90 (in example fixes)
- Matrix: Switch to a better implementation of #decompose() … 3ee46ff
- Implement failing test for paperjs#968 9c9f43d
- Hit-Test: Pass viewMatrix as argument instead of in options object. fa6c1f4
- Update Curve.js … fb76065
- Update Path.js … add2866
- Clean up PR paperjs#93179d4461
- Improve handling of points on paths in getWinding() e2eaf87
- Adjust comments to match new implementation 406e6c9
- Fix regression introduced in 4e7fa2f 55e7689
- Implement more unit tests for PaperScope#settings.insertItems 01fade8
- Merge pull request paperjs#971 from iconexperience/fix-issue-968 … 4c72d98 (boolean fixes in general)
- Simplify code from paperjs#971 and activate unit test for it again. 8d5c922
- Document options.insert in #importSVG() 3c3c8d9
- List all supported events in event methods on View. 9f9222f
- Switch to PathItem.create() in unit tests. 0e2498b
- Fix failing SVG unit test. 08e51b5
- Fix failing unit tests. 3d330da
- SvgImport: Fix issues introduced in 6f4890c 16a7baa
- Merge pull request paperjs#976 from iconexperience/patch-2 … 7f48486
- SVG Export: Do not filter out empty paths. 6975690
- Fix paperjs#977: Apply hit-testing tolerance to fills in Shape. 4081afb
- Fix paperjs#977: Implement unit-tests. 6df4602
- Fix paperjs#982: Make sure `self` points to the global scope on Webpack. b5c837b
- Extend mapping of attribute names to required namespaces a4757b3
- Add trailing slashes to svg related namespaces (xmlns, xlink) 49104c5
- Merge pull request paperjs#984 from aschmi/fix-namespaces-of-exported-svg … 623ec73
- SVG Import: Fix namespacing issues introduced by paperjs#984. acb1e40
- SVG: Add comments explaining IE related changes in paperjs#984. 50bd5be
- Implement unit tests for paperjs#9911cb2916
- fix paperjs#994: Revert commit b5af47a69c3470
- Rename SegmentSelection related internal objects and properties.  … 1db419a
- Simplify Path#getArea()  … 7dd110f
- jsdom v8.0.0 equires Node.js v4.0.0 or newer. 7300260
- Implement Path#splitAt(offset)  … da7d0d8
- Add more unit tests for SvgImport.  … 7a4794d
- SVG Import: Add more tests.  … d52a6f3
- Refactor GradientStop: Improve handling of optionally defined color a…  … d93aca6
- Travis CI: See if using Arial solves the failing test. 17555b1
- Travis CI: Use Arial in all SVG tests and reduce tolerance. d6ce470
- Travis CI: Adjust SVG test tolerances. beabd6b
- Travis CI: More SVG test adjustments. bb19fad
- Replace Item#_boundsSelected with #_selectBounds  … 336bc10
- More clean-up of selection handling refactoring. 00b2102
@lehni lehni closed this as completed in 20fc3b9 Mar 28, 2016
@deanm
Copy link

deanm commented Apr 20, 2016

I don't know much about paper but I just ran across this bug, and thought maybe this might be useful. Many graphics systems have paths that can be a mixture of lines / quadratics / cubics (and sometimes, unfortunately, conics). I believe what paper basically does now is compute the area by elevating everything to cubic degree. You could also calculate the area directly for line / quadratic / cubic. The math is not difficult, but just in case here is a writeup of the different cases.

http://deanm.github.io/graphics/2016/03/30/CurveArea.html

And JavaScript code for line / quadratic / cubic "signed area under curve".

function line_sauc(x0, y0, x1, y1) {
  return (x0*y1 - x1*y0) / 2;
}

function bez2_sauc(x0, y0, x1, y1, x2, y2) {
  return (  x2*( -y0 - 2*y1) +
          2*x1*(  y2 -   y0) +
            x0*(2*y1 +   y2)) / 6;
}

function bez3_sauc(x0, y0, x1, y1, x2, y2, x3, y3) {
  return (  x3*(  -y0 - 3*y1 - 6*y2) -
          3*x2*(   y0 +   y1 - 2*y3) +
          3*x1*(-2*y0 +   y2 +   y3) +
            x0*( 6*y1 + 3*y2 +   y3)) / 20;
}

@lehni
Copy link
Member

lehni commented Apr 20, 2016

In which version of paper.js are you observing the bug?

That's a great write up, thanks!

@deanm
Copy link

deanm commented Apr 20, 2016

Sorry, by "this bug", I just meant the github issue, and that I just left a comment here in case it was relevant. I didn't mean that I encountered a bug in paper, just the "github issue". : )

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

4 participants