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 unsafe integer usage by using string keys instead #5724

Merged
merged 4 commits into from
Sep 13, 2021

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Sep 9, 2021

In skeleton.js a recursive cantor pairing was used to combine three integers into a unique single integer which is used as a Map key. However, these numbers quickly become very large, exceeding javascript's Number.MAX_SAFE_INTEGER. In that case, the Map access to delete edges will fail sometimes resulting in wrongly colored or left over edges.
I switched to a simple string concatenation approach to create the unique keys. According to a quick search this shouldn't be much slower and I didn't notice any slow-downs.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open an empty tracing and drag-and-drop this NML into it
    orphan-nodes.zip
    • Make sure the tracing is empty so that the IDs are not relabeled!
  • Jump to position 198722, 346458, 5801 and reload the page to fix an issue with the 3D view if annotations are far outside of the dataset
  • Remove the node there, then delete the smaller of the two agglomerates that were created by the split
    • On master you'll see some left over edges without nodes and warnings in the console ([Skeleton] Unable to find buffer position for id 78519564543334160, ...)
    • On this branch the agglomerate should be correctly deleted without any left over edges

Issues:


@daniel-wer daniel-wer added the bug label Sep 9, 2021
@daniel-wer daniel-wer self-assigned this Sep 9, 2021
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome fix! Great testing steps, too 👍 Worked perfectly.

and reload the page to fix an issue with the 3D view if annotations are far outside of the dataset

I did not see this for master nor for this branch. Is this something we should take care of?

@daniel-wer
Copy link
Member Author

I did not see this for master nor for this branch. Is this something we should take care of?

No, I don't think so. I have only ever encountered this during development where I sometimes use NMLs from much larger datasets. This is not a real-world use case, I would say.

@bulldozer-boy bulldozer-boy bot merged commit ef535e5 into master Sep 13, 2021
@bulldozer-boy bulldozer-boy bot deleted the fix-unsafe-integer-usage branch September 13, 2021 09:11
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.

3D viewport shows orphan nodes and edges after splitting tree and deleting one component
2 participants