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

Add option to force SRGB off within opengl #30

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

seifane
Copy link
Contributor

@seifane seifane commented Oct 23, 2023

Following issue #29

By default, GFX enables FRAMEBUFFER_SRGB in OpenGL. Even if shadertoy only uses RGBA at some point the conversion to SRGB is made which leads to disparities between shadertoy.com's rendering and shadertoy-rs's. This PR adds the option to disable FRAMEBUFFER_SRGB in OpenGL with a flag.

Unsafe has to be used in this case.

Shader used to compare changes.

Before:
image

After:
image

For more details see initial issue as well as :

Copy link
Owner

@fmenozzi fmenozzi left a comment

Choose a reason for hiding this comment

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

Looks good! Some minor nits

@@ -118,6 +121,8 @@ impl ArgValues {
(None, false)
};

let force_srgb_off = matches.is_present("force_srgb_off");
Copy link
Owner

Choose a reason for hiding this comment

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

Here and elsewhere (README.md, src/cli.yml), I'd prefer force-srgb-off (i.e. dashes instead of underscores

Copy link
Contributor Author

@seifane seifane Oct 24, 2023

Choose a reason for hiding this comment

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

It seems there is another parameter that is making use of underscores anisotropic_max. Should this one be changed to dashes too while I'm at it :D

Copy link
Owner

Choose a reason for hiding this comment

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

Doh! I missed that. In that case, for the sake of both uniformity and backwards compatibility, let's just use underscores for everything :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted :D

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you!

README.md Outdated Show resolved Hide resolved
Adjusted help message
@seifane seifane requested a review from fmenozzi October 24, 2023 10:01
@@ -118,6 +121,8 @@ impl ArgValues {
(None, false)
};

let force_srgb_off = matches.is_present("force_srgb_off");
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you!

@fmenozzi
Copy link
Owner

Thanks for your contribution!

@fmenozzi fmenozzi merged commit ddb8774 into fmenozzi:master Oct 24, 2023
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.

2 participants