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

Fix #7644: (Cocoa) Manually set colorspace to sRGB #8023

Merged
merged 1 commit into from Mar 30, 2020

Conversation

spauka
Copy link
Contributor

@spauka spauka commented Feb 26, 2020

This commit manually sets the colorspace for the window to sRGB, as suggested in #7644 by @SoothedTau, to fix performance issues on OSX when using devices that use the P3 colorspace by default. This makes the game run smoothly on my laptop, with the full 33fps rate.

Screen Shot 2020-02-26 at 3 49 03 pm

@spauka spauka force-pushed the cocoa_set_colorspace branch 2 times, most recently from 060ace2 to 2c22838 Compare February 26, 2020 04:53
@spauka spauka changed the title Fix #7664: (Cocoa) Manually set colorspace to sRGB Fix #7644: (Cocoa) Manually set colorspace to sRGB Feb 26, 2020
@nielsmh
Copy link
Contributor

nielsmh commented Feb 26, 2020

Crashes for me on macOS 10.13.6 (Macbook Pro, late 2010 model). I haven't looked into the details of the crash yet.
Tested master and this PR rebased onto master, builds of master do work normally.

Assertion failed: (this->cgcontext != NULL), function WindowResized, file /Users/nielsm/dev/OpenTTD/src/video/cocoa/wnd_quartz.mm, line 588.
 
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff52bdab66 __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff52da5080 pthread_kill + 333
2   libsystem_c.dylib             	0x00007fff52b361ae abort + 127
3   org.openttd.openttd           	0x000000010cb53e5c HandleCrash(int) + 316 (crashlog_osx.cpp:249)
4   libsystem_platform.dylib      	0x00007fff52d98f5a _sigtramp + 26
5   ???                           	0x000000011cad3088 initialPool + 40
6   libsystem_c.dylib             	0x00007fff52b361ae abort + 127
7   libsystem_c.dylib             	0x00007fff52afe1ac __assert_rtn + 320
8   org.openttd.openttd           	0x000000010cd276e9 WindowQuartzSubdriver::WindowResized() + 345
9   org.openttd.openttd           	0x000000010cd268e5 WindowQuartzSubdriver::SetVideoMode(int, int, int) + 1109 (wnd_quartz.mm:349)
10  org.openttd.openttd           	0x000000010cd277eb QZ_CreateWindowQuartzSubdriver(int, int, int) + 251 (wnd_quartz.mm:486)
11  org.openttd.openttd           	0x000000010cd21e03 VideoDriver_Cocoa::Start(char const* const*) + 659 (cocoa_v.mm:405)
12  org.openttd.openttd           	0x000000010c9d53cc DriverFactoryBase::SelectDriverImpl(char const*, Driver::Type) + 924 (driver.cpp:120)
13  org.openttd.openttd           	0x000000010c9d4fe1 DriverFactoryBase::SelectDriver(char const*, Driver::Type) + 17 (driver.cpp:87)
14  org.openttd.openttd           	0x000000010cb4245b openttd_main(int, char**) + 4075 (openttd.cpp:770)
15  org.openttd.openttd           	0x000000010cb574d7 main + 151 (unix.cpp:261)
16  libdyld.dylib                 	0x00007fff52a8a015 start + 1

@nielsmh
Copy link
Contributor

nielsmh commented Feb 26, 2020

I tested from a clean starting point. Just adding the [ this->window setColorSpace:[ NSColorSpace sRGBColorSpace ] ]; after window creation in WindowQuartzSubdriver::SetVideoMode() seems to improve the performance a lot. All the other moving the color_space around and trying to make it a member variable seems unnecessary.

@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Feb 26, 2020
@spauka
Copy link
Contributor Author

spauka commented Feb 27, 2020

@nielsmh Thanks for checking, I was running without assertions which hid this error!

Latest commit fixes the crash you experienced, WindowResized was being called before the colorspace was set. Although if you feel it's cleaner I/you can also remove the color_space member variable...

src/video/cocoa/cocoa_v.h Outdated Show resolved Hide resolved
src/video/cocoa/wnd_quartz.mm Outdated Show resolved Hide resolved
@spauka
Copy link
Contributor Author

spauka commented Feb 27, 2020

Thanks for the review, I've implemented both requested changes.

@spauka spauka requested a review from LordAro February 27, 2020 09:56
@LordAro LordAro dismissed their stale review February 27, 2020 10:10

done, will wait on @nielsm for ok

@orudge
Copy link
Contributor

orudge commented Feb 27, 2020

Seems to work better for me on 10.14.6 (MacBook Pro 15-inch 2019)

@LordAro LordAro requested review from nielsmh and removed request for LordAro February 29, 2020 11:43
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

good enough

@nielsmh nielsmh merged commit bd3a587 into OpenTTD:master Mar 30, 2020
@andythenorth
Copy link
Contributor

andythenorth commented Mar 30, 2020

Performance is measurably better with this PR versus 1.10.0 beta 2 (using in-game FPS).

This PR re-introduces the unfortunate magenta cast per #7644 (comment)

Without wanting to be dramatic about it, that makes the game very visually unappealing, the magenta sprites are unpleasant and the contrast is significantly reduced.

I guess there might just be no remedy for that.

@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants