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

Smooth orbit lines (esp. for comets) #3371

Merged
merged 2 commits into from Aug 17, 2023
Merged

Smooth orbit lines (esp. for comets) #3371

merged 2 commits into from Aug 17, 2023

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Aug 16, 2023

Description

Drawing orbit lines from 360 time steps along the orbit works well for ellipses with low eccentricity only.
For highly elongated comet orbits, the perihel area is covered only with a few vertices, which causes a polygonal, usually vastly undersampled appearance. With the time step approach, the situation was remedied a bit by not plotting the entire orbit, but only the part which is signified by the orbit_good value in ssystem_minor.ini.

This PR provides a solution that constructs the entire orbit for ellipses with eccentricities e<0.999, by stepping over the eccentric anomaly. This looks generally much better and allows the estimation where in the outer solar system the object has its aphel. There still remain a few "polygon orbits" when they appear close to the observer.

Fixes # (issue)

Screenshots (if appropriate):

stellarium-094

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 11
  • Graphics Card: Geforce (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 (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

- Refactor KeplerOrbit
  - use Mean Anomaly or Eccentric Anomaly as input
- Draw closed orbits where possible, even for parts beyond orbit_good

The orbits for elongated ellipses (e<0.999) are now closed and look
much better by using steps in eccentric anomaly instead of time steps.
@gzotti gzotti added enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash labels Aug 16, 2023
@gzotti gzotti added this to the 23.3 milestone Aug 16, 2023
@gzotti gzotti self-assigned this Aug 16, 2023
@gzotti gzotti added this to Needs triage in Solar System via automation Aug 16, 2023
@gzotti gzotti added this to Needs triage in Visualization via automation Aug 16, 2023
@github-actions
Copy link

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.

@10110111
Copy link
Contributor

Is the screenshot the state before this PR or after?

@gzotti
Copy link
Member Author

gzotti commented Aug 16, 2023

It's the new state. Previously, only the orbit_good days were shown for the orbits, although most can now be shown entirely. Yes, there are still a few edges visible, those are close to the observer. We can increase to 720 or 1440 steps per orbit to remedy that somewhat (at probably high CPU cost), but an n-gon always has vertices...

@10110111
Copy link
Contributor

How is the second commit related to this PR?

@gzotti
Copy link
Member Author

gzotti commented Aug 16, 2023

It just happened, but has to do with comets and was long overdue. If all goes well, there will be two separate commits resulting from this PR.

@10110111
Copy link
Contributor

10110111 commented Aug 16, 2023

We can increase to 720 or 1440 steps per orbit to remedy that somewhat (at probably high CPU cost), but an n-gon always has vertices

This way would indeed require too many vertices, the closer the curve and the wider the FoV, the more vertices we'd need. But we need them only in one section, not for the whole orbit. So I think a better approach would be to generate the orbit already in the screen coordinates (which we do need to obtain anyway in drawOrbit()), and sample it smartly, something like this:

  1. Generate a few points, say, 10.
  2. For each pair of lines measure angle and distance between them on screen.
  3. If some quality function of angle and distance is too low, add a point with eccentric anomaly taken as average between the endpoints of the longer line.
  4. Stop when all lines have sufficient quality.

@gzotti
Copy link
Member Author

gzotti commented Aug 16, 2023

I did not intend to fix #1650. This here provides a significant improvement, though.

@gzotti gzotti mentioned this pull request Aug 17, 2023
15 tasks
Visualization automation moved this from Needs triage to In progress Aug 17, 2023
Solar System automation moved this from Needs triage to In progress Aug 17, 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.

Well, good improvement

@gzotti gzotti merged commit 616d56c into master Aug 17, 2023
18 of 20 checks passed
Solar System automation moved this from In progress to Done Aug 17, 2023
@gzotti gzotti deleted the fix/smooth_comet_orbits branch August 17, 2023 07:57
Visualization automation moved this from In progress to Done Aug 17, 2023
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Sep 2, 2023
@github-actions
Copy link

github-actions bot commented Sep 2, 2023

Hello @gzotti!

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 @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
Projects
Solar System
  
Done
Visualization
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants