search

cosmoscout / cosmoscout-vr

:milky_way: A virtual universe which lets you explore, analyze and present huge planetary datasets and large simulation data in real-time.
Other
354 stars 27 forks source link

Feature/wms overlays #237

Closed TheFridge76 closed 3 years ago

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 689344976

Changes Missing Coverage Covered Lines Changed/Added Lines %
plugins/csp-wms-overlays/src/WebMapService.hpp 0 2 0.0%
plugins/csp-wms-overlays/src/Plugin.hpp 0 3 0.0%
plugins/csp-wms-overlays/src/WebMapException.hpp 0 3 0.0%
plugins/csp-wms-overlays/src/WebMapLayer.hpp 0 3 0.0%
plugins/csp-wms-overlays/src/WebMapTextureLoader.hpp 0 3 0.0%
plugins/csp-wms-overlays/src/logger.cpp 1 4 25.0%
plugins/csp-wms-overlays/src/utils.hpp 0 28 0.0%
plugins/csp-wms-overlays/src/WebMapLayer.cpp 0 82 0.0%
plugins/csp-wms-overlays/src/WebMapException.cpp 0 97 0.0%
plugins/csp-wms-overlays/src/utils.cpp 1 135 0.74%
<!-- Total: 5 1462 0.34% -->
Files with Coverage Reduction New Missed Lines %
src/cs-utils/DefaultProperty.hpp 5 26.32%
<!-- Total: 5 -->
Totals Coverage Status
Change from base Build 686683138: -0.3%
Covered Lines: 546
Relevant Lines: 15909
💛 - Coveralls
Schneegans commented 3 years ago

Thank you very much for this awesome addition! Could you please merge the current develop? You'll need to rebuild the externals. Also the CEF version changed, so maybe you have to delete build\windows-externals-Release\cef.

Schneegans commented 3 years ago

You may also add this plugin to the list of plugins in the README.md :smile:

Schneegans commented 3 years ago

I just tested this on Linux and all seems to work nicely. I'll look at the code hopefully later this week - here are some initial observations / suggestions:

Thank you once more for this great addition!

TheFridge76 commented 3 years ago

Thanks for your feedback on the plugin! To address some of your suggestions:

Schneegans commented 3 years ago

Have you checked the Downloader class? It already supports parallel downloads and some progress output. It's used here: https://github.com/cosmoscout/cosmoscout-vr/blob/develop/src/cosmoscout/Application.cpp#L330

Regarding the cache: I think we should just change the default configuration and moving it to share/cache seems reasonable!

Schneegans commented 3 years ago

It also appears to me that the capabilities cache is outdated quite frequently. If you download the capabilities in parallel we could maybe try not to cache this? Most awesome solution would be to parse the capabilities document directly after downloading directly in the download thread. Only once this is finished, the UI should be filled with the newly available data sets. This way the plugin loading would be much faster!

TheFridge76 commented 3 years ago

The capability cache does bring some load time improvements at least when frequently restarting CosmoScout, so I don't think we should stop caching the capabilities. The NASA NEO server unfortunately does not specify a sequence number, so its capabilites have to be downloaded each time the plugin is loaded, but both the NASA SVS server and the DWD server profited from using the cache in a short test I did:

Server without Cache with Cache
NEO 1.7-2.9s -
SVS 4.4-5s 1,7-2s
DWD 5.9-6s 0.5-0.7s

I'm sorry, but I didn't quite get the second part of your comment: Do you suggest to proceed with the initialization of CosmoScout while the capabilities are downloaded and parsed in the background? If we added more work to be done to the download threads, this would also mean that the Downloader class can't be used anymore, right?

Schneegans commented 3 years ago

I just tested this with the DWD server. The update sequence number is not changed when the time ranges change. It still reports 13431 as it did this morning. However, the valid time dimensions for various layers have changed in the mean time. So for example, the "Niederschlag" layer only shows the most recent data if I delete the cache by hand.

And yes, you're right using the Downloader wouldn't be straight-forward anymore. You would either have to copy the relevant code from this class (it's not much. actually just a combination of the Threadpool and cs::utils::filesystem::downloadFile()) or you launch a new parsing thread once the download finished...

What do you think?

TheFridge76 commented 3 years ago

Ah, right! I guess with DWD not updating the updateSequence often enough the caching would only be useful for the SVS server. In this case I think it makes sense to remove the capability caching for now.

I'll work on implementing the parallel download in the next days. I think using Threadpool it should be quite straight-forward. However, as I'm downloading the file directly to in-memory streams I will keep using the manual curlpp request setup instead of cs::utils::filesystem::downloadFile() (Maybe a method for downloading files to any std::ostream should be added to cs::utils at some point?)

Schneegans commented 3 years ago

Currently https://gitlab.com/libtiff/libtiff is not available. See https://github.com/cosmoscout/cosmoscout-vr/pull/244.

Schneegans commented 3 years ago

I played around a bit and it seems pretty stable now. I applied the automatic fixes from clang-format and clang-tidy. The only thing I would suggest is to remove "https://svs.gsfc.nasa.gov/cgi-bin/wms" from the default config as it causes several warnings during startup which is not-so-good for the user experience.