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

Use mag in data requests, no more zoomStep/exponent #6159

Merged
merged 20 commits into from
May 9, 2022
Merged

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Apr 20, 2022

  • use mag instead of zoomStep in data requests
  • start renaming some resolution and zoomStep to mag in the backend internally (some places still use resolution)
  • remove unused datastore routes
  • clean up thumbnail management (no more json with base 64 encoding)
  • fixed the snapshot tests, see discussion below

URL of deployed dev instance (used for testing):

TODO

Follow-Up

Steps to test:

  • Load some data, isosurfaces
  • use rawCuboid via frontend API, request dataset thumbnail

Issues:


@fm3 fm3 self-assigned this Apr 21, 2022
@daniel-wer
Copy link
Member

@fm3 I'm not sure whether I got everything, but I switched the bucket request, the ad-hoc isosurface computation request, and the data cuboid request code. In my testing all of these work as expected, again. However, I'm not sure whether there's more interfaces that have changed.

@daniel-wer
Copy link
Member

daniel-wer commented May 2, 2022

I also just noticed that the snapshot tests were not doing anything, because the snapshots were missed to move when the test directory was changed in #6065 /cc @philippotto. I corrected that, let's see whether any errors pop up.

Edit: I added a82b6a1 so that this won't happen in the future. If a snapshot doesn't exist and the tests are run in a CI environment, the tests will fail.

@fm3 fm3 changed the title [WIP] use mag in data requests, no more zoomStep/exponent Use mag in data requests, no more zoomStep/exponent May 3, 2022
@fm3 fm3 marked this pull request as ready for review May 3, 2022 12:14
@fm3 fm3 requested a review from jstriebel May 3, 2022 12:27
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

Awesome stuff, huge fan of the restructured datastore API ❤️
The backend changes look very good to me, props for using mags consistently, this helps readability in several places!

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

The frontend part looks fine to me 👍 🔍 🚀

*
* @ignore
*/
_getDownloadUrlForRawDataCuboid(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice refactoring 👍

Comment on lines +184 to +187
getHighestResolution(): Vector3 {
// @ts-ignore
return this.getResolutionByPowerOf2(this.getHighestResolutionPowerOf2());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that this method isn't used anywhere. Only getLowestResolution is being used.

But I think it is fine to keep this method in case someone wants the highest resolution in the future.

@fm3 fm3 merged commit d8d5d59 into master May 9, 2022
@fm3 fm3 deleted the data-request-mag branch May 9, 2022 06:18
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.

bucket requests: replace zoomStep by actual (vec3) mag
4 participants