Fix rendering issues when built against the 10.14 SDK. #757
Conversation
|
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 |
|
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. |
|
@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. |
|
@chdiza 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. |
|
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. |
|
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. |
|
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.
|
|
Yikes. I’ll take a look at fullscreen when I’m back at a computer. Still no idea about the messages. |
|
@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. |
|
(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.) |
|
I'll test this out later, once I'm at the machine that spews messages. Thanks!
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? |
|
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. |
|
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. |
|
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 As you can see, the version from the old SDK is significantly faster and more performant. |
|
Small change: since this PR no longer does anything layer-specific, I just renamed the preference key and removed |
|
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 Also, exiting native full screen makes the MacVim window black. |
|
@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. |
|
If I build off of master, rendering is completely screwed up. However it's super responsive as you can see here: 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? |
|
@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. |
I will do this when I'm back at the relevant machine this week :) |
|
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 |
|
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 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. |
|
@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 And If we can find a way to draw to a thing without copies, and then display that in 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. |
|
@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:
A side comment: I spent a lot time before Mojave came out trying to find a replacement for |
|
That's great, thanks so much. I'm planning to work on this around 1d/week, hopefully starting this Friday. |
|
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.
|
@ychin et. al: I think this is in a good state now, PTAL! |
|
(And, to all, feel free to grab this and try to poke holes in it.) |
|
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
|
|
@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 😉. |
|
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 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 |
|
I'm also seeing some window blackouts when coming out of fullscreen (non-native mode). |
|
A bug I am also seeing is sometimes when changing the font size using |
|
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 |
|
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. |
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!!! |
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
force-pushed
the
fix-the-flicker
branch
from
31572d5 to
ab71bd8
Compare
|
The change was merged! I'll go close the related issues and prep an update soon. Thanks @s4y for the help. |
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
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
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.