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

S3 support no preload patch #44

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Conversation

cgoina
Copy link
Contributor

@cgoina cgoina commented Jan 10, 2025

This is a patch that would no start preloading tiles as soon as the Horta Control panel is open. Currently it starts pre-loading 2-D tiles even though we currently use it only for 3-D tiles. The patch is hacky and I don't know if it may break or not anything so please review it carefully. I tested it on my dev workstation and it seems to work. I am in no rush to merge this PR but if it works it can be a good optimization.

@cgoina cgoina requested review from krokicki and olbris January 10, 2025 17:41
@cgoina cgoina requested a review from porterbot January 10, 2025 17:41
Cristian Goina and others added 3 commits January 10, 2025 14:25
@olbris
Copy link
Contributor

olbris commented Jan 13, 2025

  • I was able to build and run the code, against the dev server, without problems.
  • I loaded some old sample I have around that have 2d and 3d tiles. Everything appeared to behave normally in a modest amount of navigating around.
  • I looked over the code, and at a very high level, it looks sane. I admit, though, I am not 100% sure I fully understand the current 2d tile loading flow.
  • Conceptually, eliminating this preloading for 2d tiles makes a lot of sense. For one, 3d tiles are very much the preferred mode, and locally, our users almost never load 2d tiles. Second, there are a number of things you can do in the Horta Control Center without loading any imagery (eg, importing/exporting neurons, examining their annotations). There's no need for loading image data at this point.
  • In the more distant past, the control center and the 2d view were one and the same, which is why the loading was connected in that way.

Let me know if there is more specific testing that would be useful.

@porterbot
Copy link
Collaborator

Just double-checking the commit for 6af37e7, did you basically just move the prefetching logic to only start working if the SharedVolume is loaded (3D view is started)? I don't see the commit for restraining the 2D unless the 2D view is loaded. Which commit would it be in?

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.

3 participants