Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions desktop/src/render/composite_shader.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ fn fs_main(in: VertexOutput) -> @location(0) vec4<f32> {

if (viewport_srgb.a < 0.001) {
viewport_srgb = constants.background_color;
} else if (viewport_srgb.a < 0.999) {
viewport_srgb = blend(viewport_srgb, constants.background_color);
}

if (overlay_srgb.a < 0.001) {
Expand Down
9 changes: 1 addition & 8 deletions editor/src/node_graph_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,6 @@ impl NodeGraphExecutor {
file_type,
name,
size,
#[cfg(feature = "gpu")]
transparent_background,
artboard_name,
artboard_count,
..
Expand Down Expand Up @@ -491,12 +489,7 @@ impl NodeGraphExecutor {

match file_type {
FileType::Png => {
let result = if transparent_background {
image.write_to(&mut cursor, ImageFormat::Png)
} else {
let image: RgbImage = image.convert();
image.write_to(&mut cursor, ImageFormat::Png)
};
let result = image.write_to(&mut cursor, ImageFormat::Png);
if let Err(err) = result {
return Err(format!("Failed to encode PNG: {err}"));
}
Expand Down
24 changes: 24 additions & 0 deletions node-graph/libraries/no-std-types/src/color/color_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,18 @@ impl Color {
[(gamma.red * 255.) as u8, (gamma.green * 255.) as u8, (gamma.blue * 255.) as u8, (gamma.alpha * 255.) as u8]
}

/// Return the all components as a u8 slice, first component is red, followed by green, followed by blue, followed by alpha. Use this if the [`Color`] is in gamma space.
/// # Examples
/// ```
/// use core_types::color::Color;
/// let color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap();
/// // TODO: Add test
/// ```
Comment on lines +897 to +902
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
/// # 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]);
/// ```

#[inline(always)]
pub fn to_rgba8(&self) -> [u8; 4] {
[(self.red * 255.) as u8, (self.green * 255.) as u8, (self.blue * 255.) as u8, (self.alpha * 255.) as u8]
}

/// Return the all RGB components as a u8 slice, first component is red, followed by green, followed by blue. Use this if the [`Color`] is in linear space.
///
/// # Examples
Expand All @@ -907,6 +919,18 @@ impl Color {
[(gamma.red * 255.) as u8, (gamma.green * 255.) as u8, (gamma.blue * 255.) as u8]
}

/// Return the all RGB components as a u8 slice, first component is red, followed by green, followed by blue. Use this if the [`Color`] is in gamma space.
/// # Examples
/// ```
/// use core_types::color::Color;
/// let color = Color::from_rgbaf32(0.114, 0.103, 0.98, 0.97).unwrap();
/// // TODO: Add test
/// ```
Comment on lines +923 to +928
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
/// # 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]);
/// ```

#[inline(always)]
pub fn to_rgb8(&self) -> [u8; 3] {
[(self.red * 255.) as u8, (self.green * 255.) as u8, (self.blue * 255.) as u8]
}

// https://www.niwa.nu/2013/05/math-behind-colorspace-conversions-rgb-hsl/
/// Convert a [Color] to a hue, saturation, lightness and alpha (all between 0 and 1)
///
Expand Down
13 changes: 8 additions & 5 deletions node-graph/libraries/rendering/src/renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,18 +522,21 @@ impl Render for Artboard {
fn render_to_vello(&self, scene: &mut Scene, transform: DAffine2, context: &mut RenderContext, render_params: &RenderParams) {
use vello::peniko;

// Render background
let color = peniko::Color::new([self.background.r(), self.background.g(), self.background.b(), self.background.a()]);
let [a, b] = [self.location.as_dvec2(), self.location.as_dvec2() + self.dimensions.as_dvec2()];
let rect = kurbo::Rect::new(a.x.min(b.x), a.y.min(b.y), a.x.max(b.x), a.y.max(b.y));

scene.push_layer(peniko::Fill::NonZero, peniko::Mix::Normal, 1., kurbo::Affine::new(transform.to_cols_array()), &rect);
scene.fill(peniko::Fill::NonZero, kurbo::Affine::new(transform.to_cols_array()), color, None, &rect);
scene.pop_layer();
// Render background
if !render_params.hide_artboards {
let color = peniko::Color::new([self.background.r(), self.background.g(), self.background.b(), self.background.a()]);
scene.push_layer(peniko::Fill::NonZero, peniko::Mix::Normal, 1., kurbo::Affine::new(transform.to_cols_array()), &rect);
scene.fill(peniko::Fill::NonZero, kurbo::Affine::new(transform.to_cols_array()), color, None, &rect);
scene.pop_layer();
}

if self.clip {
scene.push_clip_layer(peniko::Fill::NonZero, kurbo::Affine::new(transform.to_cols_array()), &rect);
}

// Since the content's transform is right multiplied in when rendering the content, we just need to right multiply by the artboard offset here.
let child_transform = transform * DAffine2::from_translation(self.location.as_dvec2());
let mut render_params = render_params.clone();
Expand Down
2 changes: 1 addition & 1 deletion node-graph/libraries/wgpu-executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ impl WgpuExecutor {
if let Some(target_texture) = output.as_mut() {
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Suggested 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();

let render_params = RenderParams {
base_color: vello::peniko::Color::from_rgba8(r, g, b, a),
width: size.x,
Expand Down
Loading