glue-viz / glue-wwt

WorldWideTelescope viewer in glue
BSD 3-Clause "New" or "Revised" License
2 stars 6 forks source link

Only apply longitude shifting conditionally #81

Closed Carifio24 closed 2 years ago

Carifio24 commented 2 years ago

This PR aims to update glue-wwt to account for the changes to the WWT WebGL engine in https://github.com/WorldWideTelescope/wwt-webgl-engine/pull/166 (for the original issue describing the problem, see https://github.com/WorldWideTelescope/wwt-webgl-engine/issues/34). glue-wwt had been shifting the longitudes of layers in 3D modes by 180 degrees to account for this bug. With the WWT engine update, this is now only sometimes necessary, and this PR adds appropriate conditional logic.

The reason for the conditional nature of this fix is based on the relationship between pywwt and the WebGL engine. There are three distinct cases across the pywwt timeline:

glue-wwt requires pywwt >= 0.6, so I don't think we need to worry about older versions. Once the version of the research app in (a future version of) pywwt includes the engine fix, we again won't need to make the longitude adjustment in glue-wwt. I'll make that change when we get there.

I'd appreciate any thoughts from @pkgw about whether there could be anything else worth considering here.

codecov[bot] commented 2 years ago

Codecov Report

Merging #81 (eed6be6) into master (7f4f796) will increase coverage by 0.20%. The diff coverage is 100.00%.

:exclamation: Current head eed6be6 differs from pull request most recent head 4bfd107. Consider uploading reports for the commit 4bfd107 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   74.79%   75.00%   +0.20%     
==========================================
  Files          18       18              
  Lines         853      860       +7     
==========================================
+ Hits          638      645       +7     
  Misses        215      215              
Impacted Files Coverage Δ
glue_wwt/viewer/table_layer.py 79.33% <100.00%> (+0.61%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7f4f796...4bfd107. Read the comment docs.

pkgw commented 2 years ago

I think this makes sense right now. The current release of pywwt is in a bit of a liminal space where the behavior will depend on whether you're using the widget or JupyterLab, but it's not worth the effort to try to handle that correctly — we can just make another release of pywwt that will have consistent behavior everywhere.

pywwt has a set of relatively substantial changes stacked up right now so it will take a bit more work to make a new release, but that will be done soon for the WWT 2022 launch.

Carifio24 commented 2 years ago

Since we know the next version of pywwt will be 0.15 (and use the updated engine), I've added in the logic now to turn the fix off for pywwt >= 0.15.

Also, the tests are failing due to an issue with the tour export tool. This issue is unrelated (but on my to-do list). Looking at the Azure logs, the 3.6 test that is passing is using pywwt==0.9.0, where that issue with the export tool isn't yet present.