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

Catalog HiPS #84

Merged
merged 18 commits into from
Mar 18, 2021
Merged

Catalog HiPS #84

merged 18 commits into from
Mar 18, 2021

Conversation

imbasimba
Copy link
Member

@imbasimba imbasimba commented Feb 21, 2021

All Catalog HiPS entries here need to change: http://www.worldwidetelescope.org/wwtweb/catalog.aspx?W=hips
From: DataSetType="Sky"
To: DataSetType="CatalogHips"

Catalog entries should also be added here: http://web.wwtassets.org/engine/assets/builtin-image-sets.wtml

@pkgw
Copy link
Contributor

pkgw commented Feb 26, 2021

OK, finally getting a chance to take a look at this! Overall, looks great @imbasimba. I have two main questions about the design:

First, am I right that we basically need to add the CatalogHips image set type because we can't reliably guess whether a HiPS dataset consists of imagery or catalog data a priori, and that that is in turn because the .fits file type might apply to either?

Second, I see an appeal to treating the catalog HiPS purely as a "layer" in the same broad way that the SpreadSheetLayer currently works. I can see why it needs an Imageset under the hood to manage the tile loading and rendering and whatnot. Do you think it would be possible to add this Imageset as a private field on the CatalogSpreadSheetLayer class and wire up the layer to implement its rendering through the imageset?

If we were to take such an approach, that would mean that you couldn't load up a catalog HiPS through the standard WWT imageset WTML files — in the same way that you can't currently load up a basic CSV data table that way either. That would definitely have some downsides and require a new approach in the web client UI, so I don't think that it's clearly the better way to go. But as a start I'm wondering whether there are any other important technical limitations to keep in mind.

Once again, great work!

@imbasimba
Copy link
Member Author

  1. Until some of the last commits, determining wether the imageset is a catalog or not was done by checking for the file extension .tsv. This method of checking is fully sufficient for the wwt-webgl-engine. (Even cleaner would be to check the HiPS properties) The problematic part that caused the new CatalogHips image set type to be created was in the wwt-web-client, where no HiPS properties or file extension can be checked. Here I needed some way to make the client call AddCatalogHips instead of setForegroundImage or setBackgroundImage, since we don't want to remove foreground or background. Conceivably, this could be checked inside the engine's setForegroundImageset and setBackgroundImageset instead. But I see no way of also keeping my tiny 'nice touch' (at least nice according to me) feature in the client, where a selected Catalog dataset box does not stay highlighted after a click. To me, it is logical to either highlight the boxes of all active Catalog HiPS or none.
  2. In hindsight this seem like the obvious right way to arrange the responsibilities. I don't follow why this would require a different way of selecting catalog HiPS.

@pkgw
Copy link
Contributor

pkgw commented Feb 26, 2021

I don't follow why this would require a different way of selecting catalog HiPS.

Well, what I am thinking is that right now the WWT web framework isn't set up to save and load layers from XML. WTML files can include imagesets, but not data-table layers (or any of the other special layer types: orbit, 3D models, etc.). In terms of UI, this means that there's no way to create a clickable thumbnail in the web client explorer view that will cause a data-table layer to be added to the view. So if we made catalog HiPS more of a data-table type thing and less of an imageset type thing, I think that would affect the UI work that you've done.

On the other hand, I can imagine an approach where catalog HiPS data are still included in WTML files using the <ImageSet> expression, and the webclient has special logic to take that information and use it to set up a CatalogSpreadSheetLayer.

Does that help clarify things?

If you think that it would be cleaner and not cause problems for your UI work, I think it might be worthwhile to consider iterating the implementation here to be more "layer-like". But the current approach is certainly very manageable, I think.

A side comment: the WWT Windows client defines a separate XML file format, "WWTL", for serializing information about layers. The web client does not implement this format directly. However, WWT tour files also contain serialized layer information, so the web client does have basically all of the pieces it needs to save/load layers from XML. The main UI issue is, as I wrote above, that the Explore UI works with WTML files, which don't provide a way to create a clickable thumbnail that creates a layer. I believe that the first versions of WWT supported WTMLs and foreground/background imagesets, but not the extensible layer system.

@astrojonathan
Copy link
Member

I think we could have a quick tel-con to discuss the options in more detail and then post the outcome here when we are done. There is a lot of history and extensive issues impacting how we treat images vs layers and how we integrate with tours, and especially the impact to planetariums with multiple machines cluster rendering. We do have an imageset layer type and we could do a hybrid of data/image type or use some other mechanisms designed for out community feature that can load layers from thumbnails..

@pkgw
Copy link
Contributor

pkgw commented Mar 5, 2021

@imbasimba Based on our call, how about you take a look at the relevant Windows code and think about whether it suggests any changes to the architecture used here? If it doesn't, so much the better!

@imbasimba
Copy link
Member Author

Will do

@imbasimba
Copy link
Member Author

The overarching architecture is similar in most ways. Most of the differences are the result of differences in the surrounding classes. For example: in the webgl-engine the render call originates from renderContext instead of LayerManagerCore, & in the web projects the SpreadsheetLayer has much better existing support than the votable stuff.

Regarding your second point raised last week, I believe it would be best to keep the existing structure, where the HiPSTiles sends / removes data from the spreadsheetLayer. This is most in line with how the windows client does it (with some differences caused by the above examples).

I’ll see if I can remove the need to add the new DataSetType “CatalogHips” though, since this requirement would only apply for the web implementation.

@pkgw pkgw merged commit c9000e9 into WorldWideTelescope:master Mar 18, 2021
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