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

Planet RTS #1852

Merged
merged 35 commits into from
Sep 21, 2021
Merged

Planet RTS #1852

merged 35 commits into from
Sep 21, 2021

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Aug 28, 2021

Description

This should solve the inaccurate rise/set times given for planets, esp. the Moon. It is based on the method given by Meeus but with some optimisations.

Fixes #554

Screenshots (if appropriate):

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

How Has This Been Tested?

Observe Moon (or Sun, or planet) on screen, see behaviour and times.
I have still to make tests around reaching circumpolarity and other funny things.

There is still the problem that sometimes rise time for the next day or setting time for the previous day is found.

Still not fully tested: behaviour on other planets.

The fine corrections are also applied to StelObject.

Test Configuration:

  • Operating system: Win10 21H1
  • Graphics Card: Nvidia 960M (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
  • 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 enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash labels Aug 28, 2021
@gzotti gzotti added this to the 0.21.2 milestone Aug 28, 2021
@gzotti gzotti self-assigned this Aug 28, 2021
@todo

This comment has been minimized.

@github-actions github-actions bot requested a review from alex-w August 28, 2021 21:42
@github-actions

This comment has been minimized.

@github-actions
Copy link

Hello @gzotti! Thank you for this enhancement.

@gzotti gzotti marked this pull request as draft August 28, 2021 22:52
@lgtm-com

This comment has been minimized.

@alex-w
Copy link
Member

alex-w commented Aug 29, 2021

By the fact getRTSTime and computeRTSTime methods have same behavior and I think we can move code from computeRTSTime into getRTSTime method and removed first one. The second problem: to compute RTS time of the Sun for twilight we need change the height of celestial body (+ users ask this feature for AstroCalc functions) - so, maybe add new one parameter for getRTSTime method with default value?

@alex-w
Copy link
Member

alex-w commented Aug 29, 2021

Maybe exclude DebugAid flags from Release builds?

DebugAid should be available in Debug mode only

@alex-w
Copy link
Member

alex-w commented Aug 29, 2021

A quick test: RTS time for planets give very strange results - like a double application of the time zone shift

@gzotti
Copy link
Member Author

gzotti commented Aug 29, 2021

re1: One is public, the other is not. But that's easy to decide...
re2: Yes, target altitude should be possible. An advanced solution (possible around 0.14, maybe 2022...) would be taking the landscape horizon into account. This may be possible with some binary search in hour angle. But not in this branch.
re3: DebugAid: this comment is old, just the bot found it... I could exclude it from real STELLARIUM_RELEASE_BUILDs, but not Qt's "release" mode. However, these in-screen data displays are targeted for developing particular features, so they have to be commented away (or deleted) at end of branch development.
re4: (strange) I had hoped the interpolation wraparound zone fix would remedy that. Need another look.

I tried the "goto next setting" etc. functions on hotkey which are really nice in principle. While this version is usually much better (esp. Moon), the case where the solution provides times for the previous or next day needs to be discovered and solved. There are however 2 special cases: Sidereal day is 23h56m, so a star may rise twice on a single calendar day. And with moonrise/-set ~25 hours apart, the Moon may not rise/set on some particular date. If anybody has a worked-out solution or a better algorithm than Meeus (how dare you!), this could really help. We actually need to "find nearest event" (maybe with a way to flag it "will occur on next day only") and "goto next moonrise" (not "find moonrise date from algorithm, then set current date with that time").

@alex-w
Copy link
Member

alex-w commented Aug 29, 2021

Still untested: behaviour on other planets.

Well, the calendars and duration of the day on extraterrestrial locations is separate big problem. :(

@alex-w
Copy link
Member

alex-w commented Aug 29, 2021

I tried the "goto next setting" etc. functions on hotkey which are really nice in principle. While this version is usually much better (esp. Moon), the case where the solution provides times for the previous or next day needs to be discovered and solved. There are however 2 special cases: Sidereal day is 23h56m, so a star may rise twice on a single calendar day. And with moonrise/-set ~25 hours apart, the Moon may not rise/set on some particular date. If anybody has a worked-out solution or a better algorithm than Meeus (how dare you!), this could really help. We actually need to "find nearest event" (maybe with a way to flag it "will occur on next day only") and "goto next moonrise" (not "find moonrise date from algorithm, then set current date with that time").

Well, the special cases for the Sun on high latitudes also need to remember. :)

@gzotti
Copy link
Member Author

gzotti commented Aug 29, 2021

Yes, indeed. This PR is currently only a partial fix. I think I should invent a different algorithm that starts with the current hour angle, find nearest transit, then estimates for rise/set from semidiurnal arc, with final solutions from binary search or those correction terms given by Meeus. And if sun rises above horizon in Polar summer, it will have a rise and transit time, but no more setting this day. The "next setting" call would switch to 24 o'clock of that day.

Not sure, do we accept this (maybe some more fixes?) for 0.21.2 and see to devise sth more complete ~ end of year?

@alex-w
Copy link
Member

alex-w commented Aug 29, 2021

I think we may postpone implementing the special cases to other release.

@alex-w

This comment has been minimized.

@gzotti

This comment has been minimized.

@alex-w

This comment has been minimized.

@gzotti gzotti marked this pull request as ready for review August 29, 2021 20:11
@gzotti

This comment has been minimized.

@alex-w

This comment has been minimized.

@gzotti

This comment has been minimized.

@gzotti gzotti marked this pull request as draft August 29, 2021 21:11
@alex-w

This comment has been minimized.

@gzotti

This comment has been minimized.

@alex-w alex-w self-requested a review August 31, 2021 04:15
@todo

This comment has been minimized.

@todo

This comment has been minimized.

@gzotti
Copy link
Member Author

gzotti commented Sep 21, 2021

@alex-w How shall we report circumpolarity to the InfoMap? Return transit time (or lower culmination), or use a verbal annotation?

@alex-w
Copy link
Member

alex-w commented Sep 21, 2021

Probably verbal annotation will be enough here

@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2021

This pull request introduces 1 alert when merging c3bf117 into 0647452 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

@github-actions
Copy link

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

@Atque
Copy link
Contributor

Atque commented Sep 22, 2021

Uh, I'm not sure if I'm missing something here, but selecting the Moon will give different rise/transit/set values over different times of the same day. Is that intentional?

@alex-w
Copy link
Member

alex-w commented Sep 22, 2021

Uh, I'm not sure if I'm missing something here, but selecting the Moon will give different rise/transit/set values over different times of the same day. Is that intentional?

No, this is wrong

@gzotti
Copy link
Member Author

gzotti commented Sep 22, 2021

There is a slight change in time by a few minutes if you are using topographic correction. For geocentric computation is should be stable. Given atmospheric vagaries the difference should be negligible. We accept better-working solutions in code :-)

@github-actions
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
enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash
Development

Successfully merging this pull request may close these issues.

Objects' raise/set time changes during night
3 participants