Skip to content

[DRAFT] ITEP-66486 - Replace clipperjs Part 1 #210

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jpggvilaca
Copy link
Contributor

@jpggvilaca jpggvilaca commented May 13, 2025

📝 Description

Previous PR was too big so i will make this change in multiple PRs.
This will be the biggest since it has 2.5k lines on the package-lock file alone

PR 1 (current)

  • Add turf and geojson
  • Add geometry-utils and its tests
  • Replace clipper on selecting-tool

PR 2 (next)

  • Replace a few more utils (clipper -> turf)

PR 3 (final)

  • Replace leftover methods
  • Uninstall clipperjs

✨ Type of Change

Select the type of change your PR introduces:

  • 🐞 Bug fix – Non-breaking change which fixes an issue
  • 🚀 New feature – Non-breaking change which adds functionality
  • 🔨 Refactor – Non-breaking change which refactors the code base
  • 💥 Breaking change – Changes that break existing functionality
  • 📚 Documentation update
  • 🔒 Security update
  • 🧪 Tests

🧪 Testing Scenarios

Describe how the changes were tested and how reviewers can test them too:

  • ✅ Tested manually
  • 🤖 Run automated end-to-end tests

✅ Checklist

Before submitting the PR, ensure the following:

  • 🔍 PR title is clear and descriptive
  • 📝 For internal contributors: If applicable, include the JIRA ticket number (e.g., ITEP-123456) in the PR title. Do not include full URLs
  • 💬 I have commented my code, especially in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ✅ I have added tests that prove my fix is effective or my feature works

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the annotation intersection logic by replacing clipper-js operations with Turf-based geometry utilities and adds a new geometry-utils module. Key changes include:

  • Removal of clipper-js imports and transformation logic in the selecting-tool utilities.
  • Introduction of a new geometry-utils file that wraps Turf functions for various geometry operations.
  • Addition of tests for the new geometry utilities.

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

File Description
web_ui/src/pages/annotator/tools/selecting-tool/utils.ts Removed clipper-js logic and replaced it with a Turf-based shapesIntersect function to check geometry intersections.
web_ui/src/core/annotations/geometry-utils.ts Introduced a suite of Turf-based functions for polygon conversion, area calculation, union/difference, and point-in-polygon checks.
Files not reviewed (1)
  • web_ui/package.json: Language not supported
Comments suppressed due to low confidence (1)

web_ui/src/pages/annotator/tools/selecting-tool/utils.ts:44

  • Ensure that the updated intersection logic using shapesIntersect is comprehensively covered by unit tests, verifying behavior that matches the previous clipper-js implementation.
if (shapesIntersect(shape, annotationShape)) {

@@ -37,6 +38,7 @@
"dotenv": "^16.5.0",
"filesize": "^10.1.6",
"framer-motion": "^12.9.4",
"geojson": "^0.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use case of geojson versus turf? From their readme it looks like it is very specific to geo-data (e.g. latitude and longitude information), do we need this to be able to use turf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More context: https://turfjs.org/docs/getting-started#typescript . But perhaps i can add it to devDependencies?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm but if they're only types then could we skip installing it completely? To me it would be fine if we don't use these type explicitly as they won't be part of any type contract.

@jpggvilaca jpggvilaca changed the title ITEP-66486 - Replace clipperjs Part 1 [DRAFT] ITEP-66486 - Replace clipperjs Part 1 May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants