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 computation of signed phase angle to determine apparent magnitude #3306

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

10110111
Copy link
Contributor

@10110111 10110111 commented Jul 5, 2023

Description

Current computation of apparent magnitude uses two similar quantities: phase angle and something that approximates it, adding a sign to indicate whether the it's a waxing or waning phase.

As I understand, the only reason of determining the sign is to make magnitude depend on what half of the Moon is illuminated, due to asymmetry in the albedo of the visible side of the Moon. So the quantity should actually be equal to the phase angle, up to a sign.

The unsigned phase angle is computed correctly, taking into account positions of the observer and the Moon. But the way the signed quantity is currently computed has some problems:

  1. It has some daily ripples compared to the phase angle that shouldn't be there (possibly the shift of the observer from planet center isn't taken into account);
  2. It switches sign not at the point of 0 or 180° phase, but a few dozens of minutes away from it.

This PR implements the correct computation of whether the phase angle is increasing or decreasing (this is done in Planet::isWaning()). This computation uses heliocentric position and velocity of the observer (the velocity is computed in StelCore::getObserverHeliocentricEclipticVelocity()).

I spent some time time trying to reverse-engineer orientation of the axes of the AltAz frame, so the first commit here documents my findings. Please check that it's actually correct.

Also, it appeared that the computation to determine the phase angle tendency is quite nontrivial (the simple elongation-based trick mentioned in #2916 won't yield precise results), so the third commit here exports a flag to the scripting API that indicates whether the planet is in the waning phase, i.e. whether its phase angle is increasing.

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: 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

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

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/core/StelCore.hpp Outdated Show resolved Hide resolved
@10110111 10110111 force-pushed the signed-phase-angle branch 3 times, most recently from a0f65e3 to 934f403 Compare July 7, 2023 18:43
src/core/StelCore.hpp Outdated Show resolved Hide resolved
@@ -305,6 +305,9 @@ class StelCore : public QObject

//! Return the observer heliocentric ecliptic position (GZ: presumably J2000)
Vec3d getObserverHeliocentricEclipticPos() const;
//! Return the observer heliocentric ecliptic velocity. This includes orbital and diurnal motion;
Copy link
Member

Choose a reason for hiding this comment

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

If you include diurnal changes in velocity, this should be added to the aberration correction of the observer in SolarSystem::computePositions().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose then observerPlanet->getHeliocentricEclipticPos() should also be replaced with core->getObserverHeliocentricEclipticPos(), right? Because currently that computation takes "observer" to be in the center of the planet.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I also think that's all that has to be changed. When I did aberration correction, I said we won't include the diurnal aberration, but when you are doing this computation now, it's even one more step. Not sure about the magnitude, though, it may introduce an observable milliarcsecond wobble. Of course, switching off topographic correction will also disable that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it should go right into this PR? This seems quite unrelated.

Copy link
Member

Choose a reason for hiding this comment

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

It will be an obvious next change then. Probably we can even make a switch aberration: none/annual/diurnal (for didactic purposes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you only say that it should be applied in SolarSystem? Shouldn't it equally affect everything?

@alex-w alex-w added this to the 23.3 milestone Jul 8, 2023
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!

@10110111
Copy link
Contributor Author

@gzotti What is your verdict on this PR?

@gzotti
Copy link
Member

gzotti commented Jul 16, 2023

Just having a look into its differences, but answering other duties at the same time.

Why is the Moon about 0.1-0.2mag less bright? It may not matter a lot in reality, but still it's a notable difference between my model and your change in computing one of the required angles.

@10110111
Copy link
Contributor Author

10110111 commented Jul 16, 2023

The change in magnitude reflects the change in the phase angle that's fed into the algorithm that computes the magnitude. A totally expected result.

10110111 added 2 commits July 17, 2023 01:02
It proved to be quite challenging to properly determine whether phase
angle is increasing or decreasing. So it's worth providing a flag for
this in the scripting API.
@10110111 10110111 force-pushed the signed-phase-angle branch from 934f403 to 9e8316c Compare July 16, 2023 21:07
@10110111
Copy link
Contributor Author

The difference between the old angle and the new one is that the old angle was computed between projected positions of the observer and the Moon, while the new one is between the actual 3D positions.

As we take into account different z components of the positions, we should expect the angle between the objects to increase, which will reduce the visible lit part.

Copy link
Member

@gzotti gzotti left a comment

Choose a reason for hiding this comment

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

OK, the original paper was in terms of phase angle, not dLong, so it should be fine.

This change will introduce localized heliocentric velocity, so despite previous reservation, diurnal aberration should come soon.

@10110111 10110111 merged commit 7a2d667 into Stellarium:master Jul 19, 2023
@10110111 10110111 deleted the signed-phase-angle branch July 19, 2023 14:56
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Jul 25, 2023
@github-actions
Copy link

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 Sep 26, 2023
@github-actions
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
Development

Successfully merging this pull request may close these issues.

3 participants