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: hide topology errors in line hit detection & optimizations #261

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

megawac
Copy link
Contributor

@megawac megawac commented Aug 9, 2023

This fixes an issue where jsts may throw TopologyError errors when building the intersections due to the snappable lines built having invalid geometry. We also do some minor performance work by

  1. Parsing the lines to jsts representations a single time, instead of each call through the loop
  2. moving the isSameLine check prior to the IntersectionOp for some minor performance in the case we can avoid the work.

Others

  • It's not a hack or at least an unauthorized hack :).
  • Everything in ticket description has been fixed.
  • The author of the MR has made its own review before assigning the reviewer (made this a month ago and has been thoroughly tested).

@vercel
Copy link

vercel bot commented Aug 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
openlayers-editor ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 21, 2024 4:51pm

@oterral
Copy link
Contributor

oterral commented May 10, 2024

@megawac thx for the PRs and sorry for the late answer. I review all of them and try to be responsive in the next month.

try {
intersections = OverlayOp.intersection(parsedLineA, parsedLineB);
} catch (e) {
return; // The OverlayOp will sometimes error with topology errors for certain lines
Copy link
Contributor

Choose a reason for hiding this comment

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

@megawac Have you some more precise info about those topology error ? Could be nice to have an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I seem to recall them occurring quite often when #267 was occurring... I'll see if I can track down if there was more context on internal tickets.

Copy link
Contributor Author

@megawac megawac May 28, 2024

Choose a reason for hiding this comment

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

This is from the original ticket reported on our side. So in the case the case of the error, parsedLineB snap line appears to be invalid from the error below. There is a video of the error occurring but I'd have to get clearance to share I believe

Uncaught TopologyException: found non-noded intersection between LINESTRING ( 9.952973188580247 2.35845900632583, 11.739231890263671 2.3680559374052663 ) and LINESTRING ( 9.952973188580248 2.35845900632583, 11.452360499376073 508.73944164968873 ) [ (9.952973188580248, 2.35845900632583, NaN) ]
    at g.checkValid (bundle.js)
    at E.checkValid (bundle.js)
    at E.checkValid (bundle.js)
    at A.computeOverlay (bundle.js)
    at A.getResultGeometry (bundle.js)
    at A.overlayOp (bundle.js)
    at E.getResultGeometry (bundle.js)
    at E.overlayOp (bundle.js)
    at A.intersection (bundle.js)
    at bundle.js
    at Array.forEach (<anonymous>)
    at bundle.js
    at Array.forEach (<anonymous>)
    at J (bundle.js)
    at ee.drawSnapLines (bundle.js)
    at ee.onMove (bundle.js)
    at a.handleEvent (bundle.js)
    at le.handleMapBrowserEvent (bundle.js)
    at u.dispatchEvent (bundle.js)
    at u.relayMoveEvent_ (bundle.js)
    at HTMLDivElement.d (bundle.js)

Copy link
Contributor

@oterral oterral left a comment

Choose a reason for hiding this comment

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

I've added a file for a unit test. Can you add one tests for the topolgy error?

@oterral oterral changed the title [APPS-13483] fix topology errors in line hit detection & optimizations fix: fix topology errors in line hit detection & optimizations May 10, 2024
@oterral
Copy link
Contributor

oterral commented Jun 20, 2024

@megawac can you fix the conflict? I will merge this even without test because it's usefull.

* github/master: (35 commits)
  chore: at least make sure the demo can draw a point
  chore: fix use of custom layer
  chore: fix demo
  chore: fix changelog
  chore(release): 2.4.2
  chore(release): 2.4.2-beta.1
  fix: listen only when possible
  chore(release): 2.4.2-beta.0
  fix: unlisten only when possible
  chore(release): 2.4.1
  fix: fix editor.remove and add a test
  chore: updates peer depedencies
  chore: updates readme
  chore: updates readme
  chore(release): 2.4.0
  feat: add extentFilter property  in CAD control
  test: add test for editor
  chore: we are not responsible of bad input of the user
  chore: we are not responsible of bad input of the user
  feat: add editor#removeControl (geops#266)
  ...
@megawac
Copy link
Contributor Author

megawac commented Jun 21, 2024

Done, the linting error appears to be an issue with a ci token

@oterral oterral changed the title fix: fix topology errors in line hit detection & optimizations fix: hide topology errors in line hit detection & optimizations Jun 25, 2024
@oterral oterral merged commit ae85670 into geops:master Jun 25, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants