-
-
Notifications
You must be signed in to change notification settings - Fork 787
Port node graph wires to the backend and improve graph UI performance #2795
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
Conversation
f333e49
to
e3ebaf9
Compare
fc7e515
to
ac01057
Compare
!build |
|
set_to_exposed: !exposed, | ||
start_transaction: true, | ||
} | ||
.into()])) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offscreen node culling
blank_assist, | ||
exposeable, |
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.
move exposable into the info section
let ParameterWidgetsInfo { | ||
document_node, | ||
node_id, | ||
index, | ||
name, | ||
description, | ||
input_type, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calculcated based on the type in the new
constructor
pub input_name: String, | ||
#[serde(skip)] |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow same pattern as NodeNetworkMetadata/DocumentNodeMetadata
|
||
#[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize)] |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also includes node path
897c840
to
be83dea
Compare
be83dea
to
27fd418
Compare
Found one last bug: wires break when deleting a node in the middle of a chain, undoing the deletion, then redoing the deletion. |
Closes #2830.
Move wire rendering into backend
Implement culling for offscreen nodes
Improves properties/document upgrade stability
Validate that the input metadata matches the node inputs, and get populated from the definition if non existent
Consolidate logic in the widget parameters
new
functionImprove logic for collecting/sending input metadata for the frontend nodes
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