-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Replace TaggedValue tabular variants for color and gradient with non-tabular forms #4038
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1986,6 +1986,31 @@ fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId], | |||||||||||||||||||||||||||||||||||||||
| .set_input(&InputConnector::node(*node_id, 1), NodeInput::value(TaggedValue::ImageData(image), false), network_path); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Migrate Color Value, Fill backup_color, and Stroke color inputs from Color(Table<Color>) to OptionalColor(Option<Color>) | ||||||||||||||||||||||||||||||||||||||||
| let color_to_optional_cases: &[(_, usize)] = &[ | ||||||||||||||||||||||||||||||||||||||||
| (graphene_std::math_nodes::color_value::IDENTIFIER, 1), | ||||||||||||||||||||||||||||||||||||||||
| (graphene_std::vector_nodes::fill::IDENTIFIER, 2), | ||||||||||||||||||||||||||||||||||||||||
| (graphene_std::vector::stroke::IDENTIFIER, 1), | ||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||
| for &(ref identifier, input_index) in color_to_optional_cases { | ||||||||||||||||||||||||||||||||||||||||
| if reference == DefinitionIdentifier::ProtoNode(identifier.clone()) | ||||||||||||||||||||||||||||||||||||||||
| && let Some(NodeInput::Value { tagged_value, .. }) = node.inputs.get(input_index) | ||||||||||||||||||||||||||||||||||||||||
| && let TaggedValue::Color(color) = &**tagged_value | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| let color = *color; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| document.network_interface.set_input( | ||||||||||||||||||||||||||||||||||||||||
| &InputConnector::node(*node_id, input_index), | ||||||||||||||||||||||||||||||||||||||||
| NodeInput::value(TaggedValue::OptionalColor(Some(color)), false), | ||||||||||||||||||||||||||||||||||||||||
| network_path, | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1997
to
+2005
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The color migration forcibly sets Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Migrate Artboard and Empty Image color inputs from Color(Table<Color>) to Color(Color), which is handled by serde migration already (no-op here) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Migrate GradientTable(Table<GradientStops>) to GradientStops(GradientStops), which is handled by serde migration via alias (no-op here) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // ================================== | ||||||||||||||||||||||||||||||||||||||||
| // PUT ALL MIGRATIONS ABOVE THIS LINE | ||||||||||||||||||||||||||||||||||||||||
| // ================================== | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,9 +118,9 @@ macro_rules! tagged_value { | |
| // Tries using the default for the tagged value type. If it not implemented, then uses the default used in document_node_types. If it is not used there, then TaggedValue::None is returned. | ||
| Some(match concrete_type.id? { | ||
| x if x == TypeId::of::<()>() => TaggedValue::None, | ||
| // Table-wrapped types need a single-row default with the element's default, not an empty table | ||
| x if x == TypeId::of::<Table<Color>>() => TaggedValue::Color(Table::new_from_element(Color::default())), | ||
| x if x == TypeId::of::<Table<GradientStops>>() => TaggedValue::GradientTable(Table::new_from_element(GradientStops::default())), | ||
| x if x == TypeId::of::<Table<Color>>() => TaggedValue::OptionalColor(Some(Color::default())), | ||
| x if x == TypeId::of::<Option<Color>>() => TaggedValue::OptionalColor(None), | ||
| x if x == TypeId::of::<Table<GradientStops>>() => TaggedValue::GradientStops(GradientStops::default()), | ||
| $( x if x == TypeId::of::<$ty>() => TaggedValue::$identifier(Default::default()), )* | ||
| _ => return None, | ||
| }) | ||
|
|
@@ -202,14 +202,16 @@ tagged_value! { | |
| #[serde(alias = "ArtboardGroup")] | ||
| Artboard(Table<Artboard>), | ||
| #[serde(deserialize_with = "core_types::misc::migrate_color")] // TODO: Eventually remove this migration document upgrade code | ||
| #[serde(alias = "ColorTable", alias = "OptionalColor", alias = "ColorNotInTable")] | ||
| Color(Table<Color>), | ||
| #[serde(deserialize_with = "graphic_types::vector_types::gradient::migrate_gradient_stops")] // TODO: Eventually remove this migration document upgrade code | ||
| #[serde(alias = "GradientPositions", alias = "GradientStops")] | ||
| GradientTable(Table<GradientStops>), | ||
| // ============ | ||
| // STRUCT TYPES | ||
| // ============ | ||
| #[serde(alias = "ColorTable", alias = "ColorNotInTable")] | ||
| Color(Color), | ||
| #[serde(deserialize_with = "core_types::misc::migrate_optional_color")] // TODO: Eventually remove this migration document upgrade code | ||
| OptionalColor(Option<Color>), | ||
| #[serde(deserialize_with = "graphic_types::vector_types::gradient::migrate_gradient_stops")] // TODO: Eventually remove this migration document upgrade code | ||
| #[serde(alias = "GradientPositions", alias = "GradientTable")] | ||
| GradientStops(GradientStops), | ||
|
Comment on lines
+209
to
+214
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing the Since the
|
||
| FVec2(Vec2), | ||
| FAffine2(Affine2), | ||
| #[serde(alias = "IVec2", alias = "UVec2")] | ||
|
|
@@ -397,10 +399,9 @@ impl TaggedValue { | |
| () if ty == TypeId::of::<u32>() => FromStr::from_str(string).map(TaggedValue::U32).ok()?, | ||
| () if ty == TypeId::of::<DVec2>() => to_dvec2(string).map(TaggedValue::DVec2)?, | ||
| () if ty == TypeId::of::<bool>() => FromStr::from_str(string).map(TaggedValue::Bool).ok()?, | ||
| // `Color` (not in a table) is still currently needed by `BlackAndWhiteNode` and `ColorOverlayNode` GPU `shader_node(PerPixelAdjust)` variants | ||
| () if ty == TypeId::of::<Color>() => to_color(string).map(|color| TaggedValue::Color(Table::new_from_element(color)))?, | ||
| () if ty == TypeId::of::<Table<Color>>() => to_color(string).map(|color| TaggedValue::Color(Table::new_from_element(color)))?, | ||
| () if ty == TypeId::of::<Table<GradientStops>>() => to_gradient(string).map(|color| TaggedValue::GradientTable(Table::new_from_element(color)))?, | ||
| () if ty == TypeId::of::<Color>() => to_color(string).map(TaggedValue::Color)?, | ||
| () if ty == TypeId::of::<Option<Color>>() => TaggedValue::OptionalColor(to_color(string)), | ||
| () if ty == TypeId::of::<GradientStops>() => to_gradient(string).map(TaggedValue::GradientStops)?, | ||
| () if ty == TypeId::of::<Fill>() => to_color(string).map(|color| TaggedValue::Fill(Fill::solid(color)))?, | ||
| () if ty == TypeId::of::<ReferencePoint>() => to_reference_point(string).map(TaggedValue::ReferencePoint)?, | ||
| _ => return None, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of
Table<Color>andTable<GradientStops>from theTypeIdmatching inproperty_from_typecauses a UI regression. Many nodes in the graph still return these tabular types (e.g.,color_value,rgba_to_color,sample_gradient). For these nodes, the Properties panel will now fail to display a color picker widget and instead show a fallback message indicating that no widget exists for the type.These cases should be restored to maintain UI compatibility with nodes that still produce tabular return types. Note that
color_widgetwill still work correctly becauseTaggedValue::from_type(used for initializing widgets) already maps these tabular types to the newOptionalColorandGradientStopsvariants.