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

feat: limit max guides generated per feature via an option #271

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

Conversation

megawac
Copy link
Contributor

@megawac megawac commented May 22, 2024

How to

Create some absurd shapes as described in #267 or allow a high nbFeatures. Set limitGuidesPerFeature on your cad control (defaults to 3). Observe that the max number of snapping guides ever shown on the screen is nbFeatures X limitGuidesPerFeatures

Notes on Implementation

I included lodash here as a peer dependency as my project uses it anyway. The main need was to get access to sortByIndex which allows the includedCoords array to be built optimally in the parseCoordList function. This can easily be replaced by something like the following with efficiency tradeoffs

coords.sort((a, b) => euclideanDistance(a, coordinate) - euclideanDistance(b, coordinate));
const includedCoords = coords.slice(0, this.limitGuidesPerFeature);

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.
  • The title is formatted as a conventional-commit message.
  • Tests added.

Fixes #267

Copy link

vercel bot commented May 22, 2024

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

Name Status Preview Comments Updated (UTC)
openlayers-editor ❌ Failed (Inspect) May 22, 2024 5:48am

}
}
} else {
includedCoords = times(coords.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to rely on lodash speciially when native function can do the job. It seems Array.from(Array(coords.length)) does the same .

@@ -1,9 +1,11 @@
import { times, sortedIndexBy } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to not add lodash as library if you can do it without. IS that possible ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fairly easy to do without lodash as mentioned in the original comment. As discussed in the original post, I used it primarily for the sortedIndexedBy optimization of the list build

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.

Complex geometries can generate a large number of cad guides
2 participants