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

New fairly simple boolean edge case #951

Closed
lehni opened this issue Feb 9, 2016 · 15 comments
Closed

New fairly simple boolean edge case #951

lehni opened this issue Feb 9, 2016 · 15 comments

Comments

@lehni
Copy link
Member

lehni commented Feb 9, 2016

This came out of your test suite @iconexperience, by using the slightly different code now for calculating the curve points: Sketch

var path1 = new Path('M387.6708283605882,409.85893821175773c43.34991911960765,43.42692935982444 7.025356174944989,-7.309686568753534 -21.280175804952762,21.350763849679026l42.68991000890696,42.16125690051729c-28.31035574280338,28.66533466571275 -77.0540871232289,-34.32700789309064 -63.873775729843544,-21.123281947448902z');
var path2 = new Path('M366.39065230143535,431.20970231882416c-7.258856574914432,7.349874368209157 -8.587656196327089,15.955888792558142 -8.65222959434982,20.669701880671994l59.99437111077259,0.8218485401191629c-0.06457342267412969,4.7138148876486525 -1.3933735123300153,13.319830673437139 -8.652230999115659,20.669705964951333z');

path1.strokeColor = 'red';
path2.strokeColor = 'green';
path1.strokeScaling = path2.strokeScaling = false;

var res = path1.unite(path2);

res.fillColor = 'yellow';
res.fillColor.alpha = 0.25;
@lehni lehni self-assigned this Feb 9, 2016
@lehni lehni added the type: bug label Feb 9, 2016
@lehni
Copy link
Member Author

lehni commented Feb 9, 2016

It looks like not all intersections are found: Sketch

@iconexperience
Copy link
Contributor

I am not sure. Two intersections in the center (both are crossings) could actually be correct.

@lehni
Copy link
Member Author

lehni commented Feb 9, 2016

Yeah you're right...

screen shot 2016-02-09 at 15 54 03
screen shot 2016-02-09 at 15 53 41

@lehni
Copy link
Member Author

lehni commented Feb 9, 2016

But something with these two found intersections is strange... It's very hard to see though. I can only zoom close enough on Firefox, but even there the glitches make an interpretation pretty much impossible: Sketch

@iconexperience
Copy link
Contributor

The problem is that path 2 has a self intersection, which is first resolved when calling unite(). Then one of the two new sub-paths has only one crossing with path1, which cannot be correct. Here is the sketch

BTW, there is something very strange going on in the sketch. Why does the console show the point instead of a true/false value?

@iconexperience
Copy link
Contributor

Not quite right, the problem is the other sub-path that actually has two intersections with path 1. Here is the simplified example with only two triangles:

var path1 = new Path({segments:[
    [366.3906525556354, 431.20970206143676], 
    [409.0805625645424, 473.37095896195405], 
    [345.20678683469885, 452.24767701450514]
    ], closed:true})
var path2 = new Path({segments:[
    [387.73560545337955, 452.2903284310746],
    [417.7327938178581, 452.7012527396153],
    [409.08056281874246, 473.37095870456665]
    ], closed:true});
path1.strokeColor = 'red';
path2.strokeColor = 'green';

var res = path1.unite(path2);
res.fillColor = 'yellow';

And here is the sketch

@iconexperience
Copy link
Contributor

Am I doing something wrong or does this example actually work on the current develop path branch?

@lehni
Copy link
Member Author

lehni commented Feb 9, 2016

you mean branch? It doesn't work for me...

@iconexperience
Copy link
Contributor

Yes, on the branch. I am a bit confused today.

@iconexperience
Copy link
Contributor

You are correct, it fails on the current develop branch. I better finish for today.

@iconexperience
Copy link
Contributor

As suspected in #959 removing the new edge case handling in CurveLocation.js lines 448ff fixes this issue. But the edge case handling is certainly there for reason.

@lehni Can you give an example of an edge case that CurveLocation.js lines 448ff was introduced for? I am having a difficult time trying to understand what this is actually trying to do.

@lehni
Copy link
Member Author

lehni commented Feb 12, 2016

I developed this code for #865. But many things have since changed, and I am not sure that we still need it. I am a bit surprised that the line intersection code produces a solution for perfectly parallel lines though, it shouldn't really. I'll look into this all a bit more now.

@lehni
Copy link
Member Author

lehni commented Feb 12, 2016

I can find only one failing boolean test, and this is actually not a failure, juts a rendering imprecision that results from a slightly different path topology that is actually more correct. So I think we can safely delete this code and adjust that one test. Well spotted, @iconexperience!

@lehni
Copy link
Member Author

lehni commented Feb 12, 2016

Here the sketch for that one case.

@lehni
Copy link
Member Author

lehni commented Feb 12, 2016

I think that the need for this workaround has disappeared with the recent improvements of precision in Curve evaluate()!

@lehni lehni closed this as completed in 84a75e3 Feb 12, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants