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

[OSX] Cleanup video driver. #8652

Merged
merged 21 commits into from Feb 13, 2021
Merged

[OSX] Cleanup video driver. #8652

merged 21 commits into from Feb 13, 2021

Conversation

michicc
Copy link
Member

@michicc michicc commented Feb 6, 2021

Motivation / Problem / D

The OSX video driver is quite messy and littered with remnants of the old subdriver architecture. As only one subdriver is left, most of it can be cleaned up.

There are also a lot of cargo-culted things that make it quite different from the other drivers and/or are simply useless for us.

Description

This PR tries to clean up the most egregious things and switches to using newer APIs where useful.

Mouse tracking changed to newer APIs. Also, using a layer-backed view improves draw performance.

Limitations

Cocoa OSX design is very different from e.g. Win32 or SDL, so the video driver is still quite different from the other drivers.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@michicc michicc force-pushed the pr/osx_cleanup_2 branch 3 times, most recently from 9e51895 to 80d4759 Compare February 6, 2021 22:26
src/video/cocoa/cocoa_v.mm Outdated Show resolved Hide resolved
@michicc michicc force-pushed the pr/osx_cleanup_2 branch 3 times, most recently from 52da7c7 to 2516f27 Compare February 7, 2021 13:03
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.

I'm not going to pretend to understand the code itself, but here's some things I noticed while reading through

src/video/cocoa/cocoa_v.h Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_v.h Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_v.h Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_v.mm Outdated Show resolved Hide resolved
*/
bool VideoDriver_Cocoa::IsFullscreen()
{
return this->window != nil && ([ this->window styleMask ] & NSWindowStyleMaskFullScreen) != 0;
Copy link
Member

Choose a reason for hiding this comment

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

I suppose nil is some objective C thing? Should be consistent on nullptr/nil/NULL I suppose

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, nil is Objective-C. It is supposed to be consistent in the sense that Obj-C types are compared with nil and C/C++ pointers with nullptr.

src/video/cocoa/cocoa_v.mm Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_v.mm Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_wnd.mm Show resolved Hide resolved
src/video/cocoa/cocoa_wnd.mm Outdated Show resolved Hide resolved
{
driver = drv;
if (self = [ super init ]) {
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the = is intentional? Could use (()) to make it more obviously intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Common Objective-C ideom and usually written like this. I can add ()s if necessary.

@michicc
Copy link
Member Author

michicc commented Feb 9, 2021

@LordAro General note: many things are just literal copy/paste from before, but I can certainly fix some of the most egregious things.

@orudge
Copy link
Contributor

orudge commented Feb 10, 2021

Testing on my 2019 MacBook Pro, graphics performance does seem better with this patch.

…zes.

We never change the real screen resolution on OSX. As such, offering a list
of resolutions is pointless. Instead of that, offer the user a list of
commonly used window sizes up to the current screen size.
This allows the drawing backend code to be independent
of any event or command handling.
It wasn't displayed anyway as it was never copied to the bundle.
Copy link
Contributor

@orudge orudge left a comment

Choose a reason for hiding this comment

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

I have had a look over the code (albeit having done very little Objective C), and it looks reasonable to me. Tested it and it works, with a mild performance boost by the look of it, so happy for it to be merged if nobody else objects.

@michicc michicc merged commit acca56b into OpenTTD:master Feb 13, 2021
@michicc michicc deleted the pr/osx_cleanup_2 branch February 13, 2021 21:21
@LordAro LordAro mentioned this pull request Feb 21, 2021
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.

None yet

5 participants