Skip to content

Improve quality of rings rendering#2917

Merged
10110111 merged 1 commit into
Stellarium:masterfrom
10110111:rings
Jan 7, 2023
Merged

Improve quality of rings rendering#2917
10110111 merged 1 commit into
Stellarium:masterfrom
10110111:rings

Conversation

@10110111
Copy link
Copy Markdown
Contributor

@10110111 10110111 commented Dec 14, 2022

Description

Current code that renders planetary rings has some issues:

  • Outermost layer has a gap, as noted in Outer ring of Uranus has a gap #2775, and it becomes obvious even on Saturn if we reduce number of "stacks" in the call to sRing() to 3 or 2;
  • Rings of Saturn are obviously polygonal when zoomed in to screen size (see left and right sides of the screenshot below);
  • Additionally, ring shadow is not quite smooth: sort of aliased.

This PR redoes the rings in the following way:

  • Instead of making a polygonal model of the rings, the whole model becomes just a quad, and texture coordinates are computed in the fragment shader to yield an annulus instead of the square.
  • Mip mapping is enabled for ring texture.
  • Shadow is made behave correctly with mip mapping, finally becoming smooth.

Screenshots (if appropriate):

Old Saturn (viewed from Jupiter):

stellarium-003

New Saturn (viewed from Jupiter)

stellarium-004

Old Uranus

stellarium-001

New Uranus

stellarium-002

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
Copy Markdown

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

@alex-w alex-w added this to the 1.2 milestone Dec 15, 2022
@gzotti
Copy link
Copy Markdown
Member

gzotti commented Dec 16, 2022

Not yet, please! Observe rings from Pan...
grafik

or from Saturn:
grafik

It seems in perspective projection the rings vanish altogether. Other projections also look weird.

See #435, hopefully you can find a solution that also fixes that?

@10110111
Copy link
Copy Markdown
Contributor Author

But this is already much better than what we have in current master:

This PR:

stellarium-007

Master:

stellarium-008

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Dec 16, 2022

Yes, the problem around #435 is that Pan is inside the rings, so the tessellation in all but Perspective projection makes an ugly mess. Perspective is somewhat bearable. It seems that in your solution the observer is now not surrounded by rings.
And look into the strange break in the rings also when observed from Saturn's surface. This works well in master.

EDIT: You see, actually Saturn's shadow on the ring should be below the horizon.

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Dec 24, 2022

@10110111 , do you have a working solution at least for this view from Saturn? Else we should push it to 23.1 to not block 1.2.

@10110111
Copy link
Copy Markdown
Contributor Author

No, let this go to a next release.

@alex-w alex-w modified the milestones: 1.2, 23.1 Dec 24, 2022
@github-actions github-actions Bot added the has conflicts The pull request has conflicts label Jan 5, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 5, 2023

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

@github-actions github-actions Bot removed the has conflicts The pull request has conflicts label Jan 5, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 5, 2023

Conflicts have been resolved. A maintainer will review the pull request shortly.

@10110111 10110111 force-pushed the rings branch 2 times, most recently from 894713d to a6629d7 Compare January 7, 2023 12:19
@10110111
Copy link
Copy Markdown
Contributor Author

10110111 commented Jan 7, 2023

So, I have undone the simplification of the ring mesh to a square, and fixed #2775. Only roundedness of the rings (as seen from faraway observers) remains to be the topic of this PR, which shouldn't break the look from inside the rings more than it already is.

@alex-w
Copy link
Copy Markdown
Member

alex-w commented Jan 7, 2023

So, I have undone the simplification of the ring mesh to a square, and fixed #2775. Only roundedness of the rings (as seen from faraway observers) remains to be the topic of this PR, which shouldn't break the look from inside the rings more than it already is.

I accept it and approve the PR

Copy link
Copy Markdown
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.

Very good now, thank you!
I also consider #435 solved by this solving of the indexing. The ring is a bit coarse, but no mess.

@10110111 10110111 merged commit 5a64912 into Stellarium:master Jan 7, 2023
@10110111 10110111 deleted the rings branch January 7, 2023 19:56
@github-actions
Copy link
Copy Markdown

Hello @10110111!

The fix has been merged into source code and you may test it via building Stellarium from source code or wait the weekly development snapshot...

@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Mar 13, 2023
@github-actions
Copy link
Copy Markdown

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 Mar 27, 2023
@github-actions
Copy link
Copy Markdown

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants