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

Save and restore camera parameters #95

Merged
merged 6 commits into from
May 4, 2024

Conversation

Carifio24
Copy link
Member

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
Copy link

codecov bot commented Feb 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pkgw
Copy link
Contributor

pkgw commented Feb 21, 2023

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

@Carifio24
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Contributor

pkgw commented Mar 6, 2023

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

@Carifio24
Copy link
Member Author

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
Copy link
Contributor

dhomeier commented Mar 6, 2023

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
Copy link
Contributor

dhomeier commented Mar 6, 2023

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

@Carifio24
Copy link
Member Author

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
Copy link
Member Author

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

@astrofrog
Copy link
Member

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

@Carifio24
Copy link
Member Author

Carifio24 commented Mar 21, 2024

@astrofrog Looks like the failures are all gone!

@Carifio24 Carifio24 merged commit a16c371 into glue-viz:main May 4, 2024
21 checks passed
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