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

Add lagrange points L4 and L5 #3090

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

snumlautoken
Copy link

@snumlautoken snumlautoken commented Mar 8, 2023

Description

This code adds the option to enable markings of L4 and L5 lagrange points between the current body and the solar object.

As points L1, L2 and L3 are all analogue to solar and antisolar points, I saw no reason to add them.

No additional dependencies are required.

Fixes #1111

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?

Honestly, I have just been eyeballing the feature in action and debugging to see that my math is correct. The math is not particularly hard, but I am not sure how this could be tested automatically. Let me know if you want me to add anything.

Test Configuration:

  • Operating system: Arch-linux
  • Graphics Card: Not really applicable

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 Mar 8, 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.

Copy link

@github-actions github-actions bot 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 adding your first pull request to Stellarium. If you have questions, please do not hesitate to contact us.

@alex-w alex-w added this to the 23.1 milestone Mar 9, 2023
@alex-w
Copy link
Member

alex-w commented Mar 9, 2023

OK, I see 2 Lagrange points for Earth-Sun system and this is good, but OP in the issue #1111 want to see L4 & L5 points for Earth-Moon system.

To avoid misunderstanding I propose rename "L4" into "L4 (Sun-planet)" and "L5" into "L5 (Sun-planet)" in the GridLinesMgr.cpp file for current L-points. What about L-points for the Moon?

@Atque
Copy link
Contributor

Atque commented Mar 9, 2023

As points L1, L2 and L3 are all analogue to solar and antisolar points, I saw no reason to add them.

Wouldn't those be good though, for educational reasons? And to understand the numbering of the lagrange points.

@snumlautoken
Copy link
Author

@alex-w
I have renamed the existing lagrange-points and added the L4 and L5 points for the earth-moon system

@Atque
Thought about it, but wouldn't it be cluttered? Especially seeing as how L1 and L3 occupy the same point (from the observer).
Would be easy to add if requested though.

@snumlautoken
Copy link
Author

Realized that my implementation does not take into account if the common plane between the two bodies is different from the ecliptical plane.
Will fix that.

