Improve node graph performance #2795
Conversation
Keavon
force-pushed
the
node-graph-wire-refactor
branch
from
f333e49 to
e3ebaf9
Compare
adamgerhant
force-pushed
the
node-graph-wire-refactor
branch
from
fc7e515 to
ac01057
Compare
|
!build |
|
| set_to_exposed: !exposed, | ||
| start_transaction: true, | ||
| } | ||
| .into()])) |
There was a problem hiding this comment.
only zoom into the exposed node if node graph is closed (implemented in ExposeInput)
| // Skip not exposed inputs (they still get an entry to help with finding the primary input) | ||
| let lookup = if !is_exposed { | ||
| None | ||
| } else { |
There was a problem hiding this comment.
refactor collecting frontend metadata for node inputs when sending the node graph to the frontend.
| connected_to, | ||
| }) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let mut exposed_outputs = Vec::new(); | ||
| for (index, exposed_output) in output_types.iter().enumerate() { | ||
| if index == 0 && network_interface.has_primary_output(&node_id, breadcrumb_network_path) { | ||
| for output_index in 0..network_interface.number_of_outputs(&node_id, breadcrumb_network_path) { |
There was a problem hiding this comment.
refactor output type function to return per output, rather than all types for a node
| valid_types: input.valid_types.iter().map(|ty| ty.to_string()).collect(), | ||
| name: input.input_name.unwrap_or_else(|| input.ty.nested_type().to_string()), | ||
| description: input.input_description.unwrap_or_default(), | ||
| name: input.input_name, |
There was a problem hiding this comment.
input name logic all performed when collecting the frontend_inputs_lookup
| let wires = self.collect_wires(network_interface, preferences.graph_wire_style, breadcrumb_network_path); | ||
| responses.add(FrontendMessage::UpdateNodeGraphWires { wires }); | ||
| } | ||
| NodeGraphMessage::UpdateVisibleNodes => { |
There was a problem hiding this comment.
Offscreen node culling
| blank_assist, | ||
| exposeable, |
There was a problem hiding this comment.
move exposable into the info section
| let ParameterWidgetsInfo { | ||
| document_node, | ||
| node_id, | ||
| index, | ||
| name, | ||
| description, | ||
| input_type, |
There was a problem hiding this comment.
Base expose widget type on the actual type rather than a hardcoded value
| @@ -1903,44 +1833,37 @@ pub fn math_properties(node_id: NodeId, context: &mut NodePropertiesContext) -> | |||
| } | |||
|
|
|||
| pub struct ParameterWidgetsInfo<'a> { | |||
| document_node: &'a DocumentNode, | |||
| document_node: Option<&'a DocumentNode>, | |||
There was a problem hiding this comment.
Now an option so the error logic can be handled in the widget (once), rather than the user of the widget (multiple)
| node_id: NodeId, | ||
| index: usize, | ||
| name: &'a str, | ||
| description: &'a str, | ||
| name: String, |
There was a problem hiding this comment.
Owned value, since it gets converted to an owned value when sending to the front end, and there needs to be logic to replace the name with the type if necessary.
| description: &'a str, | ||
| name: String, | ||
| description: String, | ||
| input_type: FrontendGraphDataType, |
There was a problem hiding this comment.
calculcated based on the type in the new constructor
| @@ -50,7 +50,7 @@ pub struct FrontendGraphInput { | |||
| pub name: String, | |||
| pub description: String, | |||
| #[serde(rename = "resolvedType")] | |||
| pub resolved_type: Option<String>, | |||
| pub resolved_type: String, | |||
There was a problem hiding this comment.
Frontend shouldn't perform logic for what to display, it should all be centralized in the backend.
| @@ -995,27 +990,24 @@ pub fn query_assign_colors_randomize(node_id: NodeId, context: &NodePropertiesCo | |||
| pub(crate) fn brightness_contrast_properties(node_id: NodeId, context: &mut NodePropertiesContext) -> Vec<LayoutGroup> { | |||
| use graphene_std::raster::brightness_contrast::*; | |||
|
|
|||
| // Use Classic | |||
| let use_classic = bool_widget(ParameterWidgetsInfo::new(node_id, UseClassicInput::INDEX, true, context), CheckboxInput::default()); | |||
|
|
|||
There was a problem hiding this comment.
creating a widget requires a mutable reference to the network interface to get the input type, although this will be changed in my next pr. This is why many of the properties have to be reordered
|
|
||
| let mut widgets = super::start_widgets_exposable(self.parameter_info, FrontendGraphDataType::General, self.exposable); | ||
| let mut widgets = super::start_widgets(self.parameter_info); |
There was a problem hiding this comment.
exposable now lives in the parameter info
| @@ -153,16 +153,6 @@ pub struct Transform { | |||
| pub y: f64, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
These structs are moved to node_graph/utility_types.rs, where the wire vector shape is generated
| @@ -699,60 +774,51 @@ impl NodeNetworkInterface { | |||
| /// | |||
| /// This function assumes that export indices and node IDs always exist within their respective | |||
| /// collections. It will panic if these assumptions are violated. | |||
| pub fn output_types(&self, node_id: &NodeId, network_path: &[NodeId]) -> Vec<Option<(Type, TypeSource)>> { | |||
| /// | |||
There was a problem hiding this comment.
refactors to return single type for each output index, just like input_type function
| // let mut encapsulating_path = network_path.to_vec(); | ||
| // let encapsulating_node = encapsulating_path.pop().expect("No imports exist in document network"); | ||
| // self.input_type(&InputConnector::node(encapsulating_node, *import_index), network_path) | ||
| (concrete!(()), TypeSource::Error("Could not type from network")) |
There was a problem hiding this comment.
This will be implemented in my next pr
| } | ||
| graph_craft::document::DocumentNodeImplementation::Extract => { | ||
| output_types.push(Some((concrete!(()), TypeSource::Error("extract node")))); | ||
| self.resolved_types |
There was a problem hiding this comment.
function no longer returns an option, will refactor in my next pr so that errors will display "Unknown" as the type
| @@ -1124,55 +1136,39 @@ impl NodeNetworkInterface { | |||
| Some(&node.implementation) | |||
| } | |||
|
|
|||
| pub fn input_name<'a>(&'a self, node_id: NodeId, index: usize, network_path: &[NodeId]) -> Option<&'a str> { | |||
There was a problem hiding this comment.
input name no longer relies on the definition name, since it is guaranteed to exist for every input due to validate_input_metadata when opening the file
| } | ||
|
|
||
| pub fn insert_input_properties_row(&mut self, node_id: &NodeId, index: usize, network_path: &[NodeId], row: PropertiesRow) { |
There was a problem hiding this comment.
this should never be called, since when upgrading a node the inputs and metadata are added at the same time to ensure they stay in sync
| @@ -2334,7 +2358,7 @@ impl NodeNetworkInterface { | |||
| return; | |||
| }; | |||
| network_metadata.transient_metadata.all_nodes_bounding_box.unload(); | |||
There was a problem hiding this comment.
now a function to ensure wires are also unloaded
| @@ -5679,34 +6004,6 @@ impl NodeNetworkInterface { | |||
| self.force_set_upstream_to_chain(node_id, network_path); | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Same function existed in document.rs
| pub input_name: String, | ||
| #[serde(skip)] |
There was a problem hiding this comment.
Should not be skipped, since this means renamed inputs wouldnt be saved
| impl Default for PropertiesRow { | ||
| fn default() -> Self { | ||
| ("", "TODO").into() | ||
| } |
There was a problem hiding this comment.
The "TODO" got replaces with "" when sending to the frontend, so theres no reason to default to it
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
refactor to be a chainable constructor
| @@ -6403,6 +6677,48 @@ impl DocumentNodePersistentMetadata { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug, Default, serde::Serialize, serde::Deserialize)] | |||
| pub struct InputMetadata { | |||
There was a problem hiding this comment.
follow same pattern as NodeNetworkMetadata/DocumentNodeMetadata
|
|
||
| #[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize)] |
There was a problem hiding this comment.
document upgrade struct
| DocumentNodePersistentMetadata { | ||
| reference: old.reference, | ||
| display_name: old.display_name, | ||
| input_properties, | ||
| input_metadata: Vec::new(), |
There was a problem hiding this comment.
when upgrading from an old file, replace the input metadata with an empty vec, which will be populated based on the definition in validate_input_metadata
| @@ -426,6 +427,38 @@ impl MessageHandler<PortfolioMessage, PortfolioMessageData<'_>> for PortfolioMes | |||
| // Upgrade the document's nodes to be compatible with the latest version | |||
| document_migration_upgrades(&mut document, reset_node_definitions_on_open); | |||
|
|
|||
| // Ensure each node has the metadata for its inputs | |||
| for (node_id, node, path) in document.network_interface.document_network().clone().recursive_nodes() { | |||
| document.network_interface.validate_input_metadata(node_id, node, &path); | |||
There was a problem hiding this comment.
ensure all input metadata exists for each input, if not then it uses the default from the definition. If there is no definition reference, then it uses an empty name, which will get replaced with the type
| @@ -172,9 +172,10 @@ impl EditorTestUtils { | |||
| pub fn get_node<'a, T: InputAccessor<'a, DocumentNode>>(&'a self) -> impl Iterator<Item = T> + 'a { | |||
| self.active_document() | |||
| .network_interface | |||
There was a problem hiding this comment.
migrated to function that already existed in document.rs
| @@ -363,17 +363,6 @@ impl NodeInput { | |||
| NodeInput::Reflection(_) => false, | |||
| } | |||
| } | |||
There was a problem hiding this comment.
network inputs no longer exist in the top level document network
| RecursiveNodeIter { nodes } | ||
| } | ||
| } | ||
|
|
||
| /// An iterator over all [`DocumentNode`]s, including ones that are deeply nested. | ||
| pub struct RecursiveNodeIter<'a> { | ||
| nodes: Vec<(&'a NodeId, &'a DocumentNode)>, | ||
| nodes: Vec<(&'a NodeId, &'a DocumentNode, Vec<NodeId>)>, |
There was a problem hiding this comment.
Also includes node path
adamgerhant
force-pushed
the
node-graph-wire-refactor
branch
from
897c840 to
be83dea
Compare
adamgerhant
force-pushed
the
node-graph-wire-refactor
branch
from
be83dea to
27fd418
Compare
Move wire rendering into backend
Implement culling for offscreen nodes
Improves properties/document upgrade stability
Decreases frame time from 120ms -> 20ms when zoomed in on red dress node graph
Increases frame time from 120 -> 200ms when fully zoomed out
Old:
New:
Known issues:
Subpath rectangle_intersections_exist function is buggy, so dragging onto wire is still inconsistent