Skip to content

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
@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.

4 participants