Skip to content

Add antialiasing setting. #1427

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 7 commits into from
Nov 2, 2021

Conversation

Penguin-Guru
Copy link

No description provided.

@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 25, 2021
@digisomni digisomni changed the title Antialiasing setting Add antialiasing setting. Oct 25, 2021
@ctrlaltdavid ctrlaltdavid added the needs testing (QA) The PR is ready for testing label Oct 28, 2021
@digisomni digisomni added this to the 2021.2.0 Selene Release milestone Oct 28, 2021
@digisomni digisomni requested a review from daleglass October 28, 2021 22:12
@ksuprynowicz
Copy link
Contributor

I've tested it on Linux in Desktop mode, and on Windows 10 in Desktop and VR mode. Works as intended in every case and helps avoid extreme blurriness issue with TAA on HP Reberb G2.

Copy link
Contributor

@daleglass daleglass left a comment

Choose a reason for hiding this comment

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

Some very minor notes mostly related to the comments. Otherwise, looks great!


void setAAMode(int mode);
int getAAMode() const { return _mode; }

void setDebugFXAA(bool debug) { debugFXAAX = (debug ? 0.0f : 1.0f); emit dirty();}
bool debugFXAA() const { return (debugFXAAX == 0.0f ? true : false); }

int _mode{ TAA };
int _mode{ TAA }; // '_' prefix but not private?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move it. It doesn't seem to be referenced from anywhere, so this is likely not intentional.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, that's what I thought. I couldn't find any references to it but I'm not confident I would be able to confirm any problems in testing. I think it would be best to move it separately from this PR, but I can move it here if you want.

@@ -140,7 +140,7 @@ void Antialiasing::run(const render::RenderContextPointer& renderContext, const
#else

void AntialiasingConfig::setAAMode(int mode) {
_mode = std::min((int)AntialiasingConfig::MODE_COUNT, std::max(0, mode));
_mode = std::min((int)AntialiasingConfig::MODE_COUNT, std::max(0, mode)); // Just use unsigned?
Copy link
Contributor

Choose a reason for hiding this comment

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

Ints are just more comfortable to work with, I figure. You have to take the problems of it being unsigned everywhere.

Copy link
Author

Choose a reason for hiding this comment

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

I find unsigned ints much more intuitive for enums, but I guess it doesn't matter. This particular line just seems like a waste so I wanted to flag it for you.

@Aitolda
Copy link
Collaborator

Aitolda commented Oct 30, 2021

Works. And I must say, having been involved with this engine and project for several years, Vircadia at 2x resolution on Oculus Quest (Link) in VR with antaliasing off, all effects off, looks so incredibly clean. The moment any effects are turned on there are huge amounts of aliasing introduced.

@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. and removed needs CR (code review) labels Nov 2, 2021
Copy link
Member

@digisomni digisomni left a comment

Choose a reason for hiding this comment

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

Works for me as well. Two things that should be mentioned: this is not set by any of the presets and is controlled independently. Also, it can be set while not using the Custom preset. It might be nice to put a divider between this and the other settings later. However, the ideal is for settings to all be selectable even when using a preset, upon changing something it just sets you to "Custom" anyway.

Notes for another time...

It seems to do its job, thank you!

@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Nov 2, 2021
@digisomni digisomni merged commit 61367ff into vircadia:master Nov 2, 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.

7 participants