Fix SVG even-odd fill rule imports (#4332)

editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs

@@ -13,7 +13,7 @@ use graph_craft::document::{NodeId, NodeInput};
 use graphene_std::list::List;
 use graphene_std::renderer::convert_usvg_path::convert_usvg_path;
 use graphene_std::text::{Font, TypesettingConfig};
-use graphene_std::vector::style::{GradientSpreadMethod, GradientStop, GradientStops, GradientType, PaintOrder, Stroke, StrokeAlign, StrokeCap, StrokeJoin};
+use graphene_std::vector::style::{FillRule, GradientSpreadMethod, GradientStop, GradientStops, GradientType, PaintOrder, Stroke, StrokeAlign, StrokeCap, StrokeJoin};
 use graphene_std::{Artboard, Color};
 
 #[derive(ExtractField)]
@@ -807,6 +807,13 @@ fn convert_spread_method(spread_method: usvg::SpreadMethod) -> GradientSpreadMet
 	}
 }
 
+fn convert_fill_rule(fill_rule: usvg::FillRule) -> FillRule {
+	match fill_rule {
+		usvg::FillRule::NonZero => FillRule::NonZero,
+		usvg::FillRule::EvenOdd => FillRule::EvenOdd,
+	}
+}
+
 fn apply_usvg_fill(fill: &usvg::Fill, modify_inputs: &mut ModifyInputsContext, graphite_gradient_stops: &HashMap<String, GradientStops>) {
 	match &fill.paint() {
 		usvg::Paint::Color(color) => modify_inputs.fill_color_set(Some(usvg_color(*color, fill.opacity().get()))),
@@ -860,4 +867,16 @@ fn apply_usvg_fill(fill: &usvg::Fill, modify_inputs: &mut ModifyInputsContext, g
 		}
 		usvg::Paint::Pattern(_) => warn!("SVG patterns are not currently supported"),
 	};
+	modify_inputs.fill_rule_set(convert_fill_rule(fill.rule()));
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	#[test]
+	fn converts_usvg_fill_rules() {
+		assert_eq!(convert_fill_rule(usvg::FillRule::NonZero), FillRule::NonZero);
+		assert_eq!(convert_fill_rule(usvg::FillRule::EvenOdd), FillRule::EvenOdd);
+	}
 }

editor/src/messages/portfolio/document/graph_operation/utility_types.rs

@@ -15,7 +15,7 @@ use graphene_std::raster::BlendMode;
 use graphene_std::raster_types::Image;
 use graphene_std::subpath::Subpath;
 use graphene_std::text::{Font, TypesettingConfig};
-use graphene_std::vector::style::{GradientSpreadMethod, GradientType, Stroke};
+use graphene_std::vector::style::{FillRule, GradientSpreadMethod, GradientType, Stroke};
 use graphene_std::vector::{GradientStops, PointId, Vector, VectorModification, VectorModificationType};
 use graphene_std::{Artboard, Color, Graphic, NodeInputDecleration};
 
@@ -464,6 +464,14 @@ impl<'a> ModifyInputsContext<'a> {
 		self.set_input_with_refresh(input_connector, NodeInput::value(TaggedValue::Color(color), false), false);
 	}
 
+	pub fn fill_rule_set(&mut self, fill_rule: FillRule) {
+		let Some(fill_node_id) = self.existing_proto_node_id(graphene_std::vector_nodes::fill::IDENTIFIER, true) else {
+			return;
+		};
+		let input_connector = InputConnector::node(fill_node_id, graphene_std::vector::fill::FillRuleInput::INDEX);
+		self.set_input_with_refresh(input_connector, NodeInput::value(TaggedValue::FillRule(fill_rule), false), false);
+	}
+
 	pub fn fill_gradient_set(&mut self, gradient: GradientStops, gradient_type: GradientType, spread_method: GradientSpreadMethod, transform: DAffine2) {
 		let Some(fill_node_id) = self.existing_proto_node_id(graphene_std::vector_nodes::fill::IDENTIFIER, true) else {
 			return;

editor/src/messages/portfolio/document_migration.rs

@@ -15,7 +15,7 @@ use graphene_std::text::{TextAlign, TypesettingConfig};
 use graphene_std::transform::ScaleType;
 use graphene_std::uuid::NodeId;
 use graphene_std::vector::graphic_types;
-use graphene_std::vector::style::{PaintOrder, StrokeAlign};
+use graphene_std::vector::style::{FillRule, PaintOrder, StrokeAlign};
 use std::collections::HashMap;
 use std::f64::consts::PI;
 use std::ops::Range;
@@ -1573,7 +1573,7 @@ fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId],
 	}
 
 	// Upgrade the legacy 4-input Fill node (content, fill: Fill, _backup_color, _backup_gradient: Gradient) to the
-	// value-model 7-input shape (content, fill: generic paint list, _backup_color, _backup_gradient, _gradient_type, _spread_method, _transform).
+	// current value-model shape, including the default nonzero fill rule.
 	if reference == DefinitionIdentifier::ProtoNode(graphene_std::vector_nodes::fill::IDENTIFIER) && inputs_count == 4 {
 		let mut node_template = resolve_document_node_type(&reference)?.default_node_template();
 		document.network_interface.replace_implementation(node_id, network_path, &mut node_template);
@@ -1664,7 +1664,27 @@ fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId],
 			}
 		}
 
-		inputs_count = 7;
+		document
+			.network_interface
+			.set_input(&InputConnector::node(*node_id, 7), NodeInput::value(TaggedValue::FillRule(FillRule::NonZero), false), network_path);
+
+		inputs_count = 8;
+	}
+
+	// Upgrade Fill nodes that already use the 7-input value model. Fill rules were previously implicit and always nonzero.
+	if reference == DefinitionIdentifier::ProtoNode(graphene_std::vector_nodes::fill::IDENTIFIER) && inputs_count == 7 {
+		let mut node_template = resolve_document_node_type(&reference)?.default_node_template();
+		document.network_interface.replace_implementation(node_id, network_path, &mut node_template);
+		let old_inputs = document.network_interface.replace_inputs(node_id, network_path, &mut node_template)?;
+
+		for (index, input) in old_inputs.into_iter().take(7).enumerate() {
+			document.network_interface.set_input(&InputConnector::node(*node_id, index), input, network_path);
+		}
+		document
+			.network_interface
+			.set_input(&InputConnector::node(*node_id, 7), NodeInput::value(TaggedValue::FillRule(FillRule::NonZero), false), network_path);
+
+		inputs_count = 8;
 	}
 
 	// Upgrade Stroke node to reorder parameters and add "Align" and "Paint Order" (#2644)

node-graph/graph-craft/src/document/value.rs

