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

Support non-quadratic viewports #3634

Merged
merged 36 commits into from
Feb 18, 2019
Merged

Support non-quadratic viewports #3634

merged 36 commits into from
Feb 18, 2019

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Jan 16, 2019

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • test protocol will be done by the lab

Issues:


@philippotto philippotto self-assigned this Jan 29, 2019
@philippotto
Copy link
Member Author

@daniel-wer I think it makes sense if you start reviewing this PR. There are some parts which are not spotless (bucket calculation for arbitrary mode and there are some code fragments for an rectangular arbitrary view which are not effective). However, the main part should be ready for review.

Also: Maybe you have an idea on how to estimate the bucket count which is needed for arbitrary view. Right now, I switched back to the very old way of calculating the zoomstep (log(zoomValue / 1.3)), which is not exact.

@philippotto philippotto changed the title [WIP] Support non-quadratic viewports Support non-quadratic viewports Feb 6, 2019
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.

Reviewed a little more than half, will continue tomorrow, looking very good so far :)

Only thing I noticed so far was that the 3D-View is being stretched when it is resized, so the whole scene in the 3D-View is displayed with a wrong aspect ratio.

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.

2nd part of the review, also looking very good! Awesome PR and the result is serious eye candy 👍

I agree that the determination of the number of needed buckets needs some work, maybe we should sit together for an hour, note down the constraints of this problem and think of possible solutions :)

if (x < 0) console.warn("x should be greater than 0. is currently:", x);
if (y < 0) console.warn("y should be greater than 0. is currently:", y);
if (z < 0) console.warn("z should be greater than 0. is currently:", z);
// if (x < 0) console.warn("x should be greater than 0. is currently:", x);
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure, this is not solved, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not really, but it's a bit of a performance killer (the actual log statements). I plan to re-investigate it soon.

app/assets/javascripts/oxalis/store.js Outdated Show resolved Hide resolved
app/assets/javascripts/oxalis/view/input_catcher.js Outdated Show resolved Hide resolved
@philippotto
Copy link
Member Author

I fixed the scalebars and added a tooltip for the whole viewport extent (since this isn't in the dataset info tab view anymore).

image

* use bucket picker for counting (orthogonal mode only)

* adapt arbitrary bucket pickers to new bucket counting approach

* add types for priority queue
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.

Sweet, this went smoother than expected, great design with the enqueueFunction! 🎉
I didn't test manually as the test protocol will be executed anyways.

@@ -149,12 +147,13 @@ export default function determineBucketsForFlight(
const fallbackBuckets = isFallbackAvailable ? traverseFallbackBBox(getBBox(planeBuckets)) : [];
traversedBuckets = traversedBuckets.concat(fallbackBuckets);

let currentCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

The const uniqueBucketMap = new ThreeDMap(); is used for the other two bucket pickers but not this one, is there a reason for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

"Historical growth" 🙈 I forgot about the approach in the flight bucket picker (which was there before) and used the ThreeDMap instead. I'll probably consolidate this when doing the arbitrary-fallback-rendering PR.

* https://github.com/flowtype/flow-typed
*/

declare module "js-priority-queue" {
Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

Copy link
Member Author

Choose a reason for hiding this comment

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

😇

@philippotto philippotto merged commit 347b159 into master Feb 18, 2019
@philippotto philippotto deleted the non-quadratic-viewports branch May 20, 2019 09:12
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.

Implement non-square viewports
2 participants