Add a gradient interpolation space attribute with sRGB Gamma (existing) and sRGB Linear (new) - #4412
Add a gradient interpolation space attribute with sRGB Gamma (existing) and sRGB Linear (new)#4412Keavon wants to merge 4 commits into
Conversation
… linear light by default
…uring deserialization
There was a problem hiding this comment.
3 issues found across 26 files
Confidence score: 3/5
- In
node-graph/libraries/vector-types/src/gradient.rs, treating missinggradient_interpolationasDefault::default()(SrgbLinear) appears to erase the intended “field absent” meaning, which can silently change gradient behavior on read/round-trip — preserve an explicit missing/auto state instead of defaulting. - In
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, inline SVG styles with repeatedcolor-interpolationcan resolve to the first declaration instead of the last applicable one, so imported gradients may render in the wrong color space — update the resolver to follow CSS cascade order. - In
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, SVG import reads inline/presentation attributes but not CSS<style>blocks, so gradients from common exporters may import with incorrect interpolation — add<style>parsing support (or a clear fallback path with tests) to reduce compatibility risk.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:575">
P2: SVG gradients with repeated `color-interpolation` declarations in an inline style can import with the wrong interpolation space because the resolver keeps the first declaration; selecting the last applicable declaration would match the CSS cascade.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:914">
P2: SVG import now resolves each gradient's interpolation from its inline `style` attribute and its `color-interpolation` presentation attribute, but it does not read CSS `<style>` blocks. Many exported SVGs (Illustrator/Inkscape) set `color-interpolation: linearRGB` through grouped stylesheet rules, and those gradients would fall through to the `SrgbGamma` default on import, silently discarding the author's declared linear interpolation. Consider also scanning `<style>` elements (and matching the selectors, or at least any `color-interpolation` declared there) so the import honors them rather than silently defaulting to gamma.</violation>
</file>
<file name="node-graph/libraries/vector-types/src/gradient.rs">
<violation number="1" location="node-graph/libraries/vector-types/src/gradient.rs:179">
P2: The item/attribute read path resolves a missing `gradient_interpolation` to `Default::default()` = `SrgbLinear`, which contradicts the design intent (and the doc comment on this struct) that the field's absence marks a pre-existing ramp and should read as the legacy `SrgbGamma`. The serde exchange form honors this via `serde(default = "GradientInterpolation::legacy_gamma")`, but the path the actual renderers and node graph use does not: `From<GradientRamp> for Item` writes the attribute only when non-default (`if !is_default()`), so both a newly created Linear ramp and every legacy gamma ramp leave the attribute absent, and both resolve to Linear. As a result, gradients in documents saved before this field existed will flip from gamma to linear rendering the moment one of the renderer paths (renderer.rs lines ~511/2181/2279 or render_ext.rs ~99) reads them - an unintended visual change to existing artwork. Because the attribute form can't distinguish "new Linear" from "legacy gamma", the fix needs a coordinated change (e.g. always write the attribute, or default the item read to legacy gamma and migrate), not just changing this one line.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| while let Some(element) = next { | ||
| let from_style = element.attribute("style").and_then(|style| { | ||
| style.split(';').find_map(|declaration| { |
There was a problem hiding this comment.
P2: SVG gradients with repeated color-interpolation declarations in an inline style can import with the wrong interpolation space because the resolver keeps the first declaration; selecting the last applicable declaration would match the CSS cascade.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, line 575:
<comment>SVG gradients with repeated `color-interpolation` declarations in an inline style can import with the wrong interpolation space because the resolver keeps the first declaration; selecting the last applicable declaration would match the CSS cascade.</comment>
<file context>
@@ -523,6 +525,72 @@ fn usvg_transform(c: usvg::Transform) -> DAffine2 {
+
+ while let Some(element) = next {
+ let from_style = element.attribute("style").and_then(|style| {
+ style.split(';').find_map(|declaration| {
+ let (property, value) = declaration.split_once(':')?;
+ (property.trim() == "color-interpolation").then(|| value.trim())
</file context>
| let gradient_spread = convert_gradient_spread(linear.spread_method()); | ||
| modify_inputs.fill_gradient_set(gradient, gradient_form, gradient_spread, transform); | ||
| // SVG interpolates between stops in gamma sRGB unless `color-interpolation` opts into linearRGB, carried explicitly rather than as the linear default | ||
| let gradient_interpolation = gradient_info.interpolations.get(linear.id()).copied().unwrap_or(GradientInterpolation::SrgbGamma); |
There was a problem hiding this comment.
P2: SVG import now resolves each gradient's interpolation from its inline style attribute and its color-interpolation presentation attribute, but it does not read CSS <style> blocks. Many exported SVGs (Illustrator/Inkscape) set color-interpolation: linearRGB through grouped stylesheet rules, and those gradients would fall through to the SrgbGamma default on import, silently discarding the author's declared linear interpolation. Consider also scanning <style> elements (and matching the selectors, or at least any color-interpolation declared there) so the import honors them rather than silently defaulting to gamma.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, line 914:
<comment>SVG import now resolves each gradient's interpolation from its inline `style` attribute and its `color-interpolation` presentation attribute, but it does not read CSS `<style>` blocks. Many exported SVGs (Illustrator/Inkscape) set `color-interpolation: linearRGB` through grouped stylesheet rules, and those gradients would fall through to the `SrgbGamma` default on import, silently discarding the author's declared linear interpolation. Consider also scanning `<style>` elements (and matching the selectors, or at least any `color-interpolation` declared there) so the import honors them rather than silently defaulting to gamma.</comment>
<file context>
@@ -842,7 +910,9 @@ fn apply_usvg_fill(fill: &usvg::Fill, modify_inputs: &mut ModifyInputsContext, g
let gradient_spread = convert_gradient_spread(linear.spread_method());
- modify_inputs.fill_gradient_set(gradient, gradient_form, gradient_spread, transform);
+ // SVG interpolates between stops in gamma sRGB unless `color-interpolation` opts into linearRGB, carried explicitly rather than as the linear default
+ let gradient_interpolation = gradient_info.interpolations.get(linear.id()).copied().unwrap_or(GradientInterpolation::SrgbGamma);
+ modify_inputs.fill_gradient_set(gradient, gradient_form, gradient_spread, gradient_interpolation, transform);
}
</file context>
| Self { | ||
| stops: item.element().into(), | ||
| gradient_spread: item.attribute_cloned_or_default(ATTR_GRADIENT_SPREAD), | ||
| gradient_interpolation: item.attribute_cloned_or_default(ATTR_GRADIENT_INTERPOLATION), |
There was a problem hiding this comment.
P2: The item/attribute read path resolves a missing gradient_interpolation to Default::default() = SrgbLinear, which contradicts the design intent (and the doc comment on this struct) that the field's absence marks a pre-existing ramp and should read as the legacy SrgbGamma. The serde exchange form honors this via serde(default = "GradientInterpolation::legacy_gamma"), but the path the actual renderers and node graph use does not: From<GradientRamp> for Item writes the attribute only when non-default (if !is_default()), so both a newly created Linear ramp and every legacy gamma ramp leave the attribute absent, and both resolve to Linear. As a result, gradients in documents saved before this field existed will flip from gamma to linear rendering the moment one of the renderer paths (renderer.rs lines ~511/2181/2279 or render_ext.rs ~99) reads them - an unintended visual change to existing artwork. Because the attribute form can't distinguish "new Linear" from "legacy gamma", the fix needs a coordinated change (e.g. always write the attribute, or default the item read to legacy gamma and migrate), not just changing this one line.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/gradient.rs, line 179:
<comment>The item/attribute read path resolves a missing `gradient_interpolation` to `Default::default()` = `SrgbLinear`, which contradicts the design intent (and the doc comment on this struct) that the field's absence marks a pre-existing ramp and should read as the legacy `SrgbGamma`. The serde exchange form honors this via `serde(default = "GradientInterpolation::legacy_gamma")`, but the path the actual renderers and node graph use does not: `From<GradientRamp> for Item` writes the attribute only when non-default (`if !is_default()`), so both a newly created Linear ramp and every legacy gamma ramp leave the attribute absent, and both resolve to Linear. As a result, gradients in documents saved before this field existed will flip from gamma to linear rendering the moment one of the renderer paths (renderer.rs lines ~511/2181/2279 or render_ext.rs ~99) reads them - an unintended visual change to existing artwork. Because the attribute form can't distinguish "new Linear" from "legacy gamma", the fix needs a coordinated change (e.g. always write the attribute, or default the item read to legacy gamma and migrate), not just changing this one line.</comment>
<file context>
@@ -166,6 +176,7 @@ impl From<&Item<Gradient>> for GradientRamp {
Self {
stops: item.element().into(),
gradient_spread: item.attribute_cloned_or_default(ATTR_GRADIENT_SPREAD),
+ gradient_interpolation: item.attribute_cloned_or_default(ATTR_GRADIENT_INTERPOLATION),
}
}
</file context>
Next comes the perceptual modes, at which point sRGB Linear won't be the default anymore, but this at least sets the stage for having a non-gamma default.