glue-viz / glue-wwt

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

Save and restore camera parameters #95

Closed Carifio24 closed 2 months ago

Carifio24 commented 1 year ago

It was mentioned at glue-con that the WWT viewer doesn't save its camera information (RA and dec position, FOV, roll angle) to a session file. This PR aims to fix that issue (with the exception of the viewer's roll angle, which isn't yet exposed to the pywwt widgets - I'll change that upstream and then add that here in a future PR once it's available).

I suspect the reason that this wasn't implemented before now is because this information isn't stored in either of the relevant glue objects (either the viewer state or the viewer itself). The camera position is stored and updated inside of the WWT widget object of the viewer (the _wwt member value). Since the viewer state doesn't have access to the WWT widget, the implementation here stores/reads the camera information in the glue state methods for the viewer itself. For restoring state, this PR accounts for the fact that changing the background image (which here is always a sky imageset) will cause the WWT viewer's mode to match that of the imageset, so we need to avoid that when setting the mode to something other than Sky.

Note that this PR does not fix the (pre-existing) issue where the sky mode does not respect previously set background/foreground imagesets when changing into that mode, making the UI and viewer out of sync. While I've identified the cause of that, it's unrelated and I think may be better helped with an upstream change.

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.09%. Comparing base (a7a015c) to head (2580aad). Report is 5 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #95 +/- ## ========================================== + Coverage 69.04% 70.09% +1.05% ========================================== Files 18 18 Lines 1024 1060 +36 ========================================== + Hits 707 743 +36 Misses 317 317 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pkgw commented 1 year ago

I just merged WorldWideTelescope/wwt-webgl-engine#231, which is presumably related to this.

Carifio24 commented 1 year ago

Yeah, this is one of the primary use cases I had in mind. That change will have to get plumbed through pywwt once we have a new research app release, so I don't think this particular PR needs to wait, but it will be nice to add that in the near future.

Carifio24 commented 1 year ago

@astrofrog I'm not sure why the Windows CI here is failing. All of the tests pass (except one xfail), but it then reports a fail code at the end.

pkgw commented 1 year ago

Is this also going to depend on WorldWideTelescope/pywwt#349?

Carifio24 commented 1 year ago

Yeah, definitely. Assuming we're good with the proposed API changes in that PR, I'll update this to use the roll angle functionality (if available).

dhomeier commented 1 year ago

For the py37 test just rebase once #91 and/or #94 are merged. The other windows failures are exit on STATUS_ACCESS_VIOLATION (0xC0000005); as there have already been problems with using block_until_ready in ba7e9e5, those might still not be resolved?

dhomeier commented 1 year ago

If you need block_until_ready, perhaps it has to be resolved upstream if this can be implemented without QtWidgets.QApplication.process_events.

Carifio24 commented 1 year ago

So in the course of trying to make some progress on this, I realized that we should probably catch a ViewerNotAvailableError in the WWT viewer's __gluestate__ method - we don't want to prevent a user from being able to save their session if the WWT viewer stops responding (the exported session won't have camera parameters, but that's much better than no session at all). This has the effect of making the CI tests pass (except for the Windows/Python 3.7 test), albeit in a not-particularly-satisfying way. (I suppose one could optimistically say that this tests the fallback case :) ).

Otherwise I'm not sure yet how to test this on Windows without block_until_ready, or something essentially equivalent)- getting the camera parameters when serializing requires the WWT viewer to be ready to accept messages. Note that this isn't a problem when deserializing - if the WWT viewer isn't ready to start accepting messages yet, the center_on_coordinates call will be put into a message queue and will be dispatched whenever the app is ready.

Carifio24 commented 7 months ago

I rebased to pick up some new changes. The new CI failures are due to the import issue addressed in #102.

astrofrog commented 3 months ago

I've now rebased this to see if the CI failures are gone

Carifio24 commented 3 months ago

@astrofrog Looks like the failures are all gone!