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 addresses and refines the handling of artboard transparency across various components of the application. It introduces more accurate blending for semi-transparent artboards in the rendering pipeline, simplifies the PNG export process by removing an unnecessary transparency parameter, and enhances color utility functions with new gamma-space conversion methods. These changes collectively improve the visual fidelity and internal consistency of how transparency is managed and rendered. Highlights
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. Footnotes
|
|
!build desktop (Run ID 23353978912) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix artboard transparency, and for the most part, the changes appear to be on the right track. The modifications to conditionally render the artboard background and adjust the PNG export logic are positive steps. However, I've identified a critical issue in wgpu-executor related to incorrect color space conversion that will result in improper background colors. Additionally, newly introduced color conversion helper functions in color_types.rs contain misleading examples and unresolved TODO comments for tests, which should be addressed to ensure code correctness and maintainability.
| target_texture.ensure_size(&self.context.device, size); | ||
|
|
||
| let [r, g, b, a] = background.unwrap_or(Color::TRANSPARENT).to_rgba8_srgb(); | ||
| let [r, g, b, a] = background.unwrap_or(Color::TRANSPARENT).to_rgba8(); |
There was a problem hiding this comment.
This change appears to introduce a color correctness issue. The background color is of type core_types::Color, which is in linear color space. The function to_rgba8 performs a direct float-to-u8 conversion without any gamma correction, which is only correct if the color is already in a gamma-corrected space.
However, vello::peniko::Color::from_rgba8 expects sRGB u8 values, which it then converts to linear floats for rendering. By using to_rgba8, you are passing linear values encoded as u8 to a function that expects sRGB u8, causing an incorrect double-conversion (treating linear as sRGB and converting to linear again).
The previous implementation using to_rgba8_srgb() was correct as it converted the linear Color to sRGB before converting to u8. Please revert this change.
| let [r, g, b, a] = background.unwrap_or(Color::TRANSPARENT).to_rgba8(); | |
| let [r, g, b, a] = background.unwrap_or(Color::TRANSPARENT).to_rgba8_srgb(); |
| /// # Examples | ||
| /// ``` | ||
| /// use core_types::color::Color; | ||
| /// let color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap(); | ||
| /// // TODO: Add test | ||
| /// ``` |
There was a problem hiding this comment.
The example for to_rgba8 is misleading and there's a // TODO: Add test.
The documentation states this function should be used if the Color is in gamma space. However, Color::from_rgbaf32 creates a Color in linear space. The example should reflect the correct usage, and the TODO can be resolved by adding an assertion.
| /// # Examples | |
| /// ``` | |
| /// use core_types::color::Color; | |
| /// let color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap(); | |
| /// // TODO: Add test | |
| /// ``` | |
| /// # Examples | |
| /// ``` | |
| /// use core_types::color::Color; | |
| /// let linear_color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap(); | |
| /// let gamma_color = linear_color.to_gamma_srgb(); | |
| /// assert_eq!(gamma_color.to_rgba8(), [99, 93, 252, 247]); | |
| /// ``` |
| /// # Examples | ||
| /// ``` | ||
| /// use core_types::color::Color; | ||
| /// let color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap(); | ||
| /// // TODO: Add test | ||
| /// ``` |
There was a problem hiding this comment.
Similar to to_rgba8, the example for to_rgb8 is misleading and there's a // TODO: Add test.
The documentation states this function should be used if the Color is in gamma space, but Color::from_rgbaf32 creates a Color in linear space. The example should be corrected and an assertion added to resolve the TODO.
| /// # Examples | |
| /// ``` | |
| /// use core_types::color::Color; | |
| /// let color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap(); | |
| /// // TODO: Add test | |
| /// ``` | |
| /// # Examples | |
| /// ``` | |
| /// use core_types::color::Color; | |
| /// let linear_color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap(); | |
| /// let gamma_color = linear_color.to_gamma_srgb(); | |
| /// assert_eq!(gamma_color.to_rgb8(), [99, 93, 252]); | |
| /// ``` |
|
|
|
Performance Benchmark Results
|
1 participant
No description provided.