Skip to content

Changed preset framerates; Add "low power" preset. #1422

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

Merged
merged 4 commits into from
Nov 6, 2021

Conversation

Penguin-Guru
Copy link

No description provided.

@ksuprynowicz
Copy link
Contributor

Awesome :)
I will test it later today

@digisomni digisomni added allow-build-upload Allows the upload of a build for non white-listed users enhancement New feature or request needs CR (code review) labels Oct 23, 2021
@digisomni digisomni added this to the 2021.2.0 Selene Release milestone Oct 23, 2021
@digisomni digisomni changed the title Changed preset framerates. Added preset. Changed preset framerates; Add "low-power" preset. Oct 23, 2021
@digisomni digisomni changed the title Changed preset framerates; Add "low-power" preset. Changed preset framerates; Add "low power" preset. Oct 23, 2021
Copy link
Contributor

@HifiExperiments HifiExperiments left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this solution!

@Penguin-Guru
Copy link
Author

It was ksuprynowicz's idea. Give him the credit. :p

@ksuprynowicz
Copy link
Contributor

ksuprynowicz commented Oct 24, 2021

It was ksuprynowicz's idea. Give him the credit. :p

Nah, I just wanted to switch off frame limiter on all presets. Kalila recommended adding power saving mode :)

@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. needs testing (QA) The PR is ready for testing and removed needs CR (code review) labels Oct 25, 2021
@ksuprynowicz
Copy link
Contributor

Everything works correctly on Linux

@ksuprynowicz
Copy link
Contributor

I think we should test and merge this PR before doing a release. Current frame limiter on Medium and Low is very confusing.

@digisomni
Copy link
Member

I'm on Windows 10, there's an issue where "High" gets the minimum possible resolution scale.

image

@ksuprynowicz
Copy link
Contributor

I just built and tested it on Windows 10, and I'm unable to replicate the problem. Resolution scale is always 1.0 for me in presets:
obraz

@Penguin-Guru
Copy link
Author

I'm on Windows 10, there's an issue where "High" gets the minimum possible resolution scale.

image

I'm not sure how the changes I made could have caused this. Does it occur consistently after a clean build? No scripts that might be affecting it?

@digisomni digisomni added needs CR (code review) and removed CR Approved At least one code reviewer has approved the PR. labels Oct 30, 2021
default:
// Do nothing anymore
// Do nothing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reasonable way of getting here? There should either be no default for a compile-time warning, or a warning log instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what UNKNOWN was intended for. I didn't really look into it but I don't remember seeing it used anywhere. It's not even the default mode. It wasn't related to this PR though so I didn't want to make too many unnecessary changes. Might be good to try logging it for a while and see if the log ever appears? If not, then remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think a log would be good here. I don't think anything should be reaching this point legitimately.

@digisomni
Copy link
Member

A fresh install with a fresh configuration yields the same results.

OS Name Microsoft Windows 10 Pro
Version 10.0.19042 Build 19042
Adapter Description NVIDIA GeForce RTX 3060 Ti
Driver Version 30.0.14.7196

@ksuprynowicz
Copy link
Contributor

A fresh install with a fresh configuration yields the same results.

Can you try from different Windows user account?

@Penguin-Guru
Copy link
Author

A fresh install with a fresh configuration yields the same results.

OS Name Microsoft Windows 10 Pro Version 10.0.19042 Build 19042 Adapter Description NVIDIA GeForce RTX 3060 Ti Driver Version 30.0.14.7196

So the Resolution Scale is set to minimum when you switch to HIGH (and only HIGH)? Does it change when you switch to any other preset after that?

@digisomni
Copy link
Member

A fresh install with a fresh configuration yields the same results.
OS Name Microsoft Windows 10 Pro Version 10.0.19042 Build 19042 Adapter Description NVIDIA GeForce RTX 3060 Ti Driver Version 30.0.14.7196

So the Resolution Scale is set to minimum when you switch to HIGH (and only HIGH)? Does it change when you switch to any other preset after that?

Yeah, it goes back to a normal 1.0 scale for the other options.

@Penguin-Guru
Copy link
Author

I'm compiling something to test. Will push for you to test if it works for me.

@Penguin-Guru
Copy link
Author

Can you test with that change?

@ctrlaltdavid
Copy link
Collaborator

Regarding the low resolution for HIGH setting, see the following line in PerformanceManager::applyPerformancePreset():

const float RECOMMANDED_PPI[] = { 200.0f, 120.f, 160.f, 250.f};

@Penguin-Guru
Copy link
Author

Penguin-Guru commented Oct 30, 2021

Regarding the low resolution for HIGH setting, see the following line in PerformanceManager::applyPerformancePreset():

const float RECOMMANDED_PPI[] = { 200.0f, 120.f, 160.f, 250.f};

Yeah, I just pushed that change to test. I'm not sure why the problem didn't occur for me or ksuprynowicz though. Maybe bad luck with our monitors?

@digisomni
Copy link
Member

Once it builds then I will give it a shot. :)

@digisomni
Copy link
Member

The issue appears to be solved now. Can anyone else download a copy from GH and check to confirm as well?

@digisomni digisomni requested a review from daleglass November 1, 2021 21:40
@ksuprynowicz
Copy link
Contributor

The issue appears to be solved now. Can anyone else download a copy from GH and check to confirm as well?

I just tested it on Windows 10, and it works perfectly, both in VR and in Desktop mode

@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Nov 4, 2021
default:
// Do nothing anymore
// Do nothing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think a log would be good here. I don't think anything should be reaching this point legitimately.

@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. and removed needs CR (code review) labels Nov 6, 2021
@Penguin-Guru
Copy link
Author

I could add the logging now but I wouldn't be able to test it for several days and it's kind of outside the scope of this P.R. I think it would be better if someone makes the change outside of this P.R. If you do want it here, feel free to make the change yourself. Edits by maintainers is enabled.

@daleglass daleglass merged commit 35a420d into vircadia:master Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-build-upload Allows the upload of a build for non white-listed users CR Approved At least one code reviewer has approved the PR. enhancement New feature or request QA Approved The PR has been tested successfully.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The default graphics setting of 'Low' is awful -- it throttles framerate and causes weird motion
7 participants