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.

Sign up for GitHub

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

Jump to bottom

Fix getNullBucket crash and remove dead code #6603

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Fix getNullBucket crash and remove dead code #6603

merged 2 commits into from
Nov 3, 2022

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Nov 3, 2022 •
edited by daniel-wer
Loading

URL of deployed dev instance (used for testing):

Steps to test:

  • create a new annotation and restrict the mags during creation (e.g., to one mag)
  • open the annotation --> it should not crash

Issues:

(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto self-assigned this Nov 3, 2022
philippotto
philippotto commented Nov 3, 2022
View reviewed changes
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts
Comment on lines -169 to -173
if (this.boundingBox.containsBucket(address, this.resolutionInfo)) {
return NULL_BUCKET;
} else {
return NULL_BUCKET_OUT_OF_BB;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I see, this distinction was only used in the ArbitraryCubeAdapter. However, that wasn't used at all, anymore. So, I unified the two NullBuckets.

philippotto
philippotto commented Nov 3, 2022
View reviewed changes
yarn.lock
@@ -11613,7 +11613,7 @@ rc-table@~7.26.0:
rc-util "^5.22.5"
shallowequal "^1.1.0"

rc-tabs@^12.2.2, rc-tabs@~12.2.0:
rc-tabs@~12.2.0:
Copy link
Member Author

Choose a reason for hiding this comment

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

yarn install produced this change (not for the first time). I think, it needs to be there.

philippotto
philippotto commented Nov 3, 2022
View reviewed changes
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts
Comment on lines +50 to +53
/* Checks whether a bucket is contained in the active bounding box.
* If the passed resolutionInfo does not contain the passed zoomStep, this method
* returns false.
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

This method is only used by DataCube.isWithinBounds which itself also checks for this.cubes[zoomStep] == null. So, the new behavior here is in line with the existing behavior on the call site.
The other usage of containsBucket was removed due to the NullBucket consolidation.

@philippotto philippotto changed the title Hot fix getNullBucket crash and remove dead code Fix getNullBucket crash and remove dead code Nov 3, 2022
daniel-wer
daniel-wer approved these changes Nov 3, 2022
View reviewed changes
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice fix and cleanup 👍 I'll do a quick test once the CI is ready.

Do you follow why this suddenly was an issue after the quick select PR?

@philippotto
Copy link
Member Author

I'll do a quick test once the CI is ready.

The CI currently has some problems. I hope it resolves itself:

[error]   download error: Caught java.net.ConnectException (Connection timed out (Connection timed out)) while downloading https://repo1.maven.org/maven2/com/google/protobuf/protobuf-java/3.15.8/protobuf-java-3.15.8.pom

Do you follow why this suddenly was an issue after the quick select PR?

Yes, the quick-select PR changed the bounding box method to accept the resolution info. The old code simply accessed the Store to get the resolutions (for the entire dataset). I had to change this due to cyclic references. The resolution info that is passed only refers to the layer (instead of the datasets resolutions). This made sense to me because the bounding box was used in the context of a layer, but I didn't think of the case where non-existing zoom steps are requested. Changing it back to use the dataset resolutions would have probably been ok, too, but I wanted to avoid the usage of Store (especially, because I was burnt with the cyclic dependencies before).

@philippotto philippotto merged commit 604bf48 into master Nov 3, 2022
@philippotto philippotto deleted the hot-fix-getNullBucket-crash branch November 3, 2022 10:52
@daniel-wer
Copy link
Member

Thank you for clarifying, makes sense :)

hotzenklotz added a commit that referenced this pull request Nov 4, 2022
@hotzenklotz
Merge branch 'master' of github.com:scalableminds/webknossos into pri…
2307b59 
…cing

* 'master' of github.com:scalableminds/webknossos:
  Fix getNullBucket crash and remove dead code (#6603)
  Hotfix Dataset View Mode
hotzenklotz added a commit that referenced this pull request Nov 8, 2022
@hotzenklotz
Merge branch 'master' of github.com:scalableminds/webknossos into mes…
fd4cfa3 
…sages

* 'master' of github.com:scalableminds/webknossos:
  Remove superfluous datastoreName param for convertToWkw job (#6606)
  When downloading with skipVolumeData, keep volume tag (#6566)
  Fix getNullBucket crash and remove dead code (#6603)
  Hotfix Dataset View Mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-wer daniel-wer daniel-wer approved these changes

Assignees

@philippotto philippotto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@philippotto @daniel-wer