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 planetocentric rectangular coordinates #2244

Merged
merged 10 commits into from
Feb 9, 2022
Merged

Fix planetocentric rectangular coordinates #2244

merged 10 commits into from
Feb 9, 2022

Conversation

worachate001
Copy link
Member

Description

As mentioned in #2237 Sun & Moon are not aligned at greatest solar eclipse. I suspect that something is wrong with observer's coordinates on Earth and it's right.

There is a bug in planetocentric rectangular coordinate calculations that give wrong values of rhoCosPhiPrime and rhoSinPhiPrime. After fixing this issue, the Sun and Moon are aligned perfectly for all central solar eclipses! This should also improve the accuracy of topocentric coordinates for all objects.

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

Test Configuration:

  • Operating system: Ubuntu 20.04

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

@github-actions
Copy link

github-actions bot commented Feb 6, 2022

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

src/gui/AstroCalcDialog.cpp Outdated Show resolved Hide resolved
src/core/modules/Planet.cpp Outdated Show resolved Hide resolved
src/core/modules/Planet.cpp Outdated Show resolved Hide resolved
src/gui/AstroCalcDialog.cpp Outdated Show resolved Hide resolved
src/gui/AstroCalcDialog.cpp Outdated Show resolved Hide resolved
@worachate001
Copy link
Member Author

@gzotti Thanks.

src/gui/AstroCalcDialog.cpp Outdated Show resolved Hide resolved
src/gui/AstroCalcDialog.cpp Outdated Show resolved Hide resolved
@alex-w
Copy link
Member

alex-w commented Feb 6, 2022

Part of code in Planes.cpp and AstroCalcDialog.cpp files are identical - why not reuse it?

@worachate001
Copy link
Member Author

Part of code in Planes.cpp and AstroCalcDialog.cpp files are identical - why not reuse it?

Yes. Thinking about that, but I need a rest soon, it's almost midnight here.

@worachate001
Copy link
Member Author

worachate001 commented Feb 7, 2022

Update: the revision is going well until I found a new bug in AstroCalc/Eclipses. Trying to fix it before pushing this commit.
Update2: the new bug has been fixed. It seems to work well so far without any strange looking numbers.

@worachate001 worachate001 marked this pull request as draft February 8, 2022 00:41
@worachate001 worachate001 marked this pull request as ready for review February 9, 2022 05:09
@github-actions github-actions bot requested a review from gzotti February 9, 2022 05:10
@alex-w
Copy link
Member

alex-w commented Feb 9, 2022

Why not use official term instead of "Sun/Moon diameter ratio"?

@worachate001
Copy link
Member Author

worachate001 commented Feb 9, 2022

Why not use official term instead of "Sun/Moon diameter ratio"?

magnitude-ratio-tse

This is an example of total eclipse when you're standing near the limit of umbra.

We already have 'Eclipse magnitude' below 'Eclipse obscuration' - eclipse magnitude here indicates how close you are to the northern or southern limit of umbra. It will be close to 1.0 if you are just inside the umbra.

Moon/Sun diameter ratio is different value, if we use the same term, it will be contradict to Eclipse magnitude above it. I think users may be confused of having two magnitudes at the same time.

Moon/Sun diameter ratio is central eclipse magnitude in NASA web site. If we use this value in the place below eclipse obscuration, it will cause confusion when an eclipse reaches magnitude 1.0 at 2nd contact and jump to 1.078 in this case.

@worachate001
Copy link
Member Author

worachate001 commented Feb 9, 2022

Example of magnitude at greatest eclipse from two sources that show different value for the same event (look at 1955 Jun 20)

NASA
Above from https://eclipse.gsfc.nasa.gov/JSEX/JSEX-AU.html
and
https://astro.ukho.gov.uk/eclipse/0311955/Bangkok_Thailand_1955Jun20.png

@gzotti
Copy link
Member

gzotti commented Feb 9, 2022

Probably we should re-arrange the lines, though:

  • Eclipse magnitude
  • Eclipse obscuration
  • Moon/Sun diameter ratio
  • Central eclipse duration
  • ...

By this, we would have the "coverage" items first, then the geographic data.

src/core/modules/Planet.cpp Outdated Show resolved Hide resolved
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 for improving!

src/core/modules/Planet.cpp Outdated Show resolved Hide resolved
@alex-w alex-w added this to the 0.22.0 milestone Feb 9, 2022
@alex-w
Copy link
Member

alex-w commented Feb 9, 2022

@gzotti the PR is good for me, please merge it if he is good for you too

@gzotti gzotti merged commit 238e7ef into Stellarium:master Feb 9, 2022
@worachate001 worachate001 deleted the fix-planetocentric branch February 10, 2022 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants