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

optimize isEmptyDeep #1230

Merged
merged 4 commits into from
Nov 1, 2022
Merged

Conversation

drzhbe
Copy link
Contributor

@drzhbe drzhbe commented Sep 23, 2022

TL;DR isEmptyDeep is now 300 times faster

Function isEmptyDeep suffered from 2 problems:

  1. Unnecessary memory allocations
  2. No early return

And since it's used in the snapping code, whenever you have many or just big geometries, the geometry editing is janky.

In this PR I optimized the isEmptyDeep function and added the tests for it.

Video explanation:
https://www.loom.com/share/e70379b76e5141d984e773b70dd47921

Benchmark:
https://jsbench.me/h8l8dv6d7h/2

Copy link
Collaborator

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

Wow that is great! Thank you!

Please add a few more tests like requested and then I think we can merge this

@drzhbe
Copy link
Contributor Author

drzhbe commented Sep 23, 2022

@Falke-Design I've added the requested tests and made === undefined && === null explicit
The tests look pretty tidy to me, wdyt?
Screen Shot 2022-09-23 at 9 35 18 pm

@drzhbe drzhbe requested a review from Falke-Design September 23, 2022 12:31
Copy link
Collaborator

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

This is really a nice improvement! Thank you very much!

@codeofsumit will make merge before the next release

@drzhbe
Copy link
Contributor Author

drzhbe commented Sep 23, 2022

@Falke-Design Thank you for the review! Do you know when approximately next release is gonna happen? Is it weeks/months away?

@Falke-Design
Copy link
Collaborator

Maybe with the end of oktober. I need first to fix most of the bugs but currently I have not much time

@drzhbe
Copy link
Contributor Author

drzhbe commented Sep 25, 2022

@Falke-Design I simplified the function a bit, removed the inner function hasValues. Now isEmptyDeep itself is recursive.

@Falke-Design
Copy link
Collaborator

I'm thinking about, if we should rename the function to hasValues, it would make it easier to understand the code in the function. Or does it not matter? What do you @drzhbe think?

@drzhbe
Copy link
Contributor Author

drzhbe commented Sep 29, 2022

@Falke-Design I agree the function reads a bit more straightforward now. And the name is shorter. Good suggestion 👍

@drzhbe drzhbe requested a review from Falke-Design September 29, 2022 02:32
Copy link
Contributor

@codeofsumit codeofsumit left a comment

Choose a reason for hiding this comment

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

Amazing !!

@codeofsumit codeofsumit merged commit 4b19cb6 into geoman-io:develop Nov 1, 2022
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

Successfully merging this pull request may close these issues.

3 participants