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.

Sign up for GitHub

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

Jump to bottom

Replace CADisplayLink with CVDisplayLink #7583

Merged
merged 3 commits into from Feb 8, 2024
Merged

Replace CADisplayLink with CVDisplayLink #7583

merged 3 commits into from Feb 8, 2024

Conversation

as-cii
Copy link
Member

@as-cii as-cii commented Feb 8, 2024

Release Notes:

  • Fixed a bug that caused Zed to render at 60fps even on ProMotion displays.
  • Fixed a bug that could saturate the main thread event loop in certain circumstances.

as-cii and others added 3 commits February 8, 2024 17:55
@as-cii @mrnugget
Switch from CADisplayLink to CVDisplayLink
f7eee3a 
Co-Authored-By: Thorsten <thorsten@zed.dev>
@as-cii
Remove DisplayLinker and use on_request_frame instead
16f8ad0
@as-cii @nathansobo @maxbrunsfeld
Stop display link only when window is occluded
d4615a2 
Co-Authored-By: Nathan <nathan@zed.dev>
Co-Authored-By: Max <max@zed.dev>
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Feb 8, 2024
@RemcoSmitsDev
Copy link
Contributor

Is this a related issue? When hovering over something, drops the FPS to ~10.

Screen.Recording.2024-02-08.at.19.39.33.mov

@JosephTLyons JosephTLyons merged commit 67b96b2 into main Feb 8, 2024
6 checks passed
@JosephTLyons JosephTLyons deleted the cvdisplaylink branch February 8, 2024 18:47
JosephTLyons pushed a commit that referenced this pull request Feb 8, 2024
@as-cii @mrnugget
@nathansobo @maxbrunsfeld @JosephTLyons
Replace CADisplayLink with CVDisplayLink (#7583)
f0bf6ff 
Release Notes:

- Fixed a bug that caused Zed to render at 60fps even on ProMotion
displays.
- Fixed a bug that could saturate the main thread event loop in certain
circumstances.

---------

Co-authored-by: Thorsten <thorsten@zed.dev>
Co-authored-by: Nathan <nathan@zed.dev>
Co-authored-by: Max <max@zed.dev>
maxbrunsfeld added a commit that referenced this pull request Feb 8, 2024
@as-cii @mrnugget
@nathansobo @maxbrunsfeld
Replace CADisplayLink with CVDisplayLink (#7583)
59b2671 
Release Notes:

- Fixed a bug that caused Zed to render at 60fps even on ProMotion
displays.
- Fixed a bug that could saturate the main thread event loop in certain
circumstances.

---------

Co-authored-by: Thorsten <thorsten@zed.dev>
Co-authored-by: Nathan <nathan@zed.dev>
Co-authored-by: Max <max@zed.dev>
@mrnugget mrnugget mentioned this pull request Feb 9, 2024
Closed
1 task
@mrnugget
Copy link
Member

mrnugget commented Feb 9, 2024

@RemcoSmitsDev it could be! Try the latest release, we put the fix in Zed 0.121.7: https://zed.dev/releases/stable

cc @bennetbo 😄 we did it!

@RemcoSmitsDev
Copy link
Contributor

RemcoSmitsDev commented Feb 9, 2024 •
edited

@RemcoSmitsDev it could be! Try the latest release, we put the fix in Zed 0.121.7: https://zed.dev/releases/stable

cc @bennetbo 😄 we did it!

I cannot reproduce it with the stable version but still with the preview version. I'm not sure if the preview version contains the fix.

@as-cii
Copy link
Member Author

as-cii commented Feb 9, 2024

It actually makes sense that the frame rate drops when no input occurs, we're not producing any new frames because nothing changed on screen and we wanna save battery.

@mrnugget
Copy link
Member

mrnugget commented Feb 9, 2024

Oh, yes, that. I misunderstood. @RemcoSmitsDev take a look at this video: you can see that if there is something to do, things are rendered, blue line moves. But at the end, I'm just not doing anything, and rendering stops.

screenshot-2024-02-09-10.55.13.mp4

If I do that on a 60hz display it looks exactly like in your video.

@as-cii as-cii mentioned this pull request Feb 9, 2024
Closed
@RemcoSmitsDev
Copy link
Contributor

Oh, yes, that. I misunderstood. @RemcoSmitsDev take a look at this video: you can see that if there is something to do, things are rendered, blue line moves. But at the end, I'm just not doing anything, and rendering stops.

screenshot-2024-02-09-10.55.13.mp4
If I do that on a 60hz display it looks exactly like in your video.

Okay, makes sense, thanks! But I'm not sure if it's clear on my video, but when I hover over an identifier the popup keeps open until I click somewhere or hover over a new identifier. This happens only on the preview build.

Screen.Recording.2024-02-10.at.11.43.06.mov

@as-cii
Copy link
Member Author

as-cii commented Feb 10, 2024

That seems like a regression unrelated to these changes, as this pull request was hotfixed to stable. I know we recently changed our handling of mouse events on the editor to support clicking on links, maybe @ConradIrwin knows more about this?

Either way, would you mind opening a new issue and tagging it as “regression”? Thanks!

@RemcoSmitsDev
Copy link
Contributor

That seems like a regression unrelated to these changes, as this pull request was hotfixed to stable. I know we recently changed our handling of mouse events on the editor to support clicking on links, maybe @ConradIrwin knows more about this?

Either way, would you mind opening a new issue and tagging it as “regression”? Thanks!

Sure, made the issue: #7670

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@as-cii @RemcoSmitsDev @mrnugget @JosephTLyons