Skip to content

Commit cb42891

Browse files
Keavon0HyperCube
Keavon
and
0HyperCube
authored
Instance tables refactor part 6: unwrap VectorData and ImageFrame from single-row to multi-row tables (#2684)
* Start refactoring the boolean operations code * Switch to iterators in the boolean operations code * Make boolean operations work on rows of a table, not Vecs of single-row tables * Remove more .transform() * Simplify brush code * Attempt to remove .transform() by using Instance<Image<Color>> in brush code, but a regression is introduced * Improve blend_image_closure * Simplify * Remove leading underscore from type arguments * Remove .transform() from ImageFrameTable<P> and fix Mask node behavior on stencils not fully overlapping its target image * Remove more .one_instance_ref() * Fully remove .one_instance_ref() and improve the 'Combine Channels' node robustness * Fully remove .once_instance_mut() * Fix tests * Remove .one_empty_image() * Make Instances<T>::default() return an empty table for images, but still not yet vector --------- Co-authored-by: hypercube <0hypercube@gmail.com>
1 parent 76ecdc8 commit cb42891

File tree

29 files changed

+837
-766
lines changed

29 files changed

+837
-766
lines changed

‎editor/src/messages/portfolio/document/document_message_handler.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3292,7 +3292,7 @@ mod document_message_handler_tests {
32923292
let document = editor.active_document();
32933293
let rect_bbox_before = document.metadata().bounding_box_viewport(rect_layer).unwrap();
32943294

3295-
// Moving rectangle from folder1 --> folder2
3295+
// Moving rectangle from folder1 to folder2
32963296
editor.handle_message(DocumentMessage::MoveSelectedLayersTo { parent: folder2, insert_index: 0 }).await;
32973297

32983298
// Rectangle's viewport position after moving
@@ -3304,6 +3304,10 @@ mod document_message_handler_tests {
33043304
let after_center = (rect_bbox_after[0] + rect_bbox_after[1]) / 2.;
33053305
let distance = before_center.distance(after_center);
33063306

3307-
assert!(distance < 1., "Rectangle should maintain its viewport position after moving between transformed groups");
3307+
assert!(
3308+
distance < 1.,
3309+
"Rectangle should maintain its viewport position after moving between transformed groups\n\
3310+
Before: {before_center:?}, After: {after_center:?}, Distance: {distance} (should be < 1)"
3311+
);
33083312
}
33093313
}

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

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -413,13 +413,10 @@ impl<'a> ModifyInputsContext<'a> {
413413
pub fn transform_set_direct(&mut self, transform: DAffine2, skip_rerender: bool, transform_node_id: Option<NodeId>) {
414414
// If the Transform node didn't exist yet, create it now
415415
let Some(transform_node_id) = transform_node_id.or_else(|| {
416-
// Check if the transform is the identity transform and if so, don't create a new Transform node
417-
if let Some((scale, angle, translation)) = (transform.matrix2.determinant() != 0.).then(|| transform.to_scale_angle_translation()) {
418-
// Check if the transform is the identity transform within an epsilon
419-
if scale.x.abs() < 1e-6 && scale.y.abs() < 1e-6 && angle.abs() < 1e-6 && translation.x.abs() < 1e-6 && translation.y.abs() < 1e-6 {
420-
// We don't want to pollute the graph with an unnecessary Transform node, so we avoid creating and setting it by returning None
421-
return None;
422-
}
416+
// Check if the transform is the identity transform (within an epsilon) and if so, don't create a new Transform node
417+
if transform.abs_diff_eq(DAffine2::IDENTITY, 1e-6) {
418+
// We don't want to pollute the graph with an unnecessary Transform node, so we avoid creating and setting it by returning None
419+
return None;
423420
}
424421

425422
// Create the Transform node
@@ -453,7 +450,7 @@ impl<'a> ModifyInputsContext<'a> {
453450

454451
pub fn brush_modify(&mut self, strokes: Vec<BrushStroke>) {
455452
let Some(brush_node_id) = self.existing_node_id("Brush", true) else { return };
456-
self.set_input_with_refresh(InputConnector::node(brush_node_id, 2), NodeInput::value(TaggedValue::BrushStrokes(strokes), false), false);
453+
self.set_input_with_refresh(InputConnector::node(brush_node_id, 1), NodeInput::value(TaggedValue::BrushStrokes(strokes), false), false);
457454
}
458455

459456
pub fn resize_artboard(&mut self, location: IVec2, dimensions: IVec2) {

‎editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -636,7 +636,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
636636
.collect(),
637637
..Default::default()
638638
}),
639-
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true)],
639+
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true)],
640640
..Default::default()
641641
},
642642
persistent_node_metadata: DocumentNodePersistentMetadata {
@@ -893,7 +893,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
893893
.collect(),
894894
..Default::default()
895895
}),
896-
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true)],
896+
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true)],
897897
..Default::default()
898898
},
899899
persistent_node_metadata: DocumentNodePersistentMetadata {
@@ -978,7 +978,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
978978

979979
..Default::default()
980980
}),
981-
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true)],
981+
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true)],
982982
..Default::default()
983983
},
984984
persistent_node_metadata: DocumentNodePersistentMetadata {
@@ -1019,6 +1019,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
10191019
description: Cow::Borrowed("TODO"),
10201020
properties: None,
10211021
},
1022+
// TODO: Remove this and just use the proto node definition directly
10221023
DocumentNodeDefinition {
10231024
identifier: "Brush",
10241025
category: "Raster",
@@ -1029,9 +1030,8 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
10291030
nodes: vec![DocumentNode {
10301031
inputs: vec![
10311032
NodeInput::network(concrete!(ImageFrameTable<Color>), 0),
1032-
NodeInput::network(concrete!(ImageFrameTable<Color>), 1),
1033-
NodeInput::network(concrete!(Vec<graphene_core::vector::brush_stroke::BrushStroke>), 2),
1034-
NodeInput::network(concrete!(BrushCache), 3),
1033+
NodeInput::network(concrete!(Vec<graphene_core::vector::brush_stroke::BrushStroke>), 1),
1034+
NodeInput::network(concrete!(BrushCache), 2),
10351035
],
10361036
manual_composition: Some(concrete!(Context)),
10371037
implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_std::brush::BrushNode")),
@@ -1044,15 +1044,14 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
10441044
..Default::default()
10451045
}),
10461046
inputs: vec![
1047-
NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true),
1048-
NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), false),
1047+
NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true),
10491048
NodeInput::value(TaggedValue::BrushStrokes(Vec::new()), false),
10501049
NodeInput::value(TaggedValue::BrushCache(BrushCache::new_proto()), false),
10511050
],
10521051
..Default::default()
10531052
},
10541053
persistent_node_metadata: DocumentNodePersistentMetadata {
1055-
input_properties: vec![("Background", "TODO").into(), ("Bounds", "TODO").into(), ("Trace", "TODO").into(), ("Cache", "TODO").into()],
1054+
input_properties: vec![("Background", "TODO").into(), ("Trace", "TODO").into(), ("Cache", "TODO").into()],
10561055
output_names: vec!["Image".to_string()],
10571056
network_metadata: Some(NodeNetworkMetadata {
10581057
persistent_metadata: NodeNetworkPersistentMetadata {
@@ -1084,7 +1083,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
10841083
node_template: NodeTemplate {
10851084
document_node: DocumentNode {
10861085
implementation: DocumentNodeImplementation::proto("graphene_core::memo::MemoNode"),
1087-
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true)],
1086+
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true)],
10881087
manual_composition: Some(concrete!(Context)),
10891088
..Default::default()
10901089
},
@@ -1103,7 +1102,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
11031102
node_template: NodeTemplate {
11041103
document_node: DocumentNode {
11051104
implementation: DocumentNodeImplementation::proto("graphene_core::memo::ImpureMemoNode"),
1106-
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true)],
1105+
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true)],
11071106
manual_composition: Some(concrete!(Context)),
11081107
..Default::default()
11091108
},
@@ -1803,7 +1802,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
18031802
.collect(),
18041803
..Default::default()
18051804
}),
1806-
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true)],
1805+
inputs: vec![NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true)],
18071806
..Default::default()
18081807
},
18091808
persistent_node_metadata: DocumentNodePersistentMetadata {
@@ -2685,7 +2684,7 @@ fn static_nodes() -> Vec<DocumentNodeDefinition> {
26852684
// ..Default::default()
26862685
// }),
26872686
// inputs: vec![
2688-
// NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::one_empty_image()), true),
2687+
// NodeInput::value(TaggedValue::ImageFrame(ImageFrameTable::default()), true),
26892688
// NodeInput::scope("editor-api"),
26902689
// NodeInput::value(TaggedValue::ImaginateController(Default::default()), false),
26912690
// NodeInput::value(TaggedValue::F64(0.), false), // Remember to keep index used in `ImaginateRandom` updated with this entry's index

‎editor/src/messages/portfolio/document/utility_types/transformation.rs‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -635,17 +635,17 @@ impl<'a> Selected<'a> {
635635
}
636636

637637
pub fn apply_transformation(&mut self, transformation: DAffine2, transform_operation: Option<TransformOperation>) {
638-
if !self.selected.is_empty() {
639-
// TODO: Cache the result of `shallowest_unique_layers` to avoid this heavy computation every frame of movement, see https://github.com/GraphiteEditor/Graphite/pull/481
640-
for layer in self.network_interface.shallowest_unique_layers(&[]) {
641-
match &mut self.original_transforms {
642-
OriginalTransforms::Layer(layer_transforms) => {
643-
Self::transform_layer(self.network_interface.document_metadata(), layer, layer_transforms.get(&layer), transformation, self.responses)
644-
}
645-
OriginalTransforms::Path(path_transforms) => {
646-
if let Some(initial_points) = path_transforms.get_mut(&layer) {
647-
Self::transform_path(self.network_interface.document_metadata(), layer, initial_points, transformation, self.responses, transform_operation)
648-
}
638+
if self.selected.is_empty() {
639+
return;
640+
}
641+
642+
// TODO: Cache the result of `shallowest_unique_layers` to avoid this heavy computation every frame of movement, see https://github.com/GraphiteEditor/Graphite/pull/481
643+
for layer in self.network_interface.shallowest_unique_layers(&[]) {
644+
match &mut self.original_transforms {
645+
OriginalTransforms::Layer(layer_transforms) => Self::transform_layer(self.network_interface.document_metadata(), layer, layer_transforms.get(&layer), transformation, self.responses),
646+
OriginalTransforms::Path(path_transforms) => {
647+
if let Some(initial_points) = path_transforms.get_mut(&layer) {
648+
Self::transform_path(self.network_interface.document_metadata(), layer, initial_points, transformation, self.responses, transform_operation)
649649
}
650650
}
651651
}

‎editor/src/messages/portfolio/portfolio_message_handler.rs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -930,6 +930,23 @@ impl MessageHandler<PortfolioMessage, PortfolioMessageData<'_>> for PortfolioMes
930930
document.network_interface.set_input(&InputConnector::node(*node_id, 2), old_inputs[2].clone(), network_path);
931931
// We have removed the last input, so we don't add index 3
932932
}
933+
934+
if reference == "Brush" && inputs_count == 4 {
935+
let node_definition = resolve_document_node_type(reference).unwrap();
936+
let new_node_template = node_definition.default_node_template();
937+
let document_node = new_node_template.document_node;
938+
document.network_interface.replace_implementation(node_id, network_path, document_node.implementation.clone());
939+
document
940+
.network_interface
941+
.replace_implementation_metadata(node_id, network_path, new_node_template.persistent_node_metadata);
942+
943+
let old_inputs = document.network_interface.replace_inputs(node_id, document_node.inputs.clone(), network_path);
944+
945+
document.network_interface.set_input(&InputConnector::node(*node_id, 0), old_inputs[0].clone(), network_path);
946+
// We have removed the second input ("bounds"), so we don't add index 1 and we shift the rest of the inputs down by one
947+
document.network_interface.set_input(&InputConnector::node(*node_id, 1), old_inputs[2].clone(), network_path);
948+
document.network_interface.set_input(&InputConnector::node(*node_id, 2), old_inputs[3].clone(), network_path);
949+
}
933950
}
934951

935952
// TODO: Eventually remove this document upgrade code

‎editor/src/messages/tool/tool_messages/brush_tool.rs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -276,15 +276,16 @@ impl BrushToolData {
276276
let Some(reference) = document.network_interface.reference(&node_id, &[]) else {
277277
continue;
278278
};
279+
279280
if *reference == Some("Brush".to_string()) && node_id != layer.to_node() {
280-
let points_input = node.inputs.get(2)?;
281-
let Some(TaggedValue::BrushStrokes(strokes)) = points_input.as_value() else {
282-
continue;
283-
};
281+
let points_input = node.inputs.get(1)?;
282+
let Some(TaggedValue::BrushStrokes(strokes)) = points_input.as_value() else { continue };
284283
self.strokes.clone_from(strokes);
285284

286285
return Some(layer);
287-
} else if *reference == Some("Transform".to_string()) {
286+
}
287+
288+
if *reference == Some("Transform".to_string()) {
288289
let upstream = document.metadata().upstream_transform(node_id);
289290
let pivot = DAffine2::from_translation(upstream.transform_point2(get_current_normalized_pivot(&node.inputs)));
290291
self.transform = pivot * get_current_transform(&node.inputs) * pivot.inverse() * self.transform;

‎editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs‎

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -397,31 +397,28 @@ impl MessageHandler<TransformLayerMessage, TransformData<'_>> for TransformLayer
397397
return;
398398
}
399399

400-
let Some(vector_data) = selected_layers.first().and_then(|&layer| document.network_interface.compute_modified_vector(layer)) else {
401-
selected.original_transforms.clear();
402-
return;
403-
};
404-
405-
if let [point] = selected_points.as_slice() {
406-
if matches!(point, ManipulatorPointId::Anchor(_)) {
407-
if let Some([handle1, handle2]) = point.get_handle_pair(&vector_data) {
408-
let handle1_length = handle1.length(&vector_data);
409-
let handle2_length = handle2.length(&vector_data);
410-
411-
if (handle1_length == 0. && handle2_length == 0. && !using_select_tool) || (handle1_length == f64::MAX && handle2_length == f64::MAX && !using_select_tool) {
412-
// G should work for this point but not R and S
413-
if matches!(transform_type, TransformType::Rotate | TransformType::Scale) {
414-
selected.original_transforms.clear();
415-
return;
400+
if let Some(vector_data) = selected_layers.first().and_then(|&layer| document.network_interface.compute_modified_vector(layer)) {
401+
if let [point] = selected_points.as_slice() {
402+
if matches!(point, ManipulatorPointId::Anchor(_)) {
403+
if let Some([handle1, handle2]) = point.get_handle_pair(&vector_data) {
404+
let handle1_length = handle1.length(&vector_data);
405+
let handle2_length = handle2.length(&vector_data);
406+
407+
if (handle1_length == 0. && handle2_length == 0. && !using_select_tool) || (handle1_length == f64::MAX && handle2_length == f64::MAX && !using_select_tool) {
408+
// G should work for this point but not R and S
409+
if matches!(transform_type, TransformType::Rotate | TransformType::Scale) {
410+
selected.original_transforms.clear();
411+
return;
412+
}
416413
}
417414
}
418-
}
419-
} else {
420-
let handle_length = point.as_handle().map(|handle| handle.length(&vector_data));
415+
} else {
416+
let handle_length = point.as_handle().map(|handle| handle.length(&vector_data));
421417

422-
if handle_length == Some(0.) {
423-
selected.original_transforms.clear();
424-
return;
418+
if handle_length == Some(0.) {
419+
selected.original_transforms.clear();
420+
return;
421+
}
425422
}
426423
}
427424
}

‎editor/src/node_graph_executor/runtime.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ use graphene_core::renderer::{RenderSvgSegmentList, SvgSegment};
1414
use graphene_core::text::FontCache;
1515
use graphene_core::vector::style::ViewMode;
1616
use graphene_std::Context;
17+
use graphene_std::instances::Instance;
1718
use graphene_std::vector::{VectorData, VectorDataTable};
1819
use graphene_std::wasm_application_io::{WasmApplicationIo, WasmEditorApi};
1920
use interpreted_executor::dynamic_executor::{DynamicExecutor, IntrospectError, ResolvedDocumentNodeTypesDelta};
@@ -293,9 +294,16 @@ impl NodeRuntime {
293294
Self::process_graphic_element(&mut self.thumbnail_renders, parent_network_node_id, &io.output, responses, update_thumbnails)
294295
// Insert the vector modify if we are dealing with vector data
295296
} else if let Some(record) = introspected_data.downcast_ref::<IORecord<Context, VectorDataTable>>() {
296-
self.vector_modify.insert(parent_network_node_id, record.output.one_instance_ref().instance.clone());
297+
let default = Instance {
298+
instance: VectorData::empty(),
299+
..Default::default()
300+
};
301+
self.vector_modify.insert(
302+
parent_network_node_id,
303+
record.output.instance_ref_iter().next().unwrap_or_else(|| default.to_instance_ref()).instance.clone(),
304+
);
297305
} else {
298-
log::warn!("failed to downcast monitor node output {parent_network_node_id:?}");
306+
log::warn!("Failed to downcast monitor node output {parent_network_node_id:?}");
299307
}
300308
}
301309
}

‎node-graph/gcore/src/application_io.rs‎

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::instances::Instances;
22
use crate::text::FontCache;
3-
use crate::transform::{Footprint, Transform, TransformMut};
3+
use crate::transform::Footprint;
44
use crate::vector::style::ViewMode;
55
use alloc::sync::Arc;
66
use core::fmt::Debug;
@@ -37,17 +37,6 @@ impl Hash for SurfaceFrame {
3737
}
3838
}
3939

40-
impl Transform for SurfaceFrame {
41-
fn transform(&self) -> DAffine2 {
42-
self.transform
43-
}
44-
}
45-
impl TransformMut for SurfaceFrame {
46-
fn transform_mut(&mut self) -> &mut DAffine2 {
47-
&mut self.transform
48-
}
49-
}
50-
5140
#[cfg(feature = "dyn-any")]
5241
unsafe impl StaticType for SurfaceFrame {
5342
type Static = SurfaceFrame;
@@ -152,18 +141,6 @@ unsafe impl<T: 'static> StaticType for SurfaceHandleFrame<T> {
152141
type Static = SurfaceHandleFrame<T>;
153142
}
154143

155-
impl<T> Transform for SurfaceHandleFrame<T> {
156-
fn transform(&self) -> DAffine2 {
157-
self.transform
158-
}
159-
}
160-
161-
impl<T> TransformMut for SurfaceHandleFrame<T> {
162-
fn transform_mut(&mut self) -> &mut DAffine2 {
163-
&mut self.transform
164-
}
165-
}
166-
167144
// TODO: think about how to automatically clean up memory
168145
/*
169146
impl<'a, Surface> Drop for SurfaceHandle<'a, Surface> {

0 commit comments

Comments
 (0)