From 4a760c1a70008b2ea5112b4d6026d3ae455d2d03 Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Fri, 9 May 2025 04:06:00 +0200 Subject: [PATCH] Nodes: show link errors directly on link Previously, we had shown link errors on the target node as part of the node warning system. While better than not showing any information about invalid links (as was the state before that), it's still not ideal because it's easy to miss when just looking at the link. This patch adds an error icon in the middle of the invalid link. When hovering over it, it shows the error text. When the middle of the link is not in view but part of the link is, then the error icon will also stay visible. Pull Request: https://projects.blender.org/blender/blender/pulls/138529 --- source/blender/blenkernel/BKE_node_runtime.hh | 37 +++- .../blender/blenkernel/intern/node_runtime.cc | 33 +++ .../blenkernel/intern/node_tree_update.cc | 19 +- source/blender/blenlib/BLI_rect.h | 8 + source/blender/blenlib/intern/rct.cc | 64 ++++++ source/blender/editors/space_node/drawnode.cc | 6 +- .../blender/editors/space_node/node_draw.cc | 209 ++++++++++++++---- .../blender/editors/space_node/node_intern.hh | 4 + 8 files changed, 326 insertions(+), 54 deletions(-) diff --git a/source/blender/blenkernel/BKE_node_runtime.hh b/source/blender/blenkernel/BKE_node_runtime.hh index b339cf241dc..8975a79cfb4 100644 --- a/source/blender/blenkernel/BKE_node_runtime.hh +++ b/source/blender/blenkernel/BKE_node_runtime.hh @@ -15,6 +15,7 @@ #include "BLI_multi_value_map.hh" #include "BLI_mutex.hh" #include "BLI_set.hh" +#include "BLI_struct_equality_utils.hh" #include "BLI_utility_mixins.hh" #include "BLI_vector.hh" #include "BLI_vector_set.hh" @@ -69,6 +70,34 @@ struct NodeLinkError { std::string tooltip; }; +/** + * Utility to weakly reference a link. Weak references are safer because they avoid dangling + * references which can easily happen temporarily when editing the node tree. + */ +struct NodeLinkKey { + private: + int to_node_id_; + int input_socket_index_; + int input_link_index_; + + public: + /** Assumes that the topology cache is up to date. */ + explicit NodeLinkKey(const bNodeLink &link); + + bNodeLink *try_find(bNodeTree &ntree) const; + const bNodeLink *try_find(const bNodeTree &ntree) const; + + uint64_t hash() const + { + return get_default_hash(this->to_node_id_, this->input_socket_index_, this->input_link_index_); + } + + BLI_STRUCT_EQUALITY_OPERATORS_3(NodeLinkKey, + to_node_id_, + input_socket_index_, + input_link_index_); +}; + struct LoggedZoneGraphs { Mutex mutex; /** @@ -163,11 +192,10 @@ class bNodeTreeRuntime : NonCopyable, NonMovable { geometry_nodes_lazy_function_graph_info; /** - * Stores information about invalid links. This information is then displayed to the user. The - * key of the map is the node identifier. The data is stored per target-node because we want to - * display the error information there. + * Stores information about invalid links. This information is then displayed to the user. This + * is updated in #update_link_validation and is valid during drawing code. */ - MultiValueMap link_errors_by_target_node; + MultiValueMap link_errors; /** * Protects access to all topology cache variables below. This is necessary so that the cache can @@ -421,6 +449,7 @@ inline bool topology_cache_is_available(const bNodeSocket &socket) namespace node_field_inferencing { bool update_field_inferencing(const bNodeTree &tree); } + } // namespace blender::bke /* -------------------------------------------------------------------- */ diff --git a/source/blender/blenkernel/intern/node_runtime.cc b/source/blender/blenkernel/intern/node_runtime.cc index 9a8c3496d41..0e3c86c8358 100644 --- a/source/blender/blenkernel/intern/node_runtime.cc +++ b/source/blender/blenkernel/intern/node_runtime.cc @@ -576,6 +576,39 @@ static void ensure_topology_cache(const bNodeTree &ntree) } // namespace blender::bke::node_tree_runtime +namespace blender::bke { + +NodeLinkKey::NodeLinkKey(const bNodeLink &link) +{ + to_node_id_ = link.tonode->identifier; + input_socket_index_ = link.tosock->index(); + input_link_index_ = + const_cast(link.tosock)->directly_linked_links().first_index(&link); +} + +bNodeLink *NodeLinkKey::try_find(bNodeTree &ntree) const +{ + return const_cast(this->try_find(const_cast(ntree))); +} + +const bNodeLink *NodeLinkKey::try_find(const bNodeTree &ntree) const +{ + const bNode *to_node = ntree.node_by_id(to_node_id_); + if (!to_node) { + return nullptr; + } + if (input_socket_index_ >= to_node->input_sockets().size()) { + return nullptr; + } + const bNodeSocket &input_socket = to_node->input_socket(input_socket_index_); + if (input_link_index_ >= input_socket.directly_linked_links().size()) { + return nullptr; + } + return input_socket.directly_linked_links()[input_link_index_]; +} + +} // namespace blender::bke + void bNodeTree::ensure_topology_cache() const { blender::bke::node_tree_runtime::ensure_topology_cache(*this); diff --git a/source/blender/blenkernel/intern/node_tree_update.cc b/source/blender/blenkernel/intern/node_tree_update.cc index 750a8f32162..160c9fd29d6 100644 --- a/source/blender/blenkernel/intern/node_tree_update.cc +++ b/source/blender/blenkernel/intern/node_tree_update.cc @@ -488,7 +488,7 @@ class NodeTreeMainUpdater { { TreeUpdateResult result; - ntree.runtime->link_errors_by_target_node.clear(); + ntree.runtime->link_errors.clear(); if (this->update_panel_toggle_names(ntree)) { result.interface_changed = true; @@ -1205,8 +1205,8 @@ class NodeTreeMainUpdater { } if (is_invalid_enum_ref(*link->fromsock) || is_invalid_enum_ref(*link->tosock)) { link->flag &= ~NODE_LINK_VALID; - ntree.runtime->link_errors_by_target_node.add( - link->tonode->identifier, + ntree.runtime->link_errors.add( + NodeLinkKey{*link}, NodeLinkError{TIP_("Use node groups to reuse the same menu multiple times")}); continue; } @@ -1216,9 +1216,8 @@ class NodeTreeMainUpdater { field_states[link->tosock->index_in_tree()] != FieldSocketState::IsField) { link->flag &= ~NODE_LINK_VALID; - ntree.runtime->link_errors_by_target_node.add( - link->tonode->identifier, - NodeLinkError{TIP_("The node input does not support fields")}); + ntree.runtime->link_errors.add( + NodeLinkKey{*link}, NodeLinkError{TIP_("The node input does not support fields")}); continue; } } @@ -1228,8 +1227,8 @@ class NodeTreeMainUpdater { to_node.runtime->toposort_left_to_right_index) { link->flag &= ~NODE_LINK_VALID; - ntree.runtime->link_errors_by_target_node.add( - link->tonode->identifier, + ntree.runtime->link_errors.add( + NodeLinkKey{*link}, NodeLinkError{TIP_("The links form a cycle which is not supported")}); continue; } @@ -1238,8 +1237,8 @@ class NodeTreeMainUpdater { const eNodeSocketDatatype to_type = eNodeSocketDatatype(link->tosock->type); if (!ntree.typeinfo->validate_link(from_type, to_type)) { link->flag &= ~NODE_LINK_VALID; - ntree.runtime->link_errors_by_target_node.add( - link->tonode->identifier, + ntree.runtime->link_errors.add( + NodeLinkKey{*link}, NodeLinkError{fmt::format("{}: {} " BLI_STR_UTF8_BLACK_RIGHT_POINTING_SMALL_TRIANGLE " {}", TIP_("Conversion is not supported"), diff --git a/source/blender/blenlib/BLI_rect.h b/source/blender/blenlib/BLI_rect.h index e6904cc019a..844a53eea6d 100644 --- a/source/blender/blenlib/BLI_rect.h +++ b/source/blender/blenlib/BLI_rect.h @@ -147,6 +147,14 @@ void BLI_rctf_rcti_copy(struct rctf *dst, const struct rcti *src); void BLI_rcti_rctf_copy_floor(struct rcti *dst, const struct rctf *src); void BLI_rcti_rctf_copy_round(struct rcti *dst, const struct rctf *src); +/** + * Clamps the given segment to be within the rectangle. + * + * \return False when no part of the segment is within the rectangle, in which case the s1 and s2 + * values should be ignored. + */ +bool BLI_rctf_clamp_segment(const struct rctf *rect, float s1[2], float s2[2]); + /** * Expand the rectangle to fit a rotated \a src. */ diff --git a/source/blender/blenlib/intern/rct.cc b/source/blender/blenlib/intern/rct.cc index 8eeaf094d4c..a330c58a8a8 100644 --- a/source/blender/blenlib/intern/rct.cc +++ b/source/blender/blenlib/intern/rct.cc @@ -16,7 +16,9 @@ #include #include "BLI_math_base.h" +#include "BLI_math_geom.h" #include "BLI_math_matrix.h" +#include "BLI_math_vector.hh" #include "BLI_rect.h" #include "BLI_utildefines.h" @@ -1126,4 +1128,66 @@ void BLI_rctf_rotate_expand(rctf *dst, const rctf *src, const float angle) #undef ROTATE_SINCOS +bool BLI_rctf_clamp_segment(const struct rctf *rect, float s1[2], float s2[2]) +{ + using namespace blender; + + const bool p1_inside = BLI_rctf_isect_pt_v(rect, s1); + const bool p2_inside = BLI_rctf_isect_pt_v(rect, s2); + if (p1_inside && p2_inside) { + return true; + } + + const std::array top_line = {float2{rect->xmin, rect->ymax}, + float2{rect->xmax, rect->ymax}}; + const std::array bottom_line = {float2{rect->xmin, rect->ymin}, + float2{rect->xmax, rect->ymin}}; + const std::array left_line = {float2{rect->xmin, rect->ymin}, + float2{rect->xmin, rect->ymax}}; + const std::array right_line = {float2{rect->xmax, rect->ymin}, + float2{rect->xmax, rect->ymax}}; + const std::array, 4> lines = { + top_line, bottom_line, left_line, right_line}; + + if (p1_inside && !p2_inside) { + for (const std::array &line : lines) { + float2 intersection; + if (isect_seg_seg_v2_point(s1, s2, line[0], line[1], intersection) == 1) { + copy_v2_v2(s2, intersection); + } + } + return true; + } + if (!p1_inside && p2_inside) { + for (const std::array &line : lines) { + float2 intersection; + if (isect_seg_seg_v2_point(s1, s2, line[0], line[1], intersection) == 1) { + copy_v2_v2(s1, intersection); + } + } + return true; + } + + for (const std::array &line : lines) { + float2 intersection; + if (isect_seg_seg_v2_point(s1, s2, line[0], line[1], intersection) == 1) { + copy_v2_v2(s1, intersection); + } + else { + return false; + } + } + for (const std::array &line : lines) { + float2 intersection; + if (isect_seg_seg_v2_point(s2, s1, line[0], line[1], intersection) == 1) { + copy_v2_v2(s2, intersection); + } + else { + return false; + } + } + + return true; +} + /** \} */ diff --git a/source/blender/editors/space_node/drawnode.cc b/source/blender/editors/space_node/drawnode.cc index 90007fd7a82..532128849a2 100644 --- a/source/blender/editors/space_node/drawnode.cc +++ b/source/blender/editors/space_node/drawnode.cc @@ -1674,9 +1674,9 @@ void draw_nodespace_back_pix(const bContext &C, GPU_matrix_pop(); } -static float2 socket_link_connection_location(const bNode &node, - const bNodeSocket &socket, - const bNodeLink &link) +float2 socket_link_connection_location(const bNode &node, + const bNodeSocket &socket, + const bNodeLink &link) { const float2 socket_location = socket.runtime->location; if (socket.is_multi_input() && socket.is_input() && !(node.flag & NODE_HIDDEN)) { diff --git a/source/blender/editors/space_node/node_draw.cc b/source/blender/editors/space_node/node_draw.cc index dec3e38599c..a326c8539d9 100644 --- a/source/blender/editors/space_node/node_draw.cc +++ b/source/blender/editors/space_node/node_draw.cc @@ -3068,41 +3068,6 @@ static void node_get_compositor_extra_info(TreeDrawContext &tree_draw_ctx, } } -static void node_get_invalid_links_extra_info(const SpaceNode &snode, - const bNode &node, - Vector &rows) -{ - const bNodeTree &tree = *snode.edittree; - const Span link_errors = tree.runtime->link_errors_by_target_node.lookup( - node.identifier); - if (link_errors.is_empty()) { - return; - } - NodeExtraInfoRow row; - row.text = IFACE_("Invalid Link"); - - row.tooltip_fn = [](bContext *C, void *arg, const StringRef /*tip*/) { - const bNodeTree &tree = *CTX_wm_space_node(C)->edittree; - const bNode &node = *static_cast(arg); - const Span link_errors = tree.runtime->link_errors_by_target_node.lookup( - node.identifier); - std::stringstream ss; - Set already_added_errors; - for (const int i : link_errors.index_range()) { - const StringRefNull tooltip = link_errors[i].tooltip; - if (already_added_errors.add_as(tooltip)) { - ss << "\u2022 " << tooltip << "\n"; - } - } - ss << "\n"; - ss << "Any invalid links are highlighted"; - return ss.str(); - }; - row.tooltip_fn_arg = const_cast(&node); - row.icon = ICON_ERROR; - rows.append(std::move(row)); -} - static Vector node_get_extra_info(const bContext &C, TreeDrawContext &tree_draw_ctx, const SpaceNode &snode, @@ -3123,8 +3088,6 @@ static Vector node_get_extra_info(const bContext &C, rows.append(std::move(row)); } - node_get_invalid_links_extra_info(snode, node, rows); - if (snode.edittree->type == NTREE_COMPOSIT) { node_get_compositor_extra_info(tree_draw_ctx, snode, node, rows); return rows; @@ -4855,6 +4818,168 @@ static void draw_frame_overlays(const bContext &C, } } +/** + * Tries to find a position on the link where we can draw link information like an error icon. If + * the link center is not visible, it finds the closest point to the link center that's still + * visible with some padding if possible. If none such point is found, nullopt is returned. + */ +static std::optional find_visible_center_of_link(const View2D &v2d, + const bNodeLink &link, + const float radius, + const float region_padding) +{ + /* Compute center of the link because that's used as "ideal" position. */ + const float2 start = socket_link_connection_location(*link.fromnode, *link.fromsock, link); + const float2 end = socket_link_connection_location(*link.tonode, *link.tosock, link); + const float2 center = math::midpoint(start, end); + + /* The rectangle that we would like to stay within if possible. */ + rctf inner_rect = v2d.cur; + BLI_rctf_pad(&inner_rect, -(region_padding + radius), -(region_padding + radius)); + + if (BLI_rctf_isect_pt_v(&inner_rect, center)) { + /* The center is visible. */ + return center; + } + + /* The rectangle containing all points which are valid result positions. */ + rctf outer_rect = v2d.cur; + BLI_rctf_pad(&outer_rect, radius, radius); + + /* Get the straight individual link segments. */ + std::array link_points; + node_link_bezier_points_evaluated(link, link_points); + + const float required_socket_distance = UI_UNIT_X; + /* Define a cost function that returns a value that is larger the worse the given position is. + * The point on the link with the lowest cost will be picked. */ + const auto cost_function = [&](const float2 &p) -> float { + const float distance_to_inner_rect = std::max(BLI_rctf_length_x(&inner_rect, p.x), + BLI_rctf_length_y(&inner_rect, p.y)); + const float distance_to_center = math::distance(p, center); + + /* Set a high cost when the point is close to a socket. The distance to the center still has to + * be taken account though. Otherwise there is bad behavior when both sockets are close to the + * point. */ + const float distance_to_socket = std::min(math::distance(p, start), math::distance(p, end)); + if (distance_to_socket < required_socket_distance) { + return 1e5f + distance_to_center; + } + return + /* The larger the distance to the link center, the higher the cost. The importance of this + distance decreases the further the center is away. */ + std::sqrt(distance_to_center) + /* The larger the distance to the inner rectangle, the higher the cost. Apply an additional + * factor because it's more important that the position stays visible than that it is at + * the center. */ + + 10.0f * distance_to_inner_rect; + }; + + /* Iterate over visible points on the link, compute the cost of each and pick the best one. A + * more direct algorithm to find a good position would be nice. However, that seems to be + * surprisingly tricky to achieve without resulting in very "jumpy" positions, especially when + * the link is colinear to the region border. */ + float best_cost; + std::optional best_position; + for (const int i : IndexRange(link_points.size() - 1)) { + float2 p0 = link_points[i]; + float2 p1 = link_points[i + 1]; + if (!BLI_rctf_clamp_segment(&outer_rect, p0, p1)) { + continue; + } + const float length = math::distance(p0, p1); + const float point_distance = 1.0f; + /* Might be possible to do a smarter scan of the cost function using some sort of binary sort, + * but it's not entirely straight forward because the cost function is not monotonic. */ + const int points_to_check = std::max(2, 1 + int(length / point_distance)); + for (const int j : IndexRange(points_to_check)) { + const float t = float(j) / (points_to_check - 1); + const float2 p = math::interpolate(p0, p1, t); + const float cost = cost_function(p); + if (!best_position.has_value() || cost < best_cost) { + best_cost = cost; + best_position = p; + } + } + } + return best_position; +} + +static void draw_link_errors(const bContext &C, + SpaceNode &snode, + const bNodeLink &link, + const Span errors, + uiBlock &invalid_links_block) +{ + const ARegion ®ion = *CTX_wm_region(&C); + if (errors.is_empty()) { + return; + } + if (!link.fromsock || !link.tosock || !link.fromnode || !link.tonode) { + /* Likely because the link is being dragged. */ + return; + } + + /* Generate full tooltip from potentially multiple errors. */ + std::string error_tooltip; + if (errors.size() == 1) { + error_tooltip = errors[0].tooltip; + } + else { + for (const bke::NodeLinkError &error : errors) { + error_tooltip += fmt::format("\u2022 {}\n", error.tooltip); + } + } + + const float bg_radius = UI_UNIT_X * 0.5f; + const float bg_corner_radius = UI_UNIT_X * 0.2f; + const float icon_size = UI_UNIT_X; + const float region_padding = UI_UNIT_X * 0.5f; + + /* Compute error icon location. */ + std::optional draw_position_opt = find_visible_center_of_link( + region.v2d, link, bg_radius, region_padding); + if (!draw_position_opt.has_value()) { + return; + } + const int2 draw_position = int2(draw_position_opt.value()); + + /* Draw a background for the error icon. */ + rctf bg_rect; + BLI_rctf_init_pt_radius(&bg_rect, float2(draw_position), bg_radius); + ColorTheme4f bg_color; + UI_GetThemeColor4fv(TH_REDALERT, bg_color); + UI_draw_roundbox_corner_set(UI_CNR_ALL); + ui_draw_dropshadow(&bg_rect, bg_corner_radius, UI_UNIT_X * 0.2f, snode.runtime->aspect, 0.5f); + UI_draw_roundbox_4fv(&bg_rect, true, bg_corner_radius, bg_color); + + /* Draw the icon itself with a tooltip. */ + UI_block_emboss_set(&invalid_links_block, ui::EmbossType::None); + uiBut *but = uiDefIconBut(&invalid_links_block, + UI_BTYPE_BUT, + 0, + ICON_ERROR, + draw_position.x - icon_size / 2, + draw_position.y - icon_size / 2, + icon_size, + icon_size, + nullptr, + 0, + 0, + std::nullopt); + UI_but_func_tooltip_label_set( + but, [tooltip = std::move(error_tooltip)](const uiBut * /*but*/) { return tooltip; }); +} + +static uiBlock &invalid_links_uiblock_init(const bContext &C) +{ + Scene *scene = CTX_data_scene(&C); + wmWindow *window = CTX_wm_window(&C); + ARegion *region = CTX_wm_region(&C); + return *UI_block_begin( + &C, scene, window, region, "invalid_links", blender::ui::EmbossType::None); +} + #define USE_DRAW_TOT_UPDATE static void node_draw_nodetree(const bContext &C, @@ -4896,6 +5021,7 @@ static void node_draw_nodetree(const bContext &C, } nodelink_batch_end(snode); + GPU_blend(GPU_BLEND_NONE); draw_frame_overlays(C, tree_draw_ctx, region, snode, ntree, blocks); @@ -4911,6 +5037,15 @@ static void node_draw_nodetree(const bContext &C, const bNodeInstanceKey key = bke::node_instance_key(parent_key, &ntree, &node); node_draw(C, tree_draw_ctx, region, snode, ntree, node, *blocks[node.index()], key); } + + uiBlock &invalid_links_block = invalid_links_uiblock_init(C); + for (auto &&item : ntree.runtime->link_errors.items()) { + if (const bNodeLink *link = item.key.try_find(ntree)) { + draw_link_errors(C, snode, *link, item.value, invalid_links_block); + } + } + UI_block_end(&C, &invalid_links_block); + UI_block_draw(&C, &invalid_links_block); } /* Draw the breadcrumb on the top of the editor. */ diff --git a/source/blender/editors/space_node/node_intern.hh b/source/blender/editors/space_node/node_intern.hh index d3a1cf4db48..7db272c893a 100644 --- a/source/blender/editors/space_node/node_intern.hh +++ b/source/blender/editors/space_node/node_intern.hh @@ -229,6 +229,10 @@ void NODE_OT_backimage_sample(wmOperatorType *ot); /* `drawnode.cc` */ +float2 socket_link_connection_location(const bNode &node, + const bNodeSocket &socket, + const bNodeLink &link); + NodeResizeDirection node_get_resize_direction(const SpaceNode &snode, const bNode *node, int x,