@@ -444,6 +444,7 @@ tagged_value! {
 	StrokeJoin(vector::style::StrokeJoin),
 	StrokeAlign(vector::style::StrokeAlign),
 	PaintOrder(vector::style::PaintOrder),
+	FillRule(vector::style::FillRule),
 	GradientType(vector::style::GradientType),
 	GradientSpreadMethod(vector::style::GradientSpreadMethod),
 	ReferencePoint(vector::ReferencePoint),

node-graph/interpreted-executor/src/node_registry.rs

@@ -140,6 +140,7 @@ fn node_registry() -> HashMap<ProtoNodeIdentifier, HashMap<NodeIOTypes, NodeCons
 		async_node!(graphene_core::memo::MonitorNode<_, _, _>, input: Context, fn_params: [Context => graphene_std::vector::style::StrokeCap]),
 		async_node!(graphene_core::memo::MonitorNode<_, _, _>, input: Context, fn_params: [Context => graphene_std::vector::style::StrokeJoin]),
 		async_node!(graphene_core::memo::MonitorNode<_, _, _>, input: Context, fn_params: [Context => graphene_std::vector::style::PaintOrder]),
+		async_node!(graphene_core::memo::MonitorNode<_, _, _>, input: Context, fn_params: [Context => graphene_std::vector::style::FillRule]),
 		async_node!(graphene_core::memo::MonitorNode<_, _, _>, input: Context, fn_params: [Context => graphene_std::vector::style::StrokeAlign]),
 		async_node!(graphene_core::memo::MonitorNode<_, _, _>, input: Context, fn_params: [Context => graphene_std::vector::style::Stroke]),
 		async_node!(graphene_core::memo::MonitorNode<_, _, _>, input: Context, fn_params: [Context => Box<graphene_std::vector::VectorModification>]),
@@ -279,6 +280,7 @@ fn node_registry() -> HashMap<ProtoNodeIdentifier, HashMap<NodeIOTypes, NodeCons
 		async_node!(graphene_core::memo::MemoizeNode<_, _>, input: Context, fn_params: [Context => graphene_std::vector::style::StrokeJoin]),
 		async_node!(graphene_core::memo::MemoizeNode<_, _>, input: Context, fn_params: [Context => graphene_std::vector::style::StrokeAlign]),
 		async_node!(graphene_core::memo::MemoizeNode<_, _>, input: Context, fn_params: [Context => graphene_std::vector::style::PaintOrder]),
+		async_node!(graphene_core::memo::MemoizeNode<_, _>, input: Context, fn_params: [Context => graphene_std::vector::style::FillRule]),
 		async_node!(graphene_core::memo::MemoizeNode<_, _>, input: Context, fn_params: [Context => graphene_std::vector::style::GradientType]),
 		async_node!(graphene_core::memo::MemoizeNode<_, _>, input: Context, fn_params: [Context => graphene_std::transform::ReferencePoint]),
 		async_node!(graphene_core::memo::MemoizeNode<_, _>, input: Context, fn_params: [Context => graphene_std::vector::misc::CentroidType]),

node-graph/libraries/core-types/src/list.rs

@@ -60,6 +60,8 @@ pub const ATTR_SPREAD_METHOD: &str = "spread_method";
 pub const ATTR_GRADIENT_TYPE: &str = "gradient_type";
 /// Vector graphics object's filled area paint, of type List<T> where T is any graphic type.
 pub const ATTR_FILL: &str = "fill";
+/// Vector graphics object's fill rule (`FillRule`, implicit default `NonZero`).
+pub const ATTR_FILL_RULE: &str = "fill_rule";
 /// Vector graphics object's stroke paint, of type List<T> where T is any graphic type.
 pub const ATTR_STROKE: &str = "stroke";
 /// Text item's font size in document-space units (`f64`, implicit default `24.`).

node-graph/libraries/rendering/src/renderer.rs

@@ -7,7 +7,7 @@ use core_types::bounds::RenderBoundingBox;
 use core_types::color::Color;
 use core_types::color::SRGBA8;
 use core_types::consts::DEFAULT_FONT_SIZE;
-use core_types::list::{ATTR_FILL, ATTR_STROKE, Item, List};
+use core_types::list::{ATTR_FILL, ATTR_FILL_RULE, ATTR_STROKE, Item, List};
 use core_types::math::quad::Quad;
 use core_types::render_complexity::RenderComplexity;
 use core_types::transform::Footprint;
@@ -26,7 +26,7 @@ use graphic_types::raster_types::{BitmapMut, CPU, GPU, Image, Raster, Texture};
 use graphic_types::vector_types::gradient::{GradientStops, GradientType};
 use graphic_types::vector_types::subpath::Subpath;
 use graphic_types::vector_types::vector::click_target::{ClickTarget, FreePoint};
-use graphic_types::vector_types::vector::style::{PaintOrder, RenderMode, StrokeAlign, StrokeCap, StrokeJoin};
+use graphic_types::vector_types::vector::style::{FillRule, PaintOrder, RenderMode, StrokeAlign, StrokeCap, StrokeJoin};
 use graphic_types::{Artboard, Graphic, Vector};
 use kurbo::{Affine, BezPath, Cap, Join, Shape, StrokeOpts};
 use num_traits::Zero;
@@ -352,6 +352,7 @@ fn emit_svg_fill_path(
 	render: &mut SvgRender,
 	d: String,
 	fill_graphic_list: Option<&List<Graphic>>,
+	fill_rule: FillRule,
 	item_transform: DAffine2,
 	element_transform: DAffine2,
 	applied_stroke_transform: DAffine2,
@@ -369,9 +370,21 @@ fn emit_svg_fill_path(
 			.map(|list| list.render(defs, item_transform, element_transform, applied_stroke_transform, bounds_matrix, render_params, PaintTarget::Fill))
 			.unwrap_or_else(|| r#" fill="none""#.to_string());
 		attributes.push_val(fill_attribute);
+		if fill_rule == FillRule::EvenOdd {
+			attributes.push("fill-rule", "evenodd");
+			attributes.push("clip-rule", "evenodd");
+		}
 	});
 }
 
+fn effective_fill_rule(vector: &Vector, requested: FillRule) -> FillRule {
+	if requested == FillRule::EvenOdd || (vector.is_branching() && !vector.use_face_fill()) {
+		FillRule::EvenOdd
+	} else {
+		FillRule::NonZero
+	}
+}
+
 /// Whether the affine transform inverts to a finite matrix (a zero, subnormal, or NaN determinant does not).
 pub(crate) fn transform_is_invertible(transform: DAffine2) -> bool {
 	transform.matrix2.determinant().recip().is_finite()
@@ -1063,6 +1076,8 @@ impl Render for List<Vector> {
 			let blend_mode_attr: BlendMode = self.attribute_cloned_or_default(ATTR_BLEND_MODE, index);
 			let opacity_attr: f64 = self.attribute_cloned_or(ATTR_OPACITY, index, 1.);
 			let opacity_fill_attr: f64 = self.attribute_cloned_or(ATTR_OPACITY_FILL, index, 1.);
+			let requested_fill_rule: FillRule = self.attribute_cloned_or_default(ATTR_FILL_RULE, index);
+			let fill_rule = effective_fill_rule(vector, requested_fill_rule);
 
 			// Only consider strokes with non-zero weight, since default strokes with zero weight would prevent assigning the correct stroke transform
 			let has_real_stroke = vector.stroke.as_ref().filter(|stroke| stroke.weight() > 0.);
@@ -1113,6 +1128,7 @@ impl Render for List<Vector> {
 					render,
 					path.clone(),
 					fill_graphic_list.as_deref(),
+					fill_rule,
 					item_transform,
 					element_transform,
 					applied_stroke_transform,
@@ -1129,7 +1145,9 @@ impl Render for List<Vector> {
 
 				// The mask must draw at full alpha so the SVG `<mask>`/`<clipPath>` fully zeroes the path interior.
 				// The wrapping SVG group (above) handles the user-set opacity.
-				let mut mask_item = Item::new_from_element(cloned_vector).with_attribute(ATTR_TRANSFORM, item_transform);
+				let mut mask_item = Item::new_from_element(cloned_vector)
+					.with_attribute(ATTR_TRANSFORM, item_transform)
+					.with_attribute(ATTR_FILL_RULE, fill_rule);
 				set_paint_attribute(mask_item.attributes_mut(), ATTR_FILL, List::new_from_element(Color::BLACK));
 				let vector_item = List::new_from_item(mask_item);
 
@@ -1145,6 +1163,7 @@ impl Render for List<Vector> {
 						render,
 						face_d,
 						fill_graphic_list.as_deref(),
+						FillRule::NonZero,
 						item_transform,
 						element_transform,
 						applied_stroke_transform,
@@ -1240,7 +1259,7 @@ impl Render for List<Vector> {
 				attributes.push_val(stroke_shape_attribute);
 				attributes.push_val(stroke_attribute);
 
-				if vector.is_branching() && !use_face_fill {
+				if fill_rule == FillRule::EvenOdd {
 					attributes.push("fill-rule", "evenodd");
 				}
 
@@ -1260,6 +1279,7 @@ impl Render for List<Vector> {
 					render,
 					path.clone(),
 					fill_graphic_list.as_deref(),
+					fill_rule,
 					item_transform,
 					element_transform,
 					applied_stroke_transform,
@@ -1279,6 +1299,8 @@ impl Render for List<Vector> {
 			let blend_mode_attr: BlendMode = self.attribute_cloned_or_default(ATTR_BLEND_MODE, index);
 			let opacity_attr: f64 = self.attribute_cloned_or(ATTR_OPACITY, index, 1.);
 			let opacity_fill_attr: f64 = self.attribute_cloned_or(ATTR_OPACITY_FILL, index, 1.);
+			let requested_fill_rule: FillRule = self.attribute_cloned_or_default(ATTR_FILL_RULE, index);
+			let fill_rule = effective_fill_rule(element, requested_fill_rule);
 			let multiplied_transform = parent_transform * item_transform;
 			let has_real_stroke = element.stroke.as_ref().filter(|stroke| stroke.weight() > 0.);
 			let set_stroke_transform = has_real_stroke.map(|stroke| stroke.transform).filter(|transform| transform_is_invertible(*transform));
@@ -1390,10 +1412,12 @@ impl Render for List<Vector> {
 						}
 						do_fill_path(scene, context, &kurbo_path, peniko::Fill::NonZero);
 					}
-				} else if element.is_branching() {
-					do_fill_path(scene, context, &path, peniko::Fill::EvenOdd);
 				} else {
-					do_fill_path(scene, context, &path, peniko::Fill::NonZero);
+					let fill_rule = match fill_rule {
+						FillRule::NonZero => peniko::Fill::NonZero,
+						FillRule::EvenOdd => peniko::Fill::EvenOdd,
+					};
+					do_fill_path(scene, context, &path, fill_rule);
 				}
 			};
 
@@ -1476,7 +1500,9 @@ impl Render for List<Vector> {
 
 						// The mask must draw at full alpha so `SrcOut` fully zeroes the path interior.
 						// The outer opacity/blend layer (above) handles the user-set opacity.
-						let mut mask_item = Item::new_from_element(cloned_element).with_attribute(ATTR_TRANSFORM, item_transform);
+						let mut mask_item = Item::new_from_element(cloned_element)
+							.with_attribute(ATTR_TRANSFORM, item_transform)
+							.with_attribute(ATTR_FILL_RULE, fill_rule);
 						set_paint_attribute(mask_item.attributes_mut(), ATTR_FILL, List::new_from_element(Color::BLACK));
 						let vector_list = List::new_from_item(mask_item);
 
@@ -2634,6 +2660,34 @@ impl RenderSvgSegmentList for Vec<SvgSegment> {
 	}
 }
 
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	fn render_compound_rectangles(fill_rule: FillRule) -> String {
+		let vector = Vector::from_subpaths(
+			[Subpath::new_rectangle(DVec2::ZERO, DVec2::splat(100.)), Subpath::new_rectangle(DVec2::splat(25.), DVec2::splat(75.))],
+			false,
+		);
+		let fill = List::new_from_element(Graphic::Color(List::new_from_element(Color::BLACK)));
+		let item = Item::new_from_element(vector).with_attribute(ATTR_FILL, fill).with_attribute(ATTR_FILL_RULE, fill_rule);
+		let vectors = List::new_from_item(item);
+
+		let mut render = SvgRender::new();
+		vectors.render_svg(&mut render, &RenderParams::default());
+		render.svg.to_svg_string()
+	}
+
+	#[test]
+	fn svg_renderer_emits_explicit_even_odd_fill_rule() {
+		let even_odd = render_compound_rectangles(FillRule::EvenOdd);
+		assert!(even_odd.contains(r#"fill-rule="evenodd""#), "rendered SVG: {even_odd}");
+
+		let nonzero = render_compound_rectangles(FillRule::NonZero);
+		assert!(!nonzero.contains(r#"fill-rule="evenodd""#), "rendered SVG: {nonzero}");
+	}
+}
+
 pub struct SvgRenderAttrs<'a>(&'a mut SvgRender);
 
 impl SvgRenderAttrs<'_> {

node-graph/libraries/vector-types/src/vector/style.rs

@@ -106,6 +106,20 @@ impl FillChoice {
 	}
 }
 
+/// Determines how overlapping subpaths contribute to a vector's filled area.
+#[repr(C)]
+#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
+#[derive(Default, Debug, Clone, Copy, PartialEq, Eq, Hash, graphene_hash::CacheHash, DynAny, node_macro::ChoiceType)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[widget(Radio)]
+pub enum FillRule {
+	/// A point is filled when the signed winding count around it is nonzero.
+	#[default]
+	NonZero,
+	/// A point is filled when a ray from it crosses the path an odd number of times.
+	EvenOdd,
+}
+
 /// The stroke (outline) style of an SVG element.
 #[repr(C)]
 #[cfg_attr(feature = "wasm", derive(tsify::Tsify))]

node-graph/nodes/vector/src/vector_nodes.rs

@@ -3,7 +3,7 @@ use core::f64::consts::{PI, TAU};
 use core::hash::{Hash, Hasher};
 use core_types::blending::BlendMode;
 use core_types::bounds::{BoundingBox, RenderBoundingBox};
-use core_types::list::{ATTR_FILL, ATTR_STROKE, Item, ItemAttributeValues, List, ListDyn};
+use core_types::list::{ATTR_FILL, ATTR_FILL_RULE, ATTR_STROKE, Item, ItemAttributeValues, List, ListDyn};
 use core_types::registry::types::{Angle, Length, Multiplier, Percentage, PixelLength, Progression, SeedValue};
 use core_types::transform::{Footprint, Transform};
 use core_types::uuid::NodeId;
@@ -31,7 +31,7 @@ use vector_types::vector::misc::{
 	CentroidType, ExtrudeJoiningAlgorithm, HandleId, InterpolationDistribution, MergeByDistanceAlgorithm, PointSpacingType, RowsOrColumns, bezpath_from_manipulator_groups,
 	bezpath_to_manipulator_groups, handles_to_segment, is_linear, point_to_dvec2, segment_to_handles,
 };
-use vector_types::vector::style::{GradientStops, PaintOrder, Stroke, StrokeAlign, StrokeCap, StrokeJoin};
+use vector_types::vector::style::{FillRule, GradientStops, PaintOrder, Stroke, StrokeAlign, StrokeCap, StrokeJoin};
 use vector_types::vector::{FillId, PointId, RegionId, SegmentDomain, SegmentId, StrokeId, VectorExt};
 use vector_types::{GradientSpreadMethod, GradientType};
 
@@ -176,6 +176,8 @@ async fn fill<V: VectorListIterMut + 'n + Send, F: IntoGraphicList + 'n + Send +
 	_gradient_type: GradientType,
 	_spread_method: GradientSpreadMethod,
 	_transform: Option<DAffine2>,
+	/// The rule used to determine which areas of overlapping subpaths are filled.
+	fill_rule: FillRule,
 ) -> V {
 	if let Some(gradient) = (&mut fill as &mut dyn std::any::Any).downcast_mut::<List<GradientStops>>() {
 		if gradient.iter_attribute_values::<GradientType>(ATTR_GRADIENT_TYPE).is_none() {
@@ -226,6 +228,9 @@ async fn fill<V: VectorListIterMut + 'n + Send, F: IntoGraphicList + 'n + Send +
 		for slot in vector_list.iter_attribute_values_mut_or_default::<List<Graphic>>(ATTR_FILL) {
 			*slot = fill.clone();
 		}
+		for slot in vector_list.iter_attribute_values_mut_or_default::<FillRule>(ATTR_FILL_RULE) {
+			*slot = fill_rule;
+		}
 	});
 	content
 }
@@ -3290,6 +3295,25 @@ mod test {
 		Item::new_from_element(row).with_attribute(ATTR_TRANSFORM, transform)
 	}
 
+	#[tokio::test]
+	async fn fill_sets_fill_rule_attribute() {
+		let content = vector_node_from_bezpath(Rect::new(0., 0., 100., 100.).to_path(DEFAULT_ACCURACY));
+		let result = super::fill(
+			(),
+			content,
+			List::new_from_element(Color::BLACK),
+			List::new_from_element(Color::BLACK),
+			List::default(),
+			GradientType::Linear,
+			GradientSpreadMethod::Pad,
+			None,
+			FillRule::EvenOdd,
+		)
+		.await;
+
+		assert_eq!(result.attribute_cloned_or_default::<FillRule>(ATTR_FILL_RULE, 0), FillRule::EvenOdd);
+	}
+
 	#[tokio::test]
 	async fn bounding_box() {
 		let bounding_box = super::bounding_box((), vector_node_from_bezpath(Rect::new(-1., -1., 1., 1.).to_path(DEFAULT_ACCURACY))).await;