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.

Sign up for GitHub

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

Jump to bottom

Always use texture-based text rendering in StelPainter #3343

Merged
merged 1 commit into from
Jul 27, 2023
Merged

Always use texture-based text rendering in StelPainter #3343

merged 1 commit into from
Jul 27, 2023

Conversation

10110111
Copy link
Contributor

This will let us control gamma correction by simply changing texture format when we convert our rendering pipeline to linear colors. We can't do this when using QOpenGLPaintDevice.

Text rendering quality doesn't change. From my observations via apitrace, QOpenGLPaintDevice does basically the same, even for rotated text: just renders textured quads with the texture containing text.

A side effect is a small performance improvement when the scene has a lot of text.

The -t mode now only disables antialiasing in StelPainter::drawText, and also does something for the GUI, which I didn't change.

Also, this PR will make #3003 obsolete and, possibly, fix the cause of it.

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 github-actions bot requested review from alex-w and gzotti July 25, 2023 20:29
@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.

@alex-w
Copy link
Member

alex-w commented Jul 25, 2023

The -t mode enable very strange look for text in bottom bar

@10110111
Copy link
Contributor Author

The -t mode enable very strange look for text in bottom bar

Not my fault, it's similarly ugly in master.

@alex-w alex-w added this to Needs triage in OpenGL/GLSL via automation Jul 25, 2023
@alex-w alex-w added this to Needs triage in GUI via automation Jul 25, 2023
@alex-w
Copy link
Member

alex-w commented Jul 25, 2023 •
edited

As far as I remember -t mode was added for low-cost devices such RPi or ANGLE-“powered” (low-graphics?) systems. Bad news, that I can’t say when GUI in -t mode was broken.

@10110111
Copy link
Contributor Author

It was for RPi, and the aliasing was intended. As for the wrong placement of the panel (not sure if you see it too), I suspect it might have been broken by #3125.

OpenGL/GLSL automation moved this from Needs triage to In progress Jul 25, 2023
GUI automation moved this from Needs triage to In progress Jul 25, 2023
@gzotti
Copy link
Member

gzotti commented Jul 26, 2023

Yes, I added -t for the early OpenGLES driver on RPi (2016), based on Guillaume's 0.12/Qt4(?) workaround for bad PC OpenGL drivers. Aliasing was not intended but accepted as irrelevant side aspect of a then otherwise working solution.
Meanwhile it's no longer needed on RPi. Not sure about super-old PCs, though, I have no junk PC to test.

@alex-w
Copy link
Member

alex-w commented Jul 26, 2023

But in this case we can remove support -t mode

@10110111
Copy link
Contributor Author

So, should we do it now?

@alex-w
Copy link
Member

alex-w commented Jul 26, 2023

I think yes. @gzotti?

@alex-w alex-w added this to the 23.3 milestone Jul 26, 2023
@gzotti
Copy link
Member

gzotti commented Jul 26, 2023

I hope I can test with RPi3&4 later tonight.

@gzotti
Copy link
Member

gzotti commented Jul 26, 2023

What is ugly or broken with -t in master?

@gzotti
Copy link
Member

gzotti commented Jul 26, 2023

Ok, looks like the older composition/layout of that status line. It still works, but just looks less perfect than the reworked solution. I may have not cared, given the lack of actual necessity of the -t option.

@gzotti
Copy link
Member

gzotti commented Jul 26, 2023

Please compare: right: 23.2. left: this branch, regular mode. At least on Windows, there is a significant loss of quality.
Screenshot (82)

@gzotti
Copy link
Member

gzotti commented Jul 26, 2023

RPi3, this PR
stellarium-rp3_pr3343
RPi3, master
stellarium-rp3_master

@gzotti
Copy link
Member

gzotti commented Jul 26, 2023

RP4, master
stellarium-rp4_master

RP4, master with -t
stellarium-rp4_master-t

RP4, this PR
stellarium-rp4_pr3343

RP4, this PR with -t
stellarium-rp4_pr3343-t

This really gives a nice speed gain!

@10110111
Copy link
Contributor Author

At least on Windows, there is a significant loss of quality.

What if you run with -platform windows:fontengine=freetype command line option?

@10110111
Copy link
Contributor Author

So, except for Windows, everything looks OK, right?

@gzotti
Copy link
Member

gzotti commented Jul 27, 2023

At least on Windows, there is a significant loss of quality.

What if you run with -platform windows:fontengine=freetype command line option?

Yes, that works. Fonts are a bit narrower now. Right: 23.2, left: this PR
Screenshot (83)

OK, to accept this, the platform option must go into all startup links, like -platform windows:altgr,fontengine=freetype, into the cmake/.iss. files.

I can also confirm a great speed improvement here (~40->65 fps!).

WAIT: I did a few more runs, and suddenly freetype seems not to become active. It looks thin and bad again. No log entries that could help.

@10110111
Copy link
Contributor Author

I did a few more runs, and suddenly freetype seems not to become active. It looks thin and bad again. No log entries that could help.

Try following this post and getting rid of the command line addition.

@gzotti
Copy link
Member

gzotti commented Jul 27, 2023 •
edited

Ah, thanks. It seems that the command line parsing of a comma-separated list of options -platform windows:altgr,fontengine=freetype does not work. Also giving separated -platform windows:... options fails. The altgr option is essential on many international keyboards, though.

So, what works is a file qt.conf right next to the .exe which contains:

[Platforms]
WindowsArguments = fontengine=freetype,altgr

This means:

  • On Windows builds, this files has to be created/provided (cmake?)
  • It must be deployed in installs (cmake/inno?)
  • The data/*.iss.* files must be freed from -platform windows:... options

@10110111
Copy link
Contributor Author

Yes, that works. Fonts are a bit narrower now. Right: 23.2, left: this PR

The +SEP and +SCP items look different in your two windows. Is there a good explanation, or is it a bug in this PR?

@gzotti
Copy link
Member

gzotti commented Jul 27, 2023

Uh, yes. Apps are RemoteSync'ed. No explanation, looks like a bug.

@alex-w
Copy link
Member

alex-w commented Jul 27, 2023

Please check 186febd

@10110111
Copy link
Contributor Author

What do these +SEP and +SCP represent? How do I enable them?

@alex-w
Copy link
Member

alex-w commented Jul 27, 2023

What do these +SEP and +SCP represent? How do I enable them?

SCP = South Celestial Pole; SEP = South Ecliptic Pole

Please check poles in Markings tab

@10110111
Copy link
Contributor Author

I have them, and from the code I see that the cross is rendered as a sprite, so this shouldn't be related to this PR.

@gzotti Do you reproduce this lack of the crosses in master?

@10110111
Always use texture-based text rendering in StelPainter
72c737a 
This will let us control gamma correction by editing a shader when we
convert our rendering pipeline to linear colors. We can't do this when
using QOpenGLPaintDevice.

Text rendering quality doesn't change. From my observations via
apitrace, QOpenGLPaintDevice does basically the same, even for rotated
text: just renders textured quads with the texture containing text.
@gzotti
Copy link
Member

gzotti commented Jul 27, 2023

Crosses are fine in master. But I still have the flood of

D:\StelDev\GIT\stellarium\src\core\StelApp.cpp:910: OpenGL error: 1281 (GL_INVALID_VALUE)

@gzotti
Copy link
Member

gzotti commented Jul 27, 2023

For what it's worth, crosses now also show in this PR.

@10110111
Copy link
Contributor Author

But I still have the flood of

Would be useful if you could find the point where this error occurs (e.g. via apitrace).

crosses now also show in this PR

OK, is it OK to merge then? @alex-w has pushed the changes to qt.conf, so it should work now.

@alex-w
Copy link
Member

alex-w commented Jul 27, 2023

Error 1281 should not be happening after rebasing

@gzotti
Copy link
Member

gzotti commented Jul 27, 2023

Can qt.conf also be created in the temporary build directory? I usually run a test build from there, without installation. Currently I need to create this manually.

@10110111
Copy link
Contributor Author

Error 1281 should not be happening after rebasing

Where do you see this error?

@gzotti
Copy link
Member

gzotti commented Jul 27, 2023

The 1281 is in master, not in this PR. This seems OK to merge. We could however really remove the -t option (including SUG deletions).

@10110111
Copy link
Contributor Author

We could however really remove the -t option

OK, will do it separately.

@10110111 10110111 merged commit 1168932 into Stellarium:master Jul 27, 2023
11 checks passed
OpenGL/GLSL automation moved this from In progress to Done Jul 27, 2023
GUI automation moved this from In progress to Done Jul 27, 2023
@10110111 10110111 deleted the texture-text branch July 27, 2023 21:26
@10110111 10110111 mentioned this pull request Jul 28, 2023
Merged
@alex-w
Copy link
Member

alex-w commented Jul 28, 2023

Can qt.conf also be created in the temporary build directory? I usually run a test build from there, without installation. Currently I need to create this manually.

It's possible, but I can't do the solution right now - after 2 weeks only. Please fill separate report for this issue for memory.

@gzotti gzotti mentioned this pull request Jul 28, 2023
Closed
@10110111 10110111 mentioned this pull request Aug 8, 2023
Closed
@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 @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
Reviewers

@alex-w alex-w alex-w approved these changes

@gzotti gzotti Awaiting requested review from gzotti

Assignees

@10110111 10110111

Labels
None yet
Projects
GUI
  
Done
OpenGL/GLSL
  
Done
Milestone
23.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@10110111 @alex-w @gzotti