Skip to content

Render ShowMySky atmosphere calculating view direction per fragment#3075

Merged
10110111 merged 1 commit into
Stellarium:masterfrom
10110111:projectors-shaders-direct
Mar 5, 2023
Merged

Render ShowMySky atmosphere calculating view direction per fragment#3075
10110111 merged 1 commit into
Stellarium:masterfrom
10110111:projectors-shaders-direct

Conversation

@10110111
Copy link
Copy Markdown
Contributor

Description

This PR makes atmosphere look better in the parts of the screen where view direction changes rapidly.

One problem that we have here is that switching between projections takes about 3 seconds to recompile all the ShowMySky shaders. After the particular projection has been switched to, usually the shaders are cached by the GPU driver (at least by NVIDIA and Mesa), so this has a limited impact. But I'm not sure if the initial delay is OK to have.

An alternative solution is to render the view directions to a separate texture and use it as an additional input for the renderer. This gets rid of the switching delay, but reduces performance for all other times (by about 9% of frame rate on my machine).

Screenshots

Old

Screenshot_2023-02-27_00-01-40

New

Screenshot_2023-02-27_00-02-12

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.

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Feb 26, 2023

Looks great!
I think 3s for shader recompile is acceptable (switching is not done often IMO). 9% performance loss would be bad though.

@10110111
Copy link
Copy Markdown
Contributor Author

Did you try it out? I'd like to know whether it's acceptable after you actually have experienced it.

@alex-w alex-w added this to the 23.1 milestone Feb 26, 2023
@gzotti
Copy link
Copy Markdown
Member

gzotti commented Feb 27, 2023

Not tried yet.

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

github-actions Bot commented Mar 5, 2023

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

@10110111 10110111 force-pushed the projectors-shaders-direct branch from b3417b5 to a04f720 Compare March 5, 2023 06:28
@github-actions github-actions Bot removed the has conflicts The pull request has conflicts label Mar 5, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 5, 2023

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

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Mar 5, 2023

Works great on the Geforce. Switching delay totally acceptable IMO. Testing others...

@alex-w
Copy link
Copy Markdown
Member

alex-w commented Mar 5, 2023

Similar visual issue I see for Preetham model too - is it possible resolve that too?

@10110111
Copy link
Copy Markdown
Contributor Author

10110111 commented Mar 5, 2023

Similar visual issue I see for Preetham model too - is it possible resolve that too?

No, Preetham is CPU-based: all the colors are computed in computeColor(). Any attempt to move it to the GPU is going to slow it down. Given that it's useful only on slow machines, I wouldn't try to do this, just as I didn't try to support GLES2 with new-way old-style landscapes.

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Mar 5, 2023

Works well as-is on my Intel. Anything to try out? Qt5/6? Angle and Raspi have no SMS, so I skip those.

@10110111
Copy link
Copy Markdown
Contributor Author

10110111 commented Mar 5, 2023

Works well as-is on my Intel

Was it on Windows? AFAIR, Intel@Windows doesn't even cache the shaders. On Mesa and nvidia at least the delay happens only once per projection, because the compilation&linking results are cached.

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Mar 5, 2023

Yes, WIn10. Core i5-4570. (Intel HD4600). Performance is low (about 6-7fps with SMS, about 17fps with Preetham). Stellarium 1.2 has 6-7fps with SMS. Switching now causes a delay, but is acceptable.

@10110111 10110111 marked this pull request as ready for review March 5, 2023 11:25
@github-actions github-actions Bot requested a review from alex-w March 5, 2023 11:25
@10110111 10110111 merged commit 57a7aec into Stellarium:master Mar 5, 2023
@10110111 10110111 deleted the projectors-shaders-direct branch March 5, 2023 12:21
@github-actions
Copy link
Copy Markdown

Hello @10110111!

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

@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 state: published The fix has been published for testing in weekly binary package and removed state: fixed labels 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