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

Expose grid text and additional constellation settings #354

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

Carifio24
Copy link
Member

This PR exposes the ability to toggle the grid text for the four grids that are exposed (equatorial, ecliptic, alt/az, and galactic), as well as the ability to toggle constellation labels and pictures. One intended use case is glue-wwt (i.e. as an extension to what's currently in glue-viz/glue-wwt#97).

The implementation for some of these properties already existed but were commented out. There are some notes about the constellation properties in the commit where they were commented out here. In my testing both of these properties work fine (and I don't see any errors coming up while scrolling with the constellation pictures enabled), so my suspicion is that these comments relate to an older (pre-research app) pywwt implementation. Regardless, all of these properties work fine in my local testing.

I decided to name the member value corresponding to the equatorial grid text as just grid_text for consistency with the grid member value. It's worth noting that the text for a grid won't draw unless the grid is displayed - i.e. the value of alt_az_text has no effect unless alt_az_grid is True. This is consistent with the internal engine settings.

If we're okay with the implementation here, I'll update the image testing to take these settings into account.

@pkgw
Copy link
Contributor

pkgw commented Jun 29, 2023

Yeah, those remarks are from long enough ago that I wouldn't worry about them.

Hopefully, changing things here shouldn't require any of the test images to be updated, unless you're talking about adding new test cases to demonstrate the feature. Preferably none of the defaults relating to showing text would change from their current (implicit) status.

The current failure I'm seeing seems to be about updating some internal list of settings:

>       assert len(settings) == len(STARDARD_WWT_SETTINGS) + len(expected_3d_settings)
E       AssertionError: assert 33 == (19 + 8)

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #354 (fdf616f) into master (c5c063a) will increase coverage by 0.09%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
+ Coverage   56.39%   56.48%   +0.09%     
==========================================
  Files          25       25              
  Lines        2848     2854       +6     
==========================================
+ Hits         1606     1612       +6     
  Misses       1242     1242              
Impacted Files Coverage Δ
pywwt/core.py 79.41% <100.00%> (+0.28%) ⬆️

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

@Carifio24
Copy link
Member Author

Okay, I think this should be all set now. The only thing to note is that I increased the wait time for the new test step that checks some of the constellation settings - loading the constellation pictures needed some extra time, at least when testing.

@pkgw pkgw merged commit 3665ecb into WorldWideTelescope:master Jun 30, 2023
@pkgw
Copy link
Contributor

pkgw commented Jun 30, 2023

Great, thanks!

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