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

Fix consistency of different lines in solar eclipse maps #3757

Merged
merged 4 commits into from
May 31, 2024

Conversation

10110111
Copy link
Contributor

The original version of the eclipse maps has some ugly artifacts at the locations where the limit lines connect, like rise-set limit vs the penumbra limits, which should touch but not cross, while they instead either cross or don't reach each other (see the screenshots below). This happens because of a few issues with the implementation:

  • In some computations Earth flattening is taken into account in not quite accurate way;
  • In the computation of penumbra limits the fact that a single JD may imply more than one point of the curve results in loss of solutions and thus too short curves.

This PR reimplements some of these functions, trying to avoid any dubious approximations and also avoiding the procedures given in the reference book that don't yield satisfactory results.

There are a few caveats, which you can read in the commit messages, but to me the result looks better than it was before.

Screenshots (solar eclipse on 2025-03-29)

Old

Screenshot_2024-05-30_23-29-27

New

Screenshot_2024-05-30_23-29-40

Old

Screenshot_2024-05-30_23-30-00

New

Screenshot_2024-05-30_23-30-10

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

Test Configuration:

  • Operating system: Ubuntu 20.04
  • Graphics Card: Intel UHD Graphics 620

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

The old version took Earth flattening into account in a not quite
precise way. Particularly, the intersection of the shadow border with
the Earth's rim was computed assuming them both being circular,
apparently with some fixups done afterwards.

This patch implements the more direct computation, which, albeit more
complicated, gets more precise results.
This is less dependent on the way the limit lines are computed (and they
are more complicated as functions of JD than the rise-set lines).
The original implementation ignores that rho != 1 for the flattened
Earth. The new one implements the more precise version accounting for
this inequality.
Previous version didn't quite match the rise-set curve, crossing it or
failing to reach instead of touching. The crossing was likely due to a
bad approximation for taking into account Earth flattening, and the
failure to reach the final point was due to the fact that a single point
in time may generate two points of a single limit curve, while the
iteration in the original implementation (and the one in the book) are
only capable of generating one.

The new implementation goes a very different route: a single equation is
derived for all the limit points at a given time, and these points are
then found by Newton's method of locating roots.

This is not completely robust, so I there may be cases when this fails
due to Newton's method's lack of guarantees of convergence. When this
happens, we'll see discontinuities in the curves. But from what I
tested, these discontinuities, when present (particularly when the lines
naturally have multiple branches), are not critical due to refinement
being done at their ends.
Copy link

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.

@10110111
Copy link
Contributor Author

If you are going to merge this, please don't squash. These commits are big enough to be kept separate.

@alex-w alex-w added this to the 24.2 milestone May 31, 2024
Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gzotti
Copy link
Member

gzotti commented May 31, 2024

Thanks. While the shown cases look indeed much better and seem more correct, note that not all lines meet at expected places, there are sometimes unexpected intersections in the begin/end regions. See the discussions in Meeus, Morsels (1-3?).

@10110111
Copy link
Contributor Author

note that not all lines meet at expected places

Maximum in the horizon and penumbra limits must join by construction (they follow the same equation, just with different constraints). Same for shadow limits and rise-set limit curve, since they both denote the limits for the shadow, just for different stages of shadowing.

Indeed, the rise-set limit intersects the maximum in horizon curve at odd places, which is to be expected since they are quite unrelated geometrically.

Anyway, I followed the geometrical logic while implementing this, rather than try to fit the curves, so this nice result is just a side effect of correctness.

@10110111
Copy link
Contributor Author

10110111 commented May 31, 2024

Here are some examples of oddly-looking intersections that are still correct:

2025-03-29

2025-03-29

2024-10-02

2024-10-02

@10110111 10110111 merged commit 0db8a4f into Stellarium:master May 31, 2024
15 checks passed
@10110111 10110111 deleted the kml-improvements branch May 31, 2024 09:37
@gzotti
Copy link
Member

gzotti commented May 31, 2024

Yes, this is what I meant. I just wanted to start compiling to check that despite lack of time. I also like the good docs! OK, Looks good to me!

@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Jun 9, 2024
Copy link

github-actions bot commented Jun 9, 2024

Hello @10110111!

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 Jun 23, 2024
Copy link

Hello @10110111!

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants