Desktop: Add an 'Enable V-Sync' preference on Mac#3887
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the macOS desktop application experience by introducing a user-configurable VSync preference. It involves integrating this new setting into the application's core logic, updating the rendering pipeline to apply the chosen synchronization mode, and providing a user interface element for control. Additionally, it includes minor code improvements and a correction in the pixel preview rendering logic. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds a VSync preference for the desktop application on macOS. The preference is exposed in the preferences dialog and controls the wgpu::PresentMode to enable or disable VSync. The implementation correctly threads the preference from the editor UI down to the rendering state.
I've found one minor issue with the use of a lint attribute, for which I've left a specific comment. The rest of the changes, including some unrelated refactorings and a bug fix in pixel_preview.rs, look good.
| #[cfg_attr(not(target_os = "macos"), expect(unused))] | ||
| preferences: Preferences, |
There was a problem hiding this comment.
The expect(unused) attribute is likely not what you intend here. The #[expect] attribute is a Clippy attribute used to assert that a lint is triggered for the annotated code, and it will emit a clippy::needless_expect warning if the lint isn't triggered.
While it does suppress the original unused lint, the idiomatic way to suppress warnings for code that is intentionally unused on certain platforms is with #[allow]. Using #[allow(dead_code)] is more specific and clearly communicates the intent without the extra noise of expect.
| #[cfg_attr(not(target_os = "macos"), expect(unused))] | |
| preferences: Preferences, | |
| #[cfg_attr(not(target_os = "macos"), allow(dead_code))] | |
| preferences: Preferences, |
75a7a01 to
318fa0f
Compare
This gives users the option to choose between higher latency (a problem currently on Mac, specifically) or tearing, instead of forcing tearing as a latency reduction measure like we've done so far.