Sample Points node: fix major inefficiencies

libraries/bezier-rs/src/bezier/lookup.rs

@@ -6,24 +6,52 @@ use super::*;
 impl Bezier {
 	/// Convert a euclidean distance ratio along the `Bezier` curve to a parametric `t`-value.
 	pub fn euclidean_to_parametric(&self, ratio: f64, error: f64) -> f64 {
-		if ratio < error {
+		let total_length = self.length(None);
+		self.euclidean_to_parametric_with_total_length(ratio, error, total_length)
+	}
+
+	/// Convert a euclidean distance ratio along the `Bezier` curve to a parametric `t`-value.
+	/// For performance reasons, this version of the [`euclidean_to_parametric`] function allows the caller to
+	/// provide the total length of the curve so it doesn't have to be calculated every time the function is called.
+	pub fn euclidean_to_parametric_with_total_length(&self, euclidean_t: f64, error: f64, total_length: f64) -> f64 {
+		if euclidean_t < error {
 			return 0.;
 		}
-		if 1. - ratio < error {
+		if 1. - euclidean_t < error {
 			return 1.;
 		}
 
 		let mut low = 0.;
-		let mut mid = 0.;
+		let mut mid = 0.5;
 		let mut high = 1.;
-		let total_length = self.length(None);
 
+		// The euclidean t-value input generally correlates with the parametric t-value result.
+		// So we can assume a low t-value has a short length from the start of the curve, and a high t-value has a short length from the end of the curve.
+		// We'll use a strategy where we measure from either end of the curve depending on which side is closer than thus more likely to be proximate to the sought parametric t-value.
+		// This allows us to use fewer segments to approximate the curve, which usually won't go much beyond half the curve.
+		let result_likely_closer_to_start = euclidean_t < 0.5;
+		// If the curve is near either end, we need even fewer segments to approximate the curve with reasonable accuracy.
+		// A point that's likely near the center is the worst case where we need to use up to half the predefined number of max subdivisions.
+		let subdivisions_proportional_to_likely_length = ((euclidean_t - 0.5).abs() * DEFAULT_LENGTH_SUBDIVISIONS as f64).round().max(1.) as usize;
+
+		// Binary search for the parametric t-value that corresponds to the euclidean distance ratio by trimming the curve between the start and the tested parametric t-value during each iteration of the search.
 		while low < high {
 			mid = (low + high) / 2.;
-			let test_ratio = self.trim(TValue::Parametric(0.), TValue::Parametric(mid)).length(None) / total_length;
-			if f64_compare(test_ratio, ratio, error) {
+
+			// We can search from the curve start to the sought point, or from the sought point to the curve end, depending on which side is likely closer to the result.
+			let current_length = if result_likely_closer_to_start {
+				let trimmed = self.trim(TValue::Parametric(0.), TValue::Parametric(mid));
+				trimmed.length(Some(subdivisions_proportional_to_likely_length))
+			} else {
+				let trimmed = self.trim(TValue::Parametric(mid), TValue::Parametric(1.));
+				let trimmed_length = trimmed.length(Some(subdivisions_proportional_to_likely_length));
+				total_length - trimmed_length
+			};
+			let current_euclidean_t = current_length / total_length;
+
+			if f64_compare(current_euclidean_t, euclidean_t, error) {
 				break;
-			} else if test_ratio < ratio {
+			} else if current_euclidean_t < euclidean_t {
 				low = mid;
 			} else {
 				high = mid;
@@ -101,22 +129,14 @@ impl Bezier {
 	/// <iframe frameBorder="0" width="100%" height="300px" src="https://graphite.rs/libraries/bezier-rs#bezier/length/solo" title="Length Demo"></iframe>
 	pub fn length(&self, num_subdivisions: Option<usize>) -> f64 {
 		match self.handles {
-			BezierHandles::Linear => self.start.distance(self.end),
+			BezierHandles::Linear => (self.start - self.end).length(),
 			_ => {
 				// Code example from <https://gamedev.stackexchange.com/questions/5373/moving-ships-between-two-planets-along-a-bezier-missing-some-equations-for-acce/5427#5427>.
 
 				// We will use an approximate approach where we split the curve into many subdivisions
 				// and calculate the euclidean distance between the two endpoints of the subdivision
 				let lookup_table = self.compute_lookup_table(Some(num_subdivisions.unwrap_or(DEFAULT_LENGTH_SUBDIVISIONS)), Some(TValueType::Parametric));
-				let mut approx_curve_length = 0.;
-				let mut previous_point = lookup_table[0];
-				// Calculate approximate distance between subdivision
-				for current_point in lookup_table.iter().skip(1) {
-					// Calculate distance of subdivision
-					approx_curve_length += (*current_point - previous_point).length();
-					// Update the previous point
-					previous_point = *current_point;
-				}
+				let approx_curve_length: f64 = lookup_table.windows(2).map(|points| (points[1] - points[0]).length()).sum();
 
 				approx_curve_length
 			}

libraries/bezier-rs/src/subpath/lookup.rs

@@ -28,13 +28,13 @@ impl<ManipulatorGroupId: crate::Identifier> Subpath<ManipulatorGroupId> {
 	/// - `num_subdivisions` - Number of subdivisions used to approximate the curve. The default value is `1000`.
 	/// <iframe frameBorder="0" width="100%" height="300px" src="https://graphite.rs/libraries/bezier-rs#subpath/length/solo" title="Length Demo"></iframe>
 	pub fn length(&self, num_subdivisions: Option<usize>) -> f64 {
-		self.iter().fold(0., |accumulator, bezier| accumulator + bezier.length(num_subdivisions))
+		self.iter().map(|bezier| bezier.length(num_subdivisions)).sum()
 	}
 
-	fn global_euclidean_to_local_euclidean(&self, global_t: f64) -> (usize, f64) {
-		let lengths = self.iter().map(|bezier| bezier.length(None)).collect::<Vec<f64>>();
-		let total_length: f64 = lengths.iter().sum();
-
+	/// Converts from a subpath (composed of multiple segments) to a point along a certain segment represented.
+	/// The returned tuple represents the segment index and the `t` value along that segment.
+	/// Both the input global `t` value and the output `t` value are in euclidean space, meaning there is a constant rate of change along the arc length.
+	pub fn global_euclidean_to_local_euclidean(&self, global_t: f64, lengths: &[f64], total_length: f64) -> (usize, f64) {
 		let mut accumulator = 0.;
 		for (index, length) in lengths.iter().enumerate() {
 			let length_ratio = length / total_length;
@@ -77,11 +77,11 @@ impl<ManipulatorGroupId: crate::Identifier> Subpath<ManipulatorGroupId> {
 				(segment_index, self.get_segment(segment_index).unwrap().euclidean_to_parametric(t, DEFAULT_EUCLIDEAN_ERROR_BOUND))
 			}
 			SubpathTValue::GlobalEuclidean(t) => {
-				let (segment_index, segment_t) = self.global_euclidean_to_local_euclidean(t);
-				(
-					segment_index,
-					self.get_segment(segment_index).unwrap().euclidean_to_parametric(segment_t, DEFAULT_EUCLIDEAN_ERROR_BOUND),
-				)
+				let lengths = self.iter().map(|bezier| bezier.length(None)).collect::<Vec<f64>>();
+				let total_length: f64 = lengths.iter().sum();
+				let (segment_index, segment_t_euclidean) = self.global_euclidean_to_local_euclidean(t, lengths.as_slice(), total_length);
+				let segment_t_parametric = self.get_segment(segment_index).unwrap().euclidean_to_parametric(segment_t_euclidean, DEFAULT_EUCLIDEAN_ERROR_BOUND);
+				(segment_index, segment_t_parametric)
 			}
 			SubpathTValue::EuclideanWithinError { segment_index, t, error } => {
 				assert!((0.0..=1.).contains(&t));
@@ -89,7 +89,9 @@ impl<ManipulatorGroupId: crate::Identifier> Subpath<ManipulatorGroupId> {
 				(segment_index, self.get_segment(segment_index).unwrap().euclidean_to_parametric(t, error))
 			}
 			SubpathTValue::GlobalEuclideanWithinError { t, error } => {
-				let (segment_index, segment_t) = self.global_euclidean_to_local_euclidean(t);
+				let lengths = self.iter().map(|bezier| bezier.length(None)).collect::<Vec<f64>>();
+				let total_length: f64 = lengths.iter().sum();
+				let (segment_index, segment_t) = self.global_euclidean_to_local_euclidean(t, lengths.as_slice(), total_length);
 				(segment_index, self.get_segment(segment_index).unwrap().euclidean_to_parametric(segment_t, error))
 			}
 		}

libraries/bezier-rs/src/subpath/solvers.rs

@@ -17,7 +17,7 @@ impl<ManipulatorGroupId: crate::Identifier> Subpath<ManipulatorGroupId> {
 
 	/// Calculates the intersection points the subpath has with a given curve and returns a list of `(usize, f64)` tuples,
 	/// where the `usize` represents the index of the curve in the subpath, and the `f64` represents the `t`-value local to
-	/// that curve where the intersection occured.
+	/// that curve where the intersection occurred.
 	/// Expects the following:
 	/// - `other`: a [Bezier] curve to check intersections against
 	/// - `error`: an optional f64 value to provide an error bound

libraries/bezier-rs/src/utils.rs

@@ -262,8 +262,8 @@ pub fn compute_circle_center_from_points(p1: DVec2, p2: DVec2, p3: DVec2) -> Opt
 }
 
 /// Compare two `f64` numbers with a provided max absolute value difference.
-pub fn f64_compare(f1: f64, f2: f64, max_abs_diff: f64) -> bool {
-	(f1 - f2).abs() < max_abs_diff
+pub fn f64_compare(a: f64, b: f64, max_abs_diff: f64) -> bool {
+	(a - b).abs() < max_abs_diff
 }
 
 /// Determine if an `f64` number is within a given range by using a max absolute value difference comparison.
@@ -272,8 +272,8 @@ pub fn f64_approximately_in_range(value: f64, min: f64, max: f64, max_abs_diff:
 }
 
 /// Compare the two values in a `DVec2` independently with a provided max absolute value difference.
-pub fn dvec2_compare(dv1: DVec2, dv2: DVec2, max_abs_diff: f64) -> BVec2 {
-	BVec2::new((dv1.x - dv2.x).abs() < max_abs_diff, (dv1.y - dv2.y).abs() < max_abs_diff)
+pub fn dvec2_compare(a: DVec2, b: DVec2, max_abs_diff: f64) -> BVec2 {
+	BVec2::new((a.x - b.x).abs() < max_abs_diff, (a.y - b.y).abs() < max_abs_diff)
 }
 
 /// Determine if the values in a `DVec2` are within a given range independently by using a max absolute value difference comparison.
@@ -323,8 +323,8 @@ mod tests {
 	use crate::consts::MAX_ABSOLUTE_DIFFERENCE;
 
 	/// Compare vectors of `f64`s with a provided max absolute value difference.
-	fn f64_compare_vector(vec1: Vec<f64>, vec2: Vec<f64>, max_abs_diff: f64) -> bool {
-		vec1.len() == vec2.len() && vec1.into_iter().zip(vec2).all(|(a, b)| f64_compare(a, b, max_abs_diff))
+	fn f64_compare_vector(a: Vec<f64>, b: Vec<f64>, max_abs_diff: f64) -> bool {
+		a.len() == b.len() && a.into_iter().zip(b).all(|(a, b)| f64_compare(a, b, max_abs_diff))
 	}
 
 	#[test]

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

@@ -5,9 +5,8 @@ use crate::transform::{Footprint, Transform, TransformMut};
 use crate::{Color, GraphicGroup, Node};
 use core::future::Future;
 
-use bezier_rs::{Subpath, SubpathTValue};
+use bezier_rs::{Subpath, TValue};
 use glam::{DAffine2, DVec2};
-use num_traits::Zero;
 
 #[derive(Debug, Clone, Copy)]
 pub struct SetFillNode<FillType, SolidColor, GradientType, Start, End, Transform, Positions> {
@@ -165,7 +164,7 @@ impl ConcatElement for VectorData {
 
 impl ConcatElement for GraphicGroup {
 	fn concat(&mut self, other: &Self, transform: DAffine2) {
-		// TODO: Decide if we want to keep this behaviour whereby the layers are flattened
+		// TODO: Decide if we want to keep this behavior whereby the layers are flattened
 		for mut element in other.iter().cloned() {
 			*element.transform_mut() = transform * element.transform() * other.transform();
 			self.push(element);
@@ -223,30 +222,40 @@ fn sample_points(mut vector_data: VectorData, spacing: f32, start_offset: f32, s
 		}
 
 		subpath.apply_transform(vector_data.transform);
-		let length = subpath.length(None);
-		let used_length = length - start_offset - stop_offset;
+
+		let segment_lengths = subpath.iter().map(|bezier| bezier.length(None)).collect::<Vec<f64>>();
+		let total_length: f64 = segment_lengths.iter().sum();
+
+		let mut used_length = total_length - start_offset - stop_offset;
 		if used_length <= 0. {
 			continue;
 		}
-		let used_length_without_remainder = used_length - used_length % spacing;
 
-		let count = if adaptive_spacing {
-			(used_length / spacing).round()
+		let count;
+		if adaptive_spacing {
+			count = (used_length / spacing).round();
 		} else {
-			(used_length / spacing + f64::EPSILON).floor()
-		};
+			count = (used_length / spacing + f64::EPSILON).floor();
+			used_length = used_length - used_length % spacing;
+		}
 
 		if count >= 1. {
 			let new_anchors = (0..=count as usize).map(|c| {
 				let ratio = c as f64 / count;
 
-				// With adaptive spacing, we widen or narrow the points (that's the rounding performed above) as necessary to ensure the last point is always at the end of the path
-				// Without adaptive spacing, we just evenly space the points at the exact specified spacing, usually falling short before the end of the path
+				// With adaptive spacing, we widen or narrow the points (that's the `round()` above) as necessary to ensure the last point is always at the end of the path.
+				// Without adaptive spacing, we just evenly space the points at the exact specified spacing, usually falling short (that's the `floor()` above) before the end of the path.
 
-				let used_length_here = if adaptive_spacing { used_length } else { used_length_without_remainder };
-				let t = ratio * used_length_here + start_offset;
-				subpath.evaluate(SubpathTValue::GlobalEuclidean(t / length))
+				let t = (ratio * used_length + start_offset) / total_length;
+
+				let (segment_index, segment_t_euclidean) = subpath.global_euclidean_to_local_euclidean(t, segment_lengths.as_slice(), total_length);
+				let segment_t_parametric = subpath
+					.get_segment(segment_index)
+					.unwrap()
+					.euclidean_to_parametric_with_total_length(segment_t_euclidean, 0.001, segment_lengths[segment_index]);
+				subpath.get_segment(segment_index).unwrap().evaluate(TValue::Parametric(segment_t_parametric))
 			});
+
 			*subpath = Subpath::from_anchors(new_anchors, subpath.closed() && count as usize > 1);
 		}