Disable srgb on Linux and macOS #1885
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
last-partizan
approved these changes
Jun 3, 2023
|
I think we can resolve conflicts and merge this. Looking good to me. |
The environment variable has changed to NEOVIDE_VSYNC and NEOVIDE_SRGB, and now works as it should. Previously NEOVIDE_NO_SRGB=1 enabled SRGB so the environment variable value was inverted.
fredizzimo
force-pushed
the
fsundvik/disable-srgb
branch
from
76b4f60
to
93af3e4
Compare
|
@last-partizan, I rebased it, and also made |
|
Checked it on linux/wayland. Works fine, merging. |
falcucci
pushed a commit
to falcucci/neovide
that referenced
this pull request
Jun 6, 2023
* Improve the code for running cmd_line tests serially * Improve handling of vsync and srgb and fix the environment variable. The environment variable has changed to NEOVIDE_VSYNC and NEOVIDE_SRGB, and now works as it should. Previously NEOVIDE_NO_SRGB=1 enabled SRGB so the environment variable value was inverted. * Disable srgb on non-windows platforms. * Make _nosrgb and _novsync private
LoipesMas
added a commit
to LoipesMas/neovide
that referenced
this pull request
Jun 6, 2023
It was changed in neovide#1885, but docs weren't updated
Merged
last-partizan
pushed a commit
that referenced
this pull request
Jun 7, 2023
falcucci
pushed a commit
to falcucci/neovide
that referenced
this pull request
Jun 7, 2023
The argument was flipped, but the behaviour was not. It was changed in neovide#1885, but docs weren't updated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
This disables srgb on Linux and macOs to fix issues with wrong colors being rendered on Wayland with Nvidia.
It's still enabled on Windows by default to work around #907.
Note that the actual rendering is always non-srgb due to the options we pass to Skia. Enabling srgb only enables the support, so it should really not be needed.
Disabling it fixes a bug on Wayland with Nvidia cards where the colours are rendered wrong.
This also adds the full pair of parameters
--srgb,--nosrgb,--vsync--novsync. So that you always can override what's set in the environment variables. The implementation is not the best due to missing support in clap, see clap-rs/clap#3146 and clap-rs/clap#815Finally it fixes the environment variables, they have now been renamed to NEOVIDE_VSYNC and NEOVIDE_SRGB,
and now work as they should. Previously
NEOVIDE_NO_SRGB=1enabled SRGB, so the environment variable value was actually inverted. Note the same fix is included in #1119 @LoipesMasAnd I almost forgot, it also improves the way the cmd_line tests are run serially and isolates the environment variable setting to affect only one test.
Did this PR introduce a breaking change?
A breaking change includes anything that breaks backwards compatibility either at compile or run time.
Yes,
NEOVIDE_NO_SRGB=0is nowNEOVIDE_SRGB=0andNEOVIDE_NO_VSYNC=0is nowNEOVIDE_VSYNC=1.Srgb is disabled by default on Linux and macOS, previously it was enabled, so it could cause regressions.
NOTE: I have only tested this on X11 and Wayland with KDE Plasma 5.27.5 and Nvidia drivers 525.116.04-1. Wayland did not work properly with this configuration before these changes. Windows should be unchanged, but I haven't tested it, and I don't own a macOS machine.