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

Allow position to be set by WCS for TOAST imagesets #50

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

imbasimba
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #50 (c9be016) into master (9518719) will decrease coverage by 0.62%.
The diff coverage is 80.22%.

❗ Current head c9be016 differs from pull request most recent head cb88cb8. Consider uploading reports for the commit cb88cb8 to get more accurate results

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
- Coverage   89.41%   88.79%   -0.63%     
==========================================
  Files          10       10              
  Lines        1663     1704      +41     
==========================================
+ Hits         1487     1513      +26     
- Misses        176      191      +15     
Impacted Files Coverage Δ
wwt_data_formats/server.py 68.79% <45.45%> (-5.61%) ⬇️
wwt_data_formats/cli.py 87.50% <83.33%> (ø)
wwt_data_formats/imageset.py 93.25% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pkgw
Copy link
Contributor

pkgw commented Aug 18, 2022

Can you explain a bit more about the motivation for this? Is this so that we can compute useful Place locations for images that aren't all-sky but are big enough to need TOASTing? When in TOAST mode, do the imageset-level settings like OffsetX, Rotation, etc. even do anything?

@imbasimba
Copy link
Member Author

Of course! This change is indeed for the TOASTing of images that aren't all-sky, in particular WorldWideTelescope/toasty#86. In these cases it seems reasonable to goto the location of the imageset. When adding any kind of imageset, including TOAST, the engine does read Center, Offset, Scale, and Size to determine the camera position. It only reads these values if the goto parameter is true, which is always is for layers added through pywwt. Rotation is not read though.

@pkgw
Copy link
Contributor

pkgw commented Aug 19, 2022

Ah, yes. Well, for TOAST imagesets I think it would be good to simplify and normalize the WTML metadata as much as possible. I think we should force OffsetX = OffsetY = 0, and maybe there is a sensible normalization to apply to the Size/Scale parameters?

It would also be good to add a comment to the function here explaining why it's worthwhile to set these parameters on TOAST images even if they don't "need" them for basic display.

…) are no longer set when setting position from WCS
@imbasimba
Copy link
Member Author

Aha, we are talking about just the properties close inside that if/else statement. Indeed you are 100% correct. In fact, not setting these properties for TOAST (i.e. offset = 0 etc.) results in a more correct pan by the engine. My bad :)

@pkgw pkgw merged commit 9bc33ef into WorldWideTelescope:master Aug 22, 2022
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.

2 participants