{
// Takes the positional vector of the planet from the solar object
Vec3d pos= core->getCurrentObserver()->getHomePlanet()->getHeliocentricEclipticPos();
// Creates points by adding/subtracting 60 degrees to the vector along the ecliptic plane
Copy link
Member

Choose a reason for hiding this comment

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

This approach may work for circular orbits. How does it appear for e.g. Mars or Mercury? Its distance from the sun will place it far off the orbit distance at that point.

Copy link
Author

Choose a reason for hiding this comment

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

See commit 78d81e7
I am thinking it should account for deviations from orbit, but I am not really an astronomer.

@alex-w
Copy link
Member

alex-w commented Mar 9, 2023

Earth-Moon lagrange points should be visible from Earth only

@gzotti
Copy link
Member

gzotti commented Mar 9, 2023

L1, 2, 3 would be more clearly visible when observed from the associated "planet observer". Also, if they are computed with the planetocenter, an observer on the surface will see at least the distinction of L1 from L3 from parallax shift, even if only a few arcseconds. (But L3 is hidden behind the sun...)

@snumlautoken
Copy link
Author

snumlautoken commented Mar 9, 2023

Earth-Moon lagrange points should be visible from Earth only

This has been fixed

@snumlautoken
Copy link
Author

L1, 2, 3 would be more clearly visible when observed from the associated "planet observer". Also, if they are computed with the planetocenter, an observer on the surface will see at least the distinction of L1 from L3 from parallax shift, even if only a few arcseconds. (But L3 is hidden behind the sun...)

If that is the case, wouldn't we need the masses of the bodies to calculate the distances? As far as I can tell the stellar objects classes have no members for mass, so this would not work for the general case.
Please correct me if I am wrong.

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.

Well, we don't have masses of the SS bodies in our INI files, I think other LP should be implemented in separate PR if it critical. The current PR need couple cosmetic fixes, but it can be fixed after merge.

@Atque
Copy link
Contributor

Atque commented Mar 9, 2023

I think the labels should be "Sun-< name of current planet >" instead of "Sun-Planet".

@gzotti
Copy link
Member

gzotti commented Mar 9, 2023

When observing from Earth Observer, I expect to find L4, L5 somewhere close to Earth's orbit line. However, they are at no location that appear to make sense.

@snumlautoken
Copy link
Author

When observing from Earth Observer, I expect to find L4, L5 somewhere close to Earth's orbit line. However, they are at no location that appear to make sense.

Yes, I just awoke to the realization that I am calculating the common plane incorrectly, again...
It has been ages since I last touched linear algebra.
I will try to resolve it soon.

@snumlautoken
Copy link
Author

snumlautoken commented Mar 10, 2023

@gzotti
If you would please have a look at 2e838ba.
Been sure to make my testing more extensive and can at the very least confirm that the vectors (in the triangles) are the same length.
Sorry for all the trouble.

@alex-w alex-w added this to the 23.2 milestone Mar 11, 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.

OK, some cosmetic fixes (like: “L4(Sun-planet)” -> “L4 (Sun-planet)”) may be fixes before merging.

@snumlautoken
Copy link
Author

If you then move away from the earth (Alt+End), you will see this.

Gotcha! This has been fixed!
image

@snumlautoken
Copy link
Author

OK, some cosmetic fixes (like: “L4(Sun-planet)” -> “L4 (Sun-planet)”) may be fixes before merging.

Also fixed this in the commit, though it is not visible in the screenshot as I had not added it at the time that the screenshot was taken.

@gzotti
Copy link
Member

gzotti commented Mar 11, 2023

OK, seems we are getting somewhere. Now in this view, L1,2,3 would be a welcome addition. So, maybe show L1,2,3 if observed from an observer location?

The other thing is the label. IMO it is too much screen clutter. "L<n> (<planetname>)" or even "L<n><planetsymbol>" should be enough.

The next question is of course the actual reason for showing L points. OP wanted to see the location of the elusive Kordylewski clouds in the Moon's L4,5. AS soon as we have that, other people will become interested in seeing L4,5 of the other planets in search of what are Trojans for Jupiter. This would mean, 8x2 extra points (or 8x5 if observed from an "observer") over the whole sky if they are plotted for every planet. Or a fine-grained selection of which planet's L points the users are interested.

@snumlautoken
Copy link
Author

Now in this view, L1,2,3 would be a welcome addition

Is there a way to get my hands on the mass of the planet without hard-coding it? As far as I can tell it will be necessary for the calculation of the points.

This would mean, 82 extra points (or 85 if observed from an "observer") over the whole sky if they are plotted for every planet

I could imagine adding a drop-down menu for other planets in the SS, so while the current planet would be the default the user would have the option to choose whatever other planet in the SS. Does that sound reasonable?

@gzotti
Copy link
Member

gzotti commented Mar 11, 2023

We could introduce an optional mass_kg entry (default 0) to the ssystem_*.ini files, massKg instance variable and getMass() function to the Planet class.

@alex-w
Copy link
Member

alex-w commented Mar 11, 2023

The other thing is the label. IMO it is too much screen clutter. "L<n> (<planetname>)" or even "L<n>``<planetsymbol>" should be enough.

Bad idea for performance

The next question is of course the actual reason for showing L points. OP wanted to see the location of the elusive Kordylewski clouds in the Moon's L4,5. AS soon as we have that, other people will become interested in seeing L4,5 of the other planets in search of what are Trojans for Jupiter. This would mean, 8x2 extra points (or 8x5 if observed from an "observer") over the whole sky if they are plotted for every planet. Or a fine-grained selection of which planet's L points the users are interested.

Showing L-points for selected planet or for current planet when any planet is not selected?

@gzotti
Copy link
Member

gzotti commented Mar 11, 2023

The other thing is the label. IMO it is too much screen clutter. "L<n> (<planetname>)" or even "L<n><planetsymbol> " should be enough.

Bad idea for performance

What is so costly here? String creation in SkyPoint::updateLabel() is done once whenever language changes. String display? Is a short but complex string so much slower to draw than a long one? How much? If the sub is costly, we can omit it. But short labels should in any case be used to avoid screen clutter.

The next question is of course the actual reason for showing L points. OP wanted to see the location of the elusive Kordylewski clouds in the Moon's L4,5. AS soon as we have that, other people will become interested in seeing L4,5 of the other planets in search of what are Trojans for Jupiter. This would mean, 8x2 extra points (or 8x5 if observed from an "observer") over the whole sky if they are plotted for every planet. Or a fine-grained selection of which planet's L points the users are interested.

Showing L-points for selected planet or for current planet when any planet is not selected?

A choice of up to 8 planets plus Earth's Moon for which L points could be made available. This would call for a new sub-dialog available from the Gridline viewconfig page. It should be possible to display L4,5 for Jupiter and Saturn while having selection on some other object.

However, this all comes not in time for the upcoming release. Please let us discuss this further after 23.1 is out, there are still tasks to do.

@alex-w
Copy link
Member

alex-w commented Mar 12, 2023

What is so costly here? String creation in SkyPoint::updateLabel() is done once whenever language changes. String display? Is a short but complex string so much slower to draw than a long one? How much? If the sub is costly, we can omit it. But short labels should in any case be used to avoid screen clutter.

You need call SolarSystem/Planet classes every frame to get the localised name of the planets.

@gzotti
Copy link
Member

gzotti commented Mar 12, 2023

What is so costly here? String creation in SkyPoint::updateLabel() is done once whenever language changes. String display? Is a short but complex string so much slower to draw than a long one? How much? If the sub is costly, we can omit it. But short labels should in any case be used to avoid screen clutter.

You need call SolarSystem/Planet classes every frame to get the localised name of the planets.

updateLabels() is called when language changes, not in every frame. This of course assumes there are L points for each planet. If there is only a set of max. 5 L points for the current planet (or observed planet if user is on an observer-type planet), labels have to be recreated also if observed planet changes.

@snumlautoken
Copy link
Author

Things currently seem to be in limbo, so what exactly should we do to move forward with the PR?

@gzotti
Copy link
Member

gzotti commented Mar 14, 2023

Sorry, we are busy with real-world jobs and bugfixes for 23.1.

I wonder whether it makes sense to have up to all 8x5 L points for all planets and maybe even some moons of the gas giants which have other moons in their L4/5 points visualized in SolarSystemObserver view. This would require a dedicated selection GUI.

If you need planet mass, you could find and insert mass data for the planets in ssystem_major.ini. Just add lines like

mass_kg = 5.972168e24

@snumlautoken
Copy link
Author

snumlautoken commented Mar 14, 2023

I was thinking about just adding a dropdown-menu to select the body whose lagrange-points to show; or alternatively to show those of the current cursor-selected body. Wouldn't that suffice?
Thinking it would be a little wonky with a dedicated selection GUI.
image

@alex-w
Copy link
Member

alex-w commented Mar 20, 2023

Masses of the Sun and major planets in the master already

@alex-w
Copy link
Member

alex-w commented May 19, 2023

Any news?

@snumlautoken
Copy link
Author

Any news?

Ah, sorry, got busy with other work, so no progress has been made since
If I find time for it I could get it done for 23.2

@alex-w alex-w modified the milestones: 23.2, 23.3 Jun 15, 2023
@alex-w alex-w modified the milestones: 23.3, 23.4 Sep 10, 2023
@luzpaz
Copy link
Contributor

luzpaz commented Sep 25, 2023

soft bump, hoping this gets over the finish line soon 🤞

@github-actions github-actions bot added the has conflicts The pull request has conflicts label Oct 31, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@alex-w alex-w modified the milestones: 23.4, 24.1 Dec 5, 2023
@gzotti
Copy link
Member

gzotti commented Mar 6, 2024

@snumlautoken Any updates here?

@alex-w alex-w removed this from the 24.1 milestone Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has conflicts The pull request has conflicts
Development

Successfully merging this pull request may close these issues.

Locate the Lagrange Points
5 participants