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 crashes related to maximizing/minimizing #5746

Merged
merged 3 commits into from
Sep 29, 2021
Merged

Conversation

daniel-wer
Copy link
Member

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

  • Fix a crash when maximizing in flight mode (not part of the issue, but encountered while I was at it)
    • In-depth explanation: When maximizing the flight mode, the activeViewport was set to undefined leading to an error when calculating the global position later. activeViewport is assumed to always be defined.
  • Fix a crash when minimizing the 3D-View in datasets with lots of magnifications (10+)
    • In-depth explanation: When the 3D-View was maximized, the bucket picking still tried to select buckets. However, due to an area of 0, the results was only a single bucket for each viewport. Its position was 0, 0, 0 and its magnification was the lowest available one, for example 10. Then when the 3D-View was minimized again, the getBaseBucketsForFallbackBucket function is called to compute all bucket addresses for which the fallback bucket is the provided bucket. The fallback bucket in question is the one in mag 10 whereas the current mag is 1, leading to way too many bucket addresses being computed and usually an OOM error.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • To reproduce the flight mode maximizing crash: Open a tracing, switch to flight mode and then maximize the viewport. The page should not crash.
  • Reproducing the other crash is only possible for datasets with lots of mags (see for example https://webknossos.org/datasets/scalable_minds/l4dense_motta_et_al_demo_v2/view#2598,4693,1792,0,1).
    • Open the dataset in view mode and maximize the 3D-View, then minimize it again (on master wK freezes and eventually OOMs).
    • I checked that the 0, 0, 0 bucket in the lowest resolution is no longer selected during bucket picking (thus avoiding the OOM) by setting a breakpoint in layer_rendering_manager line 238 checking the result of consumeBucketsFromArrayBuffer.

Issues:


@daniel-wer daniel-wer self-assigned this Sep 28, 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.

Very cool that you tracked this down and found a fix 💯 🎉

I think the bug came into existence because of two factors:

  • the bucket picker didn't catch the zero-bucket case (which you have fixed now)
  • the texture bucket manager was happy to gather thousands and thousands of buckets even though this should never be necessary

The second factor is relatively new, I think, and stems from this PR: https://github.com/scalableminds/webknossos/pull/5674/files#diff-9005d9d1722fdaae1ed8348593ca56bbb599f4733ba312e639b319908c31b819R383
Before that PR, the texture bucket manager only allowed a zoom difference of 1. The PR lifted the limitation with the reasoning that a limit of 3 (see getMaxZoomStepDiff()) is acceptable and will never be violated, anyway.

I'm wondering whether the texture bucket manager should get another check using getMaxZoomStepDiff() in _getBaseBucketAddresses. Something like this:

_getBaseBucketAddresses(bucket: DataBucket, zoomStepDifference: number): Array<Vector4> {
    if (getMaxZoomStepDiff() < zoomStepDifference) {
      return [];
    }
    const resolutions = getResolutions(Store.getState().dataset);
    return getBaseBucketsForFallbackBucket(bucket.zoomedAddress, zoomStepDifference, resolutions);
  }

Strictly speaking, that check should never be necessary, but the current bug showed that the scenario can slip through. To avoid the huge computational burden in that error case, the additional check seems like a good safety net. Do you agree?

@daniel-wer daniel-wer changed the title Fix crashes related to maximzing/minimizing Fix crashes related to maximizing/minimizing Sep 29, 2021
@daniel-wer
Copy link
Member Author

Strictly speaking, that check should never be necessary, but the current bug showed that the scenario can slip through. To avoid the huge computational burden in that error case, the additional check seems like a good safety net. Do you agree?

Yep good idea, I added the safeguard!

@philippotto
Copy link
Member

Awesome :)

@daniel-wer daniel-wer merged commit 1fb0ec3 into master Sep 29, 2021
@daniel-wer daniel-wer deleted the fix-maximizing-issues branch September 29, 2021 12:37
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.

Maximizing 3D Viewport sometimes breaks webKnossos
2 participants