Orbit epoch

[gzotti]

Description

This finally adds the epoch information to the KeplerOrbit object.

For some time now we have avoided showing objects (minor bodies) when their orbital elements were outdated.
However, it was always assumed that the orbital elements' epoch was equal to perihelion date, which is not necessarily the case.
This lead to unclear situations like wrong cut-away dates for asteroids with several years' orbital period.

Orbital elements should in fact be updated regularly. I also added a warning in the InfoString when date is outside the "good" range. It may be discussed if the orbit_good evaluation or Planet::hasValidPositionalData() should be extended to allow differentiation between elements "good enough to show closed orbit" (years) and "good enough to find in telescope" (months).

Fixes #2000 (issue)

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?

Test Configuration:

• Operating system: Windows 10 (irrelevant)
• Graphics Card: (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

[todo]

does that default to sth meaningful? Do we need it at all?

stellarium/guide/app_ssystem_ini.tex

Lines 363 to 368 in 7c21f77

	\item[orbit\_visualization\_period] [days] %TODO does that default to sth meaningful? Do we need it at all?
	\end{description}
	The other parameters are like those for the major planets.
	\subsubsection{Minor Planet section}
	\label{sec:ssystem.ini:MinorPlanet}

---

This comment was generated by todo based on a TODO comment in 7c21f77 in #2027. cc @Stellarium.

[github-actions]

Hello @gzotti! Thank you for this enhancement.

[github-actions]

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

Files matching src/**/*.cpp:

• Are possibly unused includes removed?

Files matching guide/**:

• Did you remember to update screenshots to match new updates?
• Did you remember to grammar check in changed part of documentation?

This is an automatically generated QA checklist based on modified files

[alex-w]

Please update comet_orbit strings in default ssystem_minor.ini file according to Stellarium User Guide.

Please move "note" line to the end of list.

[gzotti]

I would still leave "comet_orbit" for compatibility with earlier versions. In 0.21+ we need no explicit orbit_func at all, but understand kepler_orbit and comet_orbit. But I often run earlier versions for comparison.

The NOTE in infostring is just below the coordinates. Do you mean I should move that to the end of the infostring?

[alex-w]

I would still leave "comet_orbit" for compatibility with earlier versions. In 0.21+ we need no explicit orbit_func at all, but understand kepler_orbit and comet_orbit. But I often run earlier versions for comparison.

But I said about default file, which will be used in newly installation only. Existed ssystem_minor.ini file will be used as before with comet_orbit string in backward compatible mode.

The NOTE in infostring is just below the coordinates. Do you mean I should move that to the end of the infostring?

Yes, at end of the infosting - this line will be more visible here.

[alex-w]

does that default to sth meaningful? Do we need it at all?

Yes, we need it to some moons and probably to some asteroids/comets

[gzotti]

It's an oldish comment. We could just use one revolution as default for closed orbits. I think in previous years this and orbit_good was sometimes mixed up a bit. Probably we should use this to plot comet orbit around perihelion, and report orbit_good around epoch? Or still plot orbit with this value around epoch as well? These are things to still think over a bit.

[alex-w]

It's an oldish comment. We could just use one revolution as default for closed orbits. I think in previous years this and orbit_good was sometimes mixed up a bit. Probably we should use this to plot comet orbit around perihelion, and report orbit_good around epoch? Or still plot orbit with this value around epoch as well? These are things to still think over a bit.

It’s all not for this PR

[alex-w]

Thank you very much!

[gzotti]

Don't merge yet. I may want to check a few more things.

[alex-w]

OK, please do it yourself

[axd1967]

@gzotti There exists 1P/Halley (2061) - does it fit the current algorithm? 2061 is more than 3 years (approx 1000 days) in the future. It has orbit_good = 20000

[axd1967]

@gzotti have a look at axd1967@984084d

• I really would avoid 1000 but put this in a constant
• 0 and -1 beg to be cast in enums. but i somehow have a feeling that 0 could lead to bugs

(you can fetch that branch to make work easier)

[gzotti]

The element set has an epoch of 1994. 20.000 days is definitely uselessly much for assume anything accurate, but it might be good enough for plotting an orbit.

I am just adding a distinction about whether the orbital data is "good enough for plotting an orbit" (defaults to 1/2 sidereal period or orbit_good) or "good enough to be sufficient for setting a telescope". The latter has higher requirements and uses the smaller of (epoch+/-orbit_good) and (epoch+/-1 year) to suggest updating elements after 1 year.

[axd1967]

I am just adding a distinction about whether the orbital data is "good enough for plotting an orbit" (defaults to 1/2 sidereal period or orbit_good) or "good enough to be sufficient for setting a telescope". The latter has higher requirements and uses the smaller of (epoch+/-orbit_good) and (epoch+/-1 year) to suggest updating elements after 1 year.

consider somehow coding these two requirements more explicitly? I proposed two enums to get rid of the magic 0 and -1, but these are probably unrelated.
but explicitly coding these requirements will make the code so much more clear.
I'd suggest to use 3*365 rather than 1000 days - even if both values still are magic and probably not 100% justifiable, it's just a bit more human-readable. even 20_000 is less readable than 54+ years...

[lgtm-com]

This pull request introduces 1 alert when merging 48d05ca into cf52aa7 - view on LGTM.com

new alerts:

• 1 for Missing return statement

[alex-w]

The changes is good for me.

[todo]

Add state vector equations from Heafner to create orbits from position+velocity,

stellarium/src/core/modules/Orbit.hpp

Lines 65 to 70 in 34b682f

	//! @todo Add state vector equations from Heafner to create orbits from position+velocity,
	//! or change orbits from velocity changes.
	class KeplerOrbit : public Orbit {
	public:
	//! Constructor.
	//! @param epochJDE JDE epoch of orbital elements.

---

This comment was generated by todo based on a todo comment in 34b682f in #2027. cc @Stellarium.

[lgtm-com]

This pull request introduces 1 alert when merging 34b682f into cf52aa7 - view on LGTM.com

new alerts:

• 1 for Missing return statement

[gzotti]

(According to my understanding this is a false positive. All cases in the switch are handled, and whatever is not handled by some orbit object returns false. )

[axd1967]

(According to my understanding this is a false positive. All cases in the switch are handled, and whatever is not handled by some orbit object returns false. )

one day Murphy will knock on that door...

a switch should almost always include a default, even if never used. it's part of defensive programming.

and it's cheap to add a default case; just add a warning (that should never appear; so how about as ASSERT then ? ;-) .

[gzotti]

Do you know the clang in-editor warning that all switch cases have been fulfilled so there is no need for a default clause?

Else, show me the logical error in the current code. Which code path does not return anything?

[axd1967]

I never wrote that the current situation is wrong ;-)
so currently it's OK (especially because all enums are covered.)

[alex-w]

I never wrote that the current situation is wrong ;-)

You did it 2 hours ago

[github-actions]

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