Skip to content

Fix rendering issues when built against the 10.14 SDK. #757

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 2 commits into from
Dec 4, 2018

Conversation

s4y
Copy link
Contributor

@s4y s4y commented Oct 9, 2018

Before this commit, the Core Text renderer relied on a legacy behavior
of NSView to keep its content across draws. setNeedsDisplayInRect: could
be used to draw over parts of the existing content as drawing commands
were received without needing to redraw old content.

Layer-backed views lose this behavior and may be asked to redraw any or
all of their content at any time, and layer backing becomes the default
for apps built against the macOS 10.14 SDK.

This change adds a way to draw to an intermediate NSImage and then
create a layer contents from that image. It's similar to the CGLayer
path, but I wasn't able to get the CGLayer path to work without hanging
or crashing when I scrolled. My best guess from looking at stack traces
is that using CGContextDrawLayerInRect to draw a layer into itself
doesn't actually copy pixels, but adds the self-draw as an action to be
performed when the CGLayer is drawn into a bitmap context. After a bunch
of scrolling, these actions stack deeper and deeper and either hang or
overflow the stack when drawn.

When on, the new code skips -drawRect: by returning YES from
-wantsUpdateLayer. -updateLayer draws into an NSImage and then creates a
layer contents from the NSImage. On my machine, scrolling a large
document seems to be faster and use less CPU vs drawRect, which is
promising.

The new code is controlled by the MMUseCALayer user default, which is on
by default in this change. Fixes #751.

@chdiza
Copy link
Contributor

chdiza commented Oct 9, 2018

I just tried building with this on High Sierra with Xcode 9.4.1 just to make sure it doesn't cause any regressions. It built, but as soon as macvim is launched (via mvim), I see the following at my terminal prompt:

2018-10-09 09:37:14.141 MacVim[15908:90254] Unable to find feedback application.
2018-10-09 09:37:14.322 MacVim[15908:90254] Unable to find feedback application.

@chdiza
Copy link
Contributor

chdiza commented Oct 9, 2018

Well, now those messages are still there even after I rebuilt without the patch. They persist after a restart. They were not present until I built with the patch. I.e., building 8.1.451 on High Sierra with Xcode 9.4.1 worked fine. Then I did it against the patch of this PR and the warnings started. Then I rebuilt without the patch and they're still there.

@s4y
Copy link
Contributor Author

s4y commented Oct 9, 2018

@chdiza Thanks for testing it out. I’m not sure what to say if the messages are still there after removing my patch. I searched for that message online and didn’t find anything conclusive.

@s4y
Copy link
Contributor Author

s4y commented Oct 9, 2018

@chdiza how does everything look on your system aside from that message?

@chdiza
Copy link
Contributor

chdiza commented Oct 9, 2018

how does everything look on your system aside from that message?

I don't know, I reverted the instant I saw that message. I'm a little worried to try it on my other High Sierra machine that has Xcode 10 instead of 9.4.1, in case the messages start appearing there too.

@s4y
Copy link
Contributor Author

s4y commented Oct 9, 2018

Hmm, the patch is pretty minimal. Maybe someone else is willing to give it a shot? I have VMs but I’m not currently near my computer.

@chdiza
Copy link
Contributor

chdiza commented Oct 9, 2018

Eventually the messages went away. So, thinking it was some fluke that just happened to coincide with making a test build, I tried building with the patch again. And the messages immediately are back.

@chdiza
Copy link
Contributor

chdiza commented Oct 9, 2018

But just for fun I tried to see if the patch messes up non-native fullscreen mode. And it does, tremendously. Here are two screenshots, the first is not in fullscreen and the second is with the same file put into non-native fullscreen.

reg

screen shot 2018-10-09 at 12 05 51 p

@s4y
Copy link
Contributor Author

s4y commented Oct 9, 2018

Yikes. I’ll take a look at fullscreen when I’m back at a computer. Still no idea about the messages.

@s4y s4y force-pushed the fix-the-flicker branch from 04f41a5 to 99efd8f Compare October 10, 2018 18:18
@s4y
Copy link
Contributor Author

s4y commented Oct 10, 2018

@chdiza I fixed the non-native fullscreen issue — this new code path takes the place of the CGLayer path, and I needed to make sure that the CGLayer path couldn't be turned on at the same time. I amended the PR with that change and you should be able to give it another shot.

I occasionally see a flicker when entering and exiting non-native fullscreen. I'm looking into it but about to lose internet access for the day and may not have a chance to look into it right away.

@s4y
Copy link
Contributor Author

s4y commented Oct 10, 2018

(So, it's up to the maintainers, but I think that this is in a land-able state if the occasional enter/exit flicker is the only remaining problem.)

@chdiza
Copy link
Contributor

chdiza commented Oct 10, 2018

I'll test this out later, once I'm at the machine that spews messages. Thanks!

I think that this is in a land-able state if the occasional enter/exit flicker is the only remaining problem

Do we have any actual reports of this PR working on machines other than @s4y's? Is it causing "messages" on anyone else's machine?

@mario-grgic
Copy link

I have just tried this on macOS Mojave, compiled with Xcode 10, and it works fine. No error messages printed when running command line Vim either.

It fixes the Core Text renderer issue. No flashing in the GUI.

@chdiza
Copy link
Contributor

chdiza commented Oct 10, 2018

OK, I just built and very briefly tried it out on High Sierra w/ Xcode 941.

Re the flashing: on my older, slower machine I can see that what's happening during the flash (while entering non-native FS) is that first the MacVim window expands and looks exactly like the second of my two screenshots, where everything is "stretched" out of proportion; then it quickly "fixes" itself and the fonts and character cells are the correct size, as are the &rows and &columns.

(Maybe that is a clue that will help in diagnosing the flashing.)

I hope to also test this out on Mojave later at a different machine.

@amadeus
Copy link

amadeus commented Oct 11, 2018

Just as an FYI - performance on a 5k retina display (using this branch), when the window is maximized or full screen is actually way slower than the version built with the older SDK. Here's a video example. I tab between both version - the one with the white macOS bar at the top is the current version from the releases section (built using the old SDK) and the one with the dark theme title bar is from this branch:

http://wtf.amadeus.wtf/monosnap/screencast_2018-10-10_22-00-15.mp4

Just as an FYI - I am just holding down j and k for moving the cursor up and down - not tapping it. and <c-y> and <c-e> for scrolling.

As you can see, the version from the old SDK is significantly faster and more performant.

@s4y s4y force-pushed the fix-the-flicker branch from 99efd8f to 7c952f5 Compare October 13, 2018 15:39
@s4y
Copy link
Contributor Author

s4y commented Oct 13, 2018

@amadeus I just put up a new version of this change which is somewhere in between the old code and the first version of my change (i.e. still uses drawRect: to do partial updates). It should perform better — would you mind giving it a try?

@chdiza Mind giving this new version a try, too?

@s4y s4y force-pushed the fix-the-flicker branch from 7c952f5 to 0015e85 Compare October 13, 2018 15:44
@s4y
Copy link
Contributor Author

s4y commented Oct 13, 2018

Small change: since this PR no longer does anything layer-specific, I just renamed the preference key and removed self.wantsLayer = YES. Shouldn't change behavior.

@mario-grgic
Copy link

mario-grgic commented Oct 13, 2018

Ok, tried the latest changes, things work, no flashing. Performance on 2015 5k iMac is fine here (scrolling through 3 million lines file is smooth and fast). However, transparency is now broken. Window is not transparent with set transparency=50.

Also, exiting native full screen makes the MacVim window black. CTRL+l fixes it.

@amadeus
Copy link

amadeus commented Oct 14, 2018

@s4y unfortunately performance still appears to be pretty slow - here's another video example comparing the versions: (dark theme header is built off commit 0015e854b)

video

@mario-grgic
Copy link

@amadeus Are you sure your scrolling speed is related to the changes here? Could you try macOS Mojave with Xcode 10 build of master without the changes in this commit and see if the scrolling is fine without them.

I definitely don't see slower scrolling performance on 5k iMac screen.

@amadeus
Copy link

amadeus commented Oct 14, 2018

If I build off of master, rendering is completely screwed up. However it's super responsive as you can see here:

video

As a follow up, are you ensuring you only have 1 document open and the window is either full screened or resized to the full width of the screen?

@mario-grgic
Copy link

@amadeus Ok, I see the same behavior in native full screen as well. Scrolling speed is much slower, and just moving cursor up down is much slower. In Windowed (not full screen), the scrolling speed is not that bad.

@chdiza
Copy link
Contributor

chdiza commented Oct 14, 2018

Mind giving this new version a try, too?

I will do this when I'm back at the relevant machine this week :)

@chdiza
Copy link
Contributor

chdiza commented Oct 15, 2018

I just tested the latest iteration on High Sierra with Xcode 941 (non-retina screen). This version is by far the worst; it's unusable. The whole MacVim window often blacks out (not fixable with :redraw), and when it isn't blacked out, the antialiasing of the text is worse than what it would be on Mojave with a non-retina screen.

@chdiza
Copy link
Contributor

chdiza commented Oct 18, 2018

OK, I've also just tested it on Mojave (non-Retina). It blacks out the window frequently, as described above when I tried it on High Sierra. No :redraw or Ctrl-L will fix it.

But also, it "undoes" the fix for how bad fonts look on non-Retina screens on Mojave. This trick causes some, but not all, apps to make fonts look pretty much like they used to look pre-Mojave. Much to my relief, the trick works on the master branch of MacVim, but is apparently somehow blocked by the current iteration of this PR.

Moreover, the present state of the PR somehow drastically worsens the appearance of fonts in MacVim even on High Sierra (non-Retina). It makes them look like they do on Mojave non-Retina, i.e., thin, faint, and blurry. This was unexpected.

@s4y
Copy link
Contributor Author

s4y commented Oct 18, 2018

@chdiza That’s not how the PR is supposed to look/work and doesn’t match what I see. It might be an SDK or OS compatibility issue, but I have to investigate. Thanks for trying it.

@amadeus Thanks, that’s also very good to know.

Based on profiling, the pure drawRect: path is way faster than drawing to an image: the APIs I’m using in the current PR seem to make a copy of the image every time it’s drawn to, which makes it entirely unacceptable 😶.

And scrollRect:by: does some trickery that, as far as I know, won’t work in a layer-backed world (or, at least, I don’t know how to replicate it).

If we can find a way to draw to a thing without copies, and then display that in drawRect: or similar, or use it as a layer contents such that we can invalidate specific rects, that might be good enough.

Another option is to store state so that the view can draw whatever rects are requested at any time.

I’m not sure how much time I’ll have to work on this in the near future, but I will if I can. It involves corners of the SDK that I don’t know super well and takes some digging.

@ychin
Copy link
Member

ychin commented Nov 1, 2018

@amadeus We will look into the LICENSE issue (#765).

@s4y We should just try to just merge this change in first. My comments were mostly just for discussion for future enhancement :). Slow rendering is still better than broken rendering…

I'm probably going to take over reviewing this change. I'm still looking through the whole change, but a couple feedbacks now:

  1. Instead of a static MMDrawToImage that defaults to true, can you make it depend on whether Mojave is enabled? If MacVim is either compiled with pre-10.14 SDK (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_14) or at runtime it's used in a pre-Mojave OS (floor(NSAppKitVersionNumber) <= NSAppKitVersionNumber10_13) I think we should automatically use the old rendering code if the user didn't set MMDrawToImage manually. This minimizes the damage from the slower performance.
  2. Did you have a chance to look into fixing subpixel rendering? I don't think it's a dealbreaker if we do (1) since subpixel rendering is not officially supported on Mojave anyway and the current way to turn it on is really a hack. It would still be nice to find a way to fix it though so people who use the hidden toggle to turn it on can't still use it. On the other hand I feel that we definitely shouldn't break subpixel rendering for older OSes.
  3. We could consider ditching the whole CGLayer code and just use the new draw to image code but gut feeling is it would cause some backwards compatibility issues, so maybe we should punt on that for now unless we can verify otherwise.

A side comment: I spent a lot time before Mojave came out trying to find a replacement for scrollRect:by: and I don't think there's a good alternative, since Apple really wants you to back everything by CALayer's and let the Cocoa handle scrolling for you instead of a custom rect to copy. I think your per-line layer scrolling could work, but let's do that later since that would complicate the logic by quite a bit.

@ychin
Copy link
Member

ychin commented Nov 7, 2018

@s4y There is now a top-level LICENSE file (#765). Did you have time to look at my suggestions for change? I just want to make sure we don't regress on older OS versions.

@s4y
Copy link
Contributor Author

s4y commented Nov 14, 2018

That's great, thanks so much. I'm planning to work on this around 1d/week, hopefully starting this Friday.

@ychin
Copy link
Member

ychin commented Nov 29, 2018

Pinging @s4y. Do you mind pushing your WIP branches to this PR if you have time? I can fix up the remaining issues as I would like to fix this bug soon, but I don't want to take code from un-pushed changes for miscellaneous liability reasons.

Before this commit, the Core Text renderer relied on a legacy behavior
of NSView to keep its content across draws. setNeedsDisplayInRect: drew
over parts of the existing content as drawing commands were received,
without ever redrawing old content.

Layer backed views don't preserve content between draws and may be asked
to redraw at any time, and layer backing is default on in the 10.14 SDK.

This change adds a way to draw to a CGBitmapContext and then display
that in the view's layer. It's similar to the CGLayer path, but I wasn't
able to get the CGLayer path to work without hanging or crashing when I
scrolled. My best guess from looking at stack traces is that using
CGContextDrawLayerInRect to draw a layer into itself doesn't actually
copy pixels, but adds the self-draw as an action to be performed when
the CGLayer is drawn into a bitmap context. Scrolling stacks these
actions, which either hangs or overflows the stack when drawn.

The new code is controlled by the MMBufferedDrawing user default, which
is on by default on macOS >= 10.14 with this change. Fixes macvim-dev#751.
@s4y s4y force-pushed the fix-the-flicker branch from dcf6b23 to 0db547d Compare December 2, 2018 05:18
@s4y
Copy link
Contributor Author

s4y commented Dec 2, 2018

@ychin et. al: I think this is in a good state now, PTAL!

@s4y
Copy link
Contributor Author

s4y commented Dec 2, 2018

(And, to all, feel free to grab this and try to poke holes in it.)

@amadeus
Copy link

amadeus commented Dec 2, 2018

Just built it and ran it. So far the best version yet, however it still feels a bit slower than the previous version (the version built on the old SDK - downloaded from releases). And the wider the window the worse performance gets, even though the lines don't continue all the way offscreen.

I've included a few videos showing these differences (videos with the light title bar are the old SDK version, and the dark theme bar are this new build). I have a very fast keyboard repeat rate, and I use this to show the differences in performance, since this version performs a bit slower than the old version. Also worth noting all these were conducted on a late 2015 iMac Retina.

Moving cursor up and down by holding j/k

Old SDK
New SDK

The window is full height and half the screen size during this test. You'll notice a small bit of jank in moving the cursor up and down in the new version, vs feeling much smoother on the old version.

Scrolling document by holding <c-y>/<c-e>

Old SDK
New SDK

In this test, the window was maximized (but not full screen). The new version scrolls significantly slower than the old version. Performance gets marginally better if I make a ton a vertical splits and narrow the document. Not sure how feasible it is, but it would be awesome if we could overall improve performance from the old version, but that may be out of scope for this project.

@s4y
Copy link
Contributor Author

s4y commented Dec 2, 2018

@amadeus Thanks much. I wanted to focus on finishing this version — see @ychin's comment above. However, I have some work in progress that I wager will significantly improve performance from the old version. So, I'm glad this hasn't broken anything, and stay tuned 😉.

@chdiza
Copy link
Contributor

chdiza commented Dec 2, 2018

I just tried this. Something is wrong with the cursor positioning after scrolling. I open a document, and the cursor is on line 1. I do <C-F> to go down to the end. I do <C-b> to go all the way back up. And then the cursor is on line 17 (out of 55 visible whole lines). I feel fairly sure that the cursor should either end up on line 1 or on the last screen line.

OTOH maybe I am wrong to expect that. I just tried plain vim/vim on the same file, and the cursor winds up on line 41.

So this seems probably unrelated to the PR. But the discrepancy between vim and MacVim seems worth noting.

EDIT: Stupid github ate the <C-F> and <C-b> scrolling notes.

@chdiza
Copy link
Contributor

chdiza commented Dec 2, 2018

I'm also seeing some window blackouts when coming out of fullscreen (non-native mode).

@amadeus
Copy link

amadeus commented Dec 3, 2018

A bug I am also seeing is sometimes when changing the font size using cmd+- or cmd+= the screen blacks out and doesn't seem to recover unless I resize the window. :redraw does not seem to fix it

@ychin
Copy link
Member

ychin commented Dec 3, 2018

I can see some of the issues with blacking out and this change not working properly with the zoom button etc. I have a work-in-progress fix, and will push shortly, along with some other fixes for this PR.

@amadeus The performance fix will likely have to wait. It will require a bit of change.

@chdiza Can you elaborate more about the Ctrl-F / Ctrl-B issue? I couldn't repro it.

@ychin
Copy link
Member

ychin commented Dec 3, 2018

I have pushed my changes that should fix the misc resizing issues related to this change. That means zoom button, drag-to-resize, changing font size (Cmd+=), fullscreen (non-native and native), etc should all "just works"™ now.

I believe @s4y also fixed this to work with subpixel font smoothing, but @chdiza feel free to confirm. :)

I feel pretty good about this patch, but as usual there's probably something that I missed, so I'll let it soak a little bit and see if others or @s4y have additional comments or find issues with the current patch before I merge.

Performance: It's not ideal, but I think it's within tolerable range. Let's just fix the brokeness first. It's been broken for Mojave Homebrew users for months which I will try to make sure not to happen again.

@ronen
Copy link

ronen commented Dec 3, 2018

see if others or @s4y have additional comments

Comment from the peanut gallery: I haven't done any extensive testing, but I've been using this all day and it's been working fine. Thanks!!!

@pablinos
Copy link

pablinos commented Dec 3, 2018

Comment from the peanut gallery: I haven't done any extensive testing, but I've been using this all day and it's been working fine.

Same here - I noticed ctags being a bit more active (it's set to regenerate on save) than usual, but I don't think that's got anything to do with this change.

I can't say that I've noticed anything significant from a performance point of view too, but I haven't done any specific comparisons.

Thanks for working on it everyone!

Fix the misc resizing issues with the previous CoreText renderer commit,
in particular cases where zoom button was clicked, Vim initiated
resizing (e.g. ":set lines+=10"), font size changes (Cmd-+/-),
fullscreen toggles, etc.

- The core issue is that the order of operation for those are not
  consistent. Sometimes, MacVim changes window size first before letting
  Vim knows, but other times it lets Vim handle it before resizing (e.g.
  zoom).

- The new CoreText renderer's buffer needs to know when the size
  change in order to resize the buffer, and it wasn't doing it in the
  right spot. Fix it so that it's delayed until updateLayer: is called.
  By that time both MacVim and Vim should have already come to an
  agreement on the new size.

- Also, when using the new 10.14 buffer renderer, don't use
  [NSAnimationContext beginGrouping] to block the system from resizing
  the window, because it also suffers from the order of operation issue
  and sometimes endGrouping could get called before beginGrouping,
  causing the UI to appear frozen. Instead, just have updateLayer make a
  new image and copy over the old one to avoid the black flickering when
  resizing (which was what the begin/endGrouping was trying to solve to
  begin with), and the UI now works smoother as well (e.g. double
  clicking the border now works smoothly).

The previous change also set the window background color to whatever
default background color is which is fine but it affects the tabline
separator as well and makes it look jarring. The tabline separator is
mostly a relic of the older macOS versions, so disable it on new-ish
macOS verisons.

Also, update docs in the known issues section to make it clear there's
currently an issue in performance under Mojave. That will be removed
when the performance is fixed in the future.
@ychin ychin merged commit bbad3ed into macvim-dev:master Dec 4, 2018
@ychin
Copy link
Member

ychin commented Dec 4, 2018

The change was merged! I'll go close the related issues and prep an update soon. Thanks @s4y for the help.

ychin added a commit to ychin/macvim that referenced this pull request Dec 5, 2018
Vim patch 8.1.560

Targets macOS 10.8+

Features:
- macOS Mojave (10.14) is now supported.
    - MacVim's UI now works with Dark Mode.
    - Fixed broken rendering and flickering under Mojave when using the
      default Core Text renderer. macvim-dev#757
- guioption 'k' is supported again. macvim-dev#731
    - This option prevents window from resizing when UI elements such as
      toolbars or tabs show or hide themselves.

Fixes:
- Fixed misc fullscreen and window resizing bugs and artifacts macvim-dev#745
- Dragging tabs to reorder now works properly macvim-dev#789
- Fixed timer callback handling in GUI macvim-dev#749
- Fixed native tabs (10.12+) interring with Vim tabs macvim-dev#788
- Fixed Japanese IME Ctrl-U/Ctrl-O handling macvim-dev#742
- Fixed MMShareFindPboard and Cmd-E/Cmd-G interactions macvim-dev#780
- Better handling of guifontwide font size macvim-dev#737
- Better python discovery in default vimrc macvim-dev#739

Known Issues:
- Scrolling performance is slightly worse under Mojave macvim-dev#796

Script interfaces have compatibility with these versions:
- Lua 5.3
- Perl 5.18
- Python2 2.7
- Python3 3.7
- Ruby 2.5
ychin added a commit that referenced this pull request Dec 5, 2018
Vim patch 8.1.560

Targets macOS 10.8+

Features:
- macOS Mojave (10.14) is now supported.
    - MacVim's UI now works with Dark Mode.
    - Fixed broken rendering and flickering under Mojave when using the
      default Core Text renderer. #757
- guioption 'k' is supported again. #731
    - This option prevents window from resizing when UI elements such as
      toolbars or tabs show or hide themselves.

Fixes:
- Fixed misc fullscreen and window resizing bugs and artifacts #745
- Dragging tabs to reorder now works properly #789
- Fixed timer callback handling in GUI #749
- Fixed native tabs (10.12+) interring with Vim tabs #788
- Fixed Japanese IME Ctrl-U/Ctrl-O handling #742
- Fixed MMShareFindPboard and Cmd-E/Cmd-G interactions #780
- Better handling of guifontwide font size #737
- Better python discovery in default vimrc #739

Known Issues:
- Scrolling performance is slightly worse under Mojave #796

Script interfaces have compatibility with these versions:
- Lua 5.3
- Perl 5.18
- Python2 2.7
- Python3 3.7
- Ruby 2.5
@s4y s4y deleted the fix-the-flicker branch February 6, 2019 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants