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

Simplify gridline intersect callbacks #3792

Merged
merged 5 commits into from
Sep 8, 2024
Merged

Simplify gridline intersect callbacks #3792

merged 5 commits into from
Sep 8, 2024

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Jul 2, 2024

Description

Issue #3688 describes a tiny problem in the gridline manager's labeling of equatorial coordinates.
I tried to fix and saw quite a bit of elaborate code was not even required. Or was it?

During development of this branch I will try to remove that possible bloat code also for the other grids.

Fixes #3688 (issue)

Screenshots (if appropriate):

see #3688

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • Housekeeping

How Has This Been Tested?

Test Configuration:

  • Operating system: Win10, Win11
  • Graphics Card: Geforce cards (irrelevant)

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (header file)
  • I have updated the respective chapter in the Stellarium User Guide
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@gzotti gzotti added this to the 24.3 milestone Jul 2, 2024
@gzotti gzotti self-assigned this Jul 2, 2024
@github-actions github-actions bot requested review from 10110111 and alex-w July 2, 2024 14:10
Copy link

github-actions bot commented Jul 2, 2024

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files.

@gzotti gzotti added bug Something likely wrong in the code enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash labels Jul 2, 2024
@gzotti gzotti force-pushed the fix-gridline-labels branch from d8a6060 to 1a6dd19 Compare July 2, 2024 14:15
@10110111
Copy link
Contributor

10110111 commented Jul 2, 2024

What is lon supposed to mean? The longitude of the observer on the planet? Why should it affect the label?

@gzotti
Copy link
Member Author

gzotti commented Jul 2, 2024

lon/lat (longitude/latitude) seem to be the interface points of screen intersection (?) in general spherical names. I have not gone further today, but at least in the equatorial grid they seem not to be required. Or I am missing something important.

@alex-w
Copy link
Member

alex-w commented Jul 2, 2024

OK, all tests from my side are passed

@gzotti
Copy link
Member Author

gzotti commented Jul 2, 2024

Brr, and I just discovered errors. Switch to decimal degrees! Not sure what's happening.

@alex-w
Copy link
Member

alex-w commented Jul 2, 2024

Brr, and I just discovered errors. Switch to decimal degrees! Not sure what's happening.

Oops... hour angle should be always in hours - this bug exists in the current release too

@gzotti
Copy link
Member Author

gzotti commented Jul 2, 2024

Not necessarily. It's really user's choice. See SHA for navigators.

@alex-w
Copy link
Member

alex-w commented Jul 2, 2024

Coordinates in infobox and in grids should be consistent

gzotti added 2 commits July 3, 2024 21:52
- it was possible to remove a full branch!
- Fixed HA in lower culmination sometimes still toggles 12/0h :-(
- All trouble around a tiny little -8e-15 artifact!
@gzotti gzotti marked this pull request as ready for review September 8, 2024 11:16
@gzotti
Copy link
Member Author

gzotti commented Sep 8, 2024

please test with all grids and settings (decimal numbers etc.)

@alex-w
Copy link
Member

alex-w commented Sep 8, 2024

Hour angles should be measured in hours by definition, not in degrees

P.S. Of course in the current master I see same problem and this is bug IMHO

@gzotti
Copy link
Member Author

gzotti commented Sep 8, 2024

Do you mean decimal hours? I am not aware of any use for that. Navigators and maybe others (surveyors?) may prefer decimal degrees for hour angles. This branch/fix should however only change the previous behaviour of a spurious 0 for 12h/180°. Given the simplifications, I hope I did not break other grids now. (I guess a lot of bloat was to mitigate the original problem)

@gzotti gzotti merged commit 2c2b03d into master Sep 8, 2024
25 of 26 checks passed
@gzotti gzotti deleted the fix-gridline-labels branch September 8, 2024 14:26
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Sep 8, 2024
Copy link

github-actions bot commented Sep 8, 2024

Hello @gzotti!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Sep 22, 2024
Copy link

Hello @gzotti!

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something likely wrong in the code enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash
Development

Successfully merging this pull request may close these issues.

Wrongly display of Right Ascension 12h in Equatorial System (RA-dec)
3 participants