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

Cyleaflet performance and mouse position mismatch #217

Merged
merged 7 commits into from
May 23, 2024

Conversation

Farkites
Copy link
Contributor

@Farkites Farkites commented May 17, 2024

About

This PR fixes #210 and adds a partial solution for #212

Description of changes

The event ('dragfree add remove') gets trigger for each individual node added. So initially it would get triggered 1000 times for this code you have provided. Adding a debounce should fix this without changing any functionality.

  • [BUG] CyLeaflet: Click position mismatch #212:
    • Adds calls to cy.resize() to refresh the position of the mouse on cytoscape component every time that the viewport changes or the elements are dragged. With this solution only the first interaction will have the wrong position of the mouse and it should be correct afterwards. See my full comment in 212.

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly.
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • At least one person has 💃'd the pull request.

Reference Issues

Closes #210
Partial fix for #212

@Farkites Farkites requested a review from emilykl May 17, 2024 09:05
@Farkites Farkites requested a review from alexcjohnson as a code owner May 17, 2024 09:05
@emilykl
Copy link
Contributor

emilykl commented May 17, 2024

Debounce looks good! 💪 Do you have any performance numbers for load time before & after adding the debounce?

For the click position mismatch, what if if instead of attaching it to the dragfree add remove events, you attach it to the click event, but give it a really long debounce time, like 2-5 seconds? Then it's triggered only when the user actually clicks on the canvas (not when nodes are added), and resize doesn't get called overly often.

Any idea what is the performance impact of calling cy.resize()? Depending on how long that function takes to execute, that determines how careful we have to be about calling it.

@Farkites
Copy link
Contributor Author

Debounce looks good! 💪 Do you have any performance numbers for load time before & after adding the debounce?

For 4000 nodes it takes +90s to fully load without debounce and about 2 seconds with debounce.

For the click position mismatch, what if if instead of attaching it to the dragfree add remove events, you attach it to the click event, but give it a really long debounce time, like 2-5 seconds? Then it's triggered only when the user actually clicks on the canvas (not when nodes are added), and resize doesn't get called overly often.

The click event wouldn't really work since dragging wouldn't trigger it. But I like the idea. The tapstart event should do give the expected behaviour. I think having a really long debounce time would defy the purpose of the resizing since the user would have to wait 2-5 seconds for the mouse position to be correct.

Any idea what is the performance impact of calling cy.resize()? Depending on how long that function takes to execute, that determines how careful we have to be about calling it.

In my computer it takes around 0.13 ms to execute so it should be safe.

@emilykl
Copy link
Contributor

emilykl commented May 22, 2024

Debounce looks good! 💪 Do you have any performance numbers for load time before & after adding the debounce?

For 4000 nodes it takes +90s to fully load without debounce and about 2 seconds with debounce.

98% reduction in load times... awesome. 🏆

For the second fix, I do still wonder whether calling resize() so often might have some unintended side effects, but as long as you haven't seen any issues in your testing, probably fine to merge and fix any issues as they come up.

💪 💪 💪

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

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

Looks great! 💐

@Farkites Farkites merged commit 083dc1b into main May 23, 2024
2 checks passed
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.

Performance of CyLeaflet for larger numbers of nodes (>1000)
2 participants