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

Make fps intent settings available in the GUI #3073

Merged
merged 8 commits into from
Feb 26, 2023
Merged

Make fps intent settings available in the GUI #3073

merged 8 commits into from
Feb 26, 2023

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Feb 26, 2023

Description

I saw a discussion on CloudyNights (of course not in our forum) about users running Stellarium on their little scope PCs and RemoteDesktop, and CPU use always maxing out. One found the obvious solution to limit min_fps, which was highly acclaimed as breakthrough solution. OK, this branch makes the fps intent settings readily available also in the GUI.

Fixes # (issue)

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?

Test Configuration:

  • Operating system: Win11
  • Graphics Card: (Geforce, but should not matter)

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

@gzotti gzotti added the enhancement Improve existing functionality label Feb 26, 2023
@gzotti gzotti added this to the 23.1 milestone Feb 26, 2023
@gzotti gzotti self-assigned this Feb 26, 2023
@github-actions github-actions bot requested review from 10110111 and alex-w February 26, 2023 13:27
@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

The spinboxes should only apply the values when entry field editing is finished. I.e. keyboardTracking property should be set to false. Otherwise, when I type e.g. 15 into the min fps spinbox, I get a one second delay before the 5 appears, which is bad UX.

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

Can you make a subclass of QSpinBox that does this, or suggest what exactly has to be done? We have been using these spinboxes and various connection helpers for years. But I don't type values, just scroll the mouse wheel.

@10110111
Copy link
Contributor

diff --git a/src/gui/configurationDialog.ui b/src/gui/configurationDialog.ui
index 92dc02c9a6..4fc3e0d362 100644
--- a/src/gui/configurationDialog.ui
+++ b/src/gui/configurationDialog.ui
@@ -2368,6 +2368,9 @@
                  <property name="toolTip">
                   <string>Minimum intended fps between user interaction. Set very low to conserve energy or save CPU use.</string>
                  </property>
+                 <property name="keyboardTracking">
+                  <bool>false</bool>
+                 </property>
                  <property name="minimum">
                   <number>1</number>
                  </property>
@@ -2385,6 +2388,9 @@
                  <property name="toolTip">
                   <string>Maximum allowed fps. 10000 just means &quot;as fast as possible&quot;. </string>
                  </property>
+                 <property name="keyboardTracking">
+                  <bool>false</bool>
+                 </property>
                  <property name="minimum">
                   <number>10</number>
                  </property>

@10110111
Copy link
Contributor

The min/max limiting doesn't work as expected: suppose I have min 99, max 1000, and then I set max to 50. Then I move the focus away from max spinbox, and it resets back to 1000, which is clearly not the intent of the user that was to lower the maximum. I think the better way would be one of the following:

Option 1: set the max spinbox to the maximum of what the user wanted and the min spinbox, and for the min spinbox the value should be the minimum of the user's input and the max spinbox;
Option 2: if a maximum that was input by the user is lower than current minimum, set minimum to this new value. Similarly, if a minimum input by the user is larger than current maximum, set maximum to this new value.

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

I don't think our spinbox property helpers do that, and I don't want to put exceeding effort into this trivial addition. I also hope that users will find out after one failed try that they just need to change the other value first.

@10110111
Copy link
Contributor

Actually, your current code seems to try to follow my option 1, but for some reason it doesn't work as intended when I type in the value. Instead it simply rejects the value change.

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

I can add an explicit usage hint to the SUG, although I really think it's trivial to learn how to use these two settings, or why setting an obviously forbidden value does not work.

@10110111
Copy link
Contributor

My point is that your logic doesn't appear to work as intended.

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

Why? If I enter a value that is not appropriate, the input is ignored. If I scroll the mouse wheel, it stops at the last allowed value.

@10110111
Copy link
Contributor

Because maxfps = qMax(m, minfps) is supposed to take the larger value of the input and the minimum, while what I observe is that it simply restores the old maxfps. When the code says one thing, and the observations disagree, I'd call it a bug.

@alex-w
Copy link
Member

alex-w commented Feb 26, 2023

fps is abbreviation and I think is should be visible as FPS in the GUI and SUG

@10110111
Copy link
Contributor

fps is abbreviation and I think is should be visible as FPS in the GUI and SUG

Or, even more properly, it should be called "frame rate", since it's the name of this quantity, while "frames per second" is a unit.

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

Because maxfps = qMax(m, minfps) is supposed to take the larger value of the input and the minimum, while what I observe is that it simply restores the old maxfps. When the code says one thing, and the observations disagree, I'd call it a bug.

This is the setter function. It would also be the behaviour in a script. The GUI is another topic. If the GUI rejects a user setting because it does not make sense, this is one way to solve bad user input. Accepting input in one field and changing two fields as a consequence, it may appear to be more user friendly, but is more effort. You are cordially invited to fix that to your liking if you think it's worth the effort.

@10110111
Copy link
Contributor

This is the setter function. It would also be the behaviour in a script. The GUI is another topic.

I still don't get from the diff how this difference in behavior is achieved. Where exactly does the rejection happen?

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

See the connect*Property family of functions in StelDialog.

@10110111
Copy link
Contributor

No, the problem appears to be totally different: I had minimum_fps set to 1000 in the config file, while the new spinbox has a maximum of 99 (QSpinBox's default). This is what led me to think that 50 must be reset to 99, while it actually was reset to 1000. The maximum should be the same as in the other spinbox.

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

I left the max value at 99 because it has 2 decimal places and won't grow. Our default is 18. In practice, 5-20 fps is useful for most users, 30fps may be meaningful for script installations, and >30fps exceeds movie framerate. Usually (unless you unlock particular driver features) the OS limits max. framerate to 60. Is there a practical application for min_fps at more than 60fps?

@10110111
Copy link
Contributor

10110111 commented Feb 26, 2023

Is there a practical application for min_fps at more than 60fps?

Yes, of course: benchmarking on high-end machines. This is why I have it so high (and vsync off) on each machine where I develop or test Stellarium.

@gzotti
Copy link
Member Author

gzotti commented Feb 26, 2023

What is the highest fps you have ever seen? Would 1000fps be a reasonable upper limit for both?

@10110111
Copy link
Contributor

10110111 commented Feb 26, 2023

I use 1000 FPS for both min and max, which is sufficient for me, and this frame rate has never been hit (while 240 FPS I have seen IIRC). If you want to reduce the number of digits, you can set the upper limits to 999.

@10110111
Copy link
Contributor

The screenshot has to be updated after the renaming.
Other than this and a few comments above, I'm OK with the current state.

@gzotti gzotti merged commit 13fa30f into master Feb 26, 2023
@gzotti gzotti deleted the gui/fps branch February 26, 2023 21:14
@github-actions
Copy link

Hello @gzotti!

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

Hello @gzotti!

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

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 Mar 27, 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
Development

Successfully merging this pull request may close these issues.

3 participants