-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
I just merged WorldWideTelescope/wwt-webgl-engine#231, which is presumably related to this. |
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. |
54f7bd1
to
e1443ad
Compare
@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. |
Is this also going to depend on WorldWideTelescope/pywwt#349? |
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). |
For the py37 test just rebase once #91 and/or #94 are merged. |
If you need |
e1443ad
to
b8e8964
Compare
So in the course of trying to make some progress on this, I realized that we should probably catch a Otherwise I'm not sure yet how to test this on Windows without |
4107043
to
12966c7
Compare
I rebased to pick up some new changes. The new CI failures are due to the import issue addressed in #102. |
6e20a38
to
2580aad
Compare
I've now rebased this to see if the CI failures are gone |
@astrofrog Looks like the failures are all gone! |
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.