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

Flightmode improvements #3392

Merged
merged 34 commits into from
Nov 20, 2018
Merged

Flightmode improvements #3392

merged 34 commits into from
Nov 20, 2018

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Oct 23, 2018

This PR implements various improvements to the flight mode:

  • The bucket picker strategy implements a more robust logic to select needed buckets. The gist is that 1% of the to-be-rendered texel position are transformed into bucket space and then looked up. Additionally, for each texel we check whether it borders at another bucket. If so, we add this bucket, as well. This works surprisingly well while having okay performance.
    Consequently, there are (almost?) no missing areas anymore when rendering data in flight mode.
  • I fine-tuned some values for the prefetch strategy for arbitrary mode. I verified that the new values make sense by visualizing the fetched buckets.
  • I also revised the logic which clears buckets which are still in the pull queue, but not relevant anymore. Previously, we just emptied the entire queue except for the entries with the highest priority. There were some cases in which this didn't prove to be very robust (e.g., the "highest priority" was only true for a certain frame but not after that). I replaced the logic by checking in which "tick" the bucket was added. If that tick is older than a certain threshold, the items are cleared.

In addition:

  • This PR also adds the 3D viewport to the arbitrary mode since this proved to be an invaluable tool for debugging. We can certainly hide the view by default, but I'd like to have the possibility to visualize buckets in the 3D scene by changing only a few lines of code.
  • I also added some code which renders the curved plane (by using a vertex shader).

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • test ortho and arbitrary modes

To do

Issues:


@philippotto philippotto self-assigned this Oct 25, 2018
@daniel-wer
Copy link
Member

I did a quick test after seeing the discuss post and encountered a large grey circle that doesn't go away, even after a reload. Maybe you can repro it: https://flightmodeimprovement.webknossos.xyz/annotations/Explorational/5bd040530100009e02ca258b#42046,18988,1725,1,1.24,99.95,84.82,267.14,152

@philippotto
Copy link
Member Author

I did a quick test after seeing the discuss post and encountered a large grey circle that doesn't go away, even after a reload. Maybe you can repro it: https://flightmodeimprovement.webknossos.xyz/annotations/Explorational/5bd040530100009e02ca258b#42046,18988,1725,1,1.24,99.95,84.82,267.14,152

Thanks for testing! The grey circle is outside the bounding box of the existing data. So, it's correct that nothing is rendered. However, the circle should actually be black, since gray indicates "is loading". In my PR, I changed the look up logic a bit, which is why the "display black if buckets are missing inside the bounding box"-logic does not work anymore. I have to figure out a way to fix this again. I'll add a todo item to this PR!

@philippotto
Copy link
Member Author

@daniel-wer I cleaned up the code quite a bit, but it's not really polished yet. Nevertheless, I'd like to get some feedback from you on this PR first. I updated the PR description with more details.

In addition to overall polishing, I'd like to share more code between the TD view implementation of ortho and arbitrary mode. Also, it still need to re-think how we want to render missing data black while still being able to render fallback data, in case the necessary buckets just weren't selected properly. One option which might work is to also pass null buckets to the TextureBucketManager which then just marks a -4 or similar in the look up texture without actually uploading data.

@philippotto
Copy link
Member Author

philippotto commented Nov 6, 2018

I managed to extract the duplicated code for the td controller. I think I'll do the rest (rendering missing data black) tomorrow to get a clear head :)

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.

Milestone PR! 🙏 Awesome work on this, I'm sure the added debugging functionality will be extremely valuable in the future!!!

I added smaller comments here and there, but nothing big.
The flight mode was very performant during my testing and I didn't find any black spots. Nevertheless we should do some more testing using different datasets. We can also think about adding a flight mode screenshot to the screenshot tests (doesn't have to be in this PR).

I think it would be cool to add the TD View to the oblique/flight mode (doesn't need to be visible by default, so maybe "after" the segmentation tab. The curved mesh should probably be hidden by default (right now it's also showing in oblique mode). I'm not sure how hard it would be to display the actual plane as a curved surface (probably harder). It would look cool, but shouldn't be necessary to merge this.

One thing I noticed was that when checking out the branch with an existing layout configuration, the 3D tab in the flight mode was not visible and caused an artifact:

arbitrary-bug

Resetting the layout config, fixed it. This would need to be fixed before merging.

app/assets/javascripts/oxalis/model/sagas/prefetch_saga.js Outdated Show resolved Hide resolved
app/assets/javascripts/oxalis/view/arbitrary_view.js Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@philippotto philippotto changed the title [WIP] Flightmode improvements Flightmode improvements Nov 9, 2018
@philippotto
Copy link
Member Author

@daniel-wer For merging I need an accept for this PR :) Fyi, I created #3446 as a follow-up. Gray vs. black data is rendered in this PR correctly for the ortho mode (which is probably where it's most important), but not yet for flight mode. I think, this should be acceptable though for now.

@philippotto philippotto merged commit 18cf49c into master Nov 20, 2018
philippotto added a commit that referenced this pull request Nov 20, 2018
philippotto added a commit that referenced this pull request Nov 20, 2018
* Revert "also flow-ignore binaryData when using symlinks (#3471)"

This reverts commit 1f60048.

* Revert "Flightmode improvements (#3392)"

This reverts commit 18cf49c.
jfrohnhofen added a commit that referenced this pull request Nov 23, 2018
* origin/master:
  Optimize performance for the list request /api/datasets (#3441)
  add annotation dataset foreign key  (#3482)
  thumbnails: correctly use zoom value if specified (#3487)
  Store Meshes in Postgres (#3367)
  fix alpha return (#3483)
  Added script to apply all new evolutions (#3427)
  Simple fix to speed up dataset gallery (#3480)
  better errors for screenshot tests, fix imports, refresh screenshots (#3479)
  (Backend only) Add project priority to progress report json (#3476)
  Handle missing write access on datastore (#3411)
  Re-introduce "Flightmode improvements"" (#3473)
  Circleci-notify: linkify PR number (#3469)
  Revert "Flightmode improvements" (#3472)
  also flow-ignore binaryData when using symlinks (#3471)
  Flightmode improvements (#3392)
  Circleci custom notification (#3465)
  enable /api/switch cross-organization (#3464)
@normanrz normanrz deleted the flightmode-improvement branch February 20, 2019 16:36
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.

Doublecheck loading speed for msem datasets
2 participants