Signal viewer dock#1174
Draft
tindrew wants to merge 11 commits into
Draft
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a DEBUG-only SignalViewerRuntime (singleton) that registers a global emission callback, forwards batched/rate-limited signal events to the editor; SceneDebugger and ScriptEditorDebugger plumbing route messages; a new SignalizeDock/inspector header exposes editor-side runtime visualization entry points. ChangesSignal viewer + editor integration
Sequence Diagram(s)sequenceDiagram
participant Node as Game Node
participant Obj as Object
participant Runtime as SignalViewerRuntime
participant EngineDbg as EngineDebugger
participant EditorDbg as ScriptEditorDebugger
participant Dock as SignalizeDock
Node->>Obj: emit_signal(name, args...)
Obj->>Obj: capture emission_callback & enabled under lock
Obj-->>Obj: release lock
alt callback enabled and set
Obj->>Runtime: emission_callback(emitter, name, args, argcount)
Runtime->>Runtime: filter, rate-limit, batch
Runtime->>EngineDbg: send_message("signal_viewer:signal_emitted", data)
end
Obj->>Obj: continue normal signal delivery
EngineDbg->>EditorDbg: forward "signal_viewer:signal_emitted"
EditorDbg->>Dock: _on_runtime_signal_emitted(data)
Dock->>Dock: update visualization
sequenceDiagram
participant Editor as Editor UI
participant Inspector as Inspector Panel
participant Plugin as SignalizeInspectorPlugin
participant EditorDbg as ScriptEditorDebugger
participant SceneDbg as SceneDebugger
participant Runtime as SignalViewerRuntime
participant Dock as SignalizeDock
Editor->>Inspector: select node
Inspector->>Plugin: parse_begin(node)
Plugin->>EditorDbg: request node signal data
EditorDbg->>SceneDbg: _msg_signal_viewer_request_node_data(node_id)
SceneDbg->>Runtime: send_node_signal_data(node_id)
Runtime->>EditorDbg: send_message("signal_viewer:node_signal_data", data)
EditorDbg->>Dock: _on_node_signal_data_received(data)
Dock->>Dock: build graph and display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@core/object/object.cpp`:
- Around line 1244-1248: The signal emission callback is being invoked while
holding signal_mutex which can deadlock if the callback re-enters signal paths;
change the code in the emission path to capture the callback pointer and enabled
flag (signal_emission_callback_enabled and signal_emission_callback) while under
signal_mutex, release the lock, and then call the captured callback outside the
mutex scope so the invocation no longer runs with signal_mutex held.
In `@core/object/object.h`:
- Around line 794-815: The static signal_emission_callback and
signal_emission_callback_enabled must be made thread-safe: change their types to
std::atomic<SignalEmissionCallback> and std::atomic<bool> respectively, replace
direct assignments in set_signal_emission_callback with .store(...,
std::memory_order_seq_cst) (or appropriate ordering), and change
get_signal_emission_callback/is_signal_emission_callback_enabled to return
.load(...). Also update the corresponding definitions in object.cpp and every
place that reads these (e.g., emit_signalp) to use .load() when fetching the
callback/flag to avoid data races.
In `@editor/editor_node.cpp`:
- Around line 8462-8467: The new Signalize bottom panel shortcut uses Alt+S
which conflicts with the Shader Editor toggle; update the
ED_SHORTCUT_AND_COMMAND call for "bottom_panels/toggle_signalize_bottom_panel"
in editor_node.cpp to use an unused Alt+<letter> binding (e.g., Alt+I) instead
of Key::S so it no longer collides; locate the SignalizeDock setup around the
SignalizeDock instantiation and the Ref<Shortcut> signalize_shortcut assignment
and change the Key constant passed to ED_SHORTCUT_AND_COMMAND accordingly.
In `@editor/register_editor_types.cpp`:
- Line 36: Remove the unused include of "docks/signalize_dock.h" from
register_editor_types.cpp: locate the `#include` "docks/signalize_dock.h" line and
delete it since SignalizeDock is instantiated in editor_node.cpp (not registered
here) and nothing in register_editor_types.cpp references SignalizeDock or
symbols from signalize_dock.h.
In `@scene/debugger/scene_debugger.cpp`:
- Around line 3166-3175: Guard against a null current scene before dereferencing
it: check SceneTree::get_current_scene() for null and return ERR_UNAVAILABLE (or
handle gracefully) if it's missing, and replace the direct call to
scene_tree->get_current_scene()->get_node(node_path) with
scene_tree->get_current_scene() stored in a local (e.g., current_scene) and use
current_scene->get_node_or_null(node_path) to obtain Node *node, then handle the
null node case without crashing or spamming errors; reference SceneTree,
get_current_scene, get_node_or_null, node_path, and the local node variable when
making these changes.
In `@scene/debugger/signal_viewer_runtime.cpp`:
- Around line 229-242: The current _send_batch_updates drops counts by clearing
signal_emission_counts without sending them; modify
SignalViewerRuntime::_send_batch_updates to iterate over signal_emission_counts
and emit or queue aggregated updates to the EngineDebugger singleton
(EngineDebugger::get_singleton()), e.g. call the appropriate EngineDebugger
method for each signal with its accumulated count (or build a single aggregated
payload) and only then clear or reset signal_emission_counts so no emissions are
lost when signals go quiet between sends.
🧹 Nitpick comments (4)
editor/editor_interface.h (1)
169-169: Parameter naming inconsistent with codebase conventions.Other methods in this file use the
p_prefix for parameters (e.g.,edit_resource(const Ref<Resource> &p_resource),edit_node(Node *p_node),edit_script(const Ref<Script> &p_script, ...)). Consider renaming for consistency.♻️ Suggested fix
- void edit_script_method(Script *script, const StringName &method_name); + void edit_script_method(Script *p_script, const StringName &p_method_name);scene/debugger/scene_debugger.h (1)
129-129: Remove development comment artifact.The "STEP 1:" comment appears to be a development note rather than documentation. Consider replacing it with a cleaner description.
♻️ Suggested fix
- // STEP 1: Signal Viewer - Track signal emissions during gameplay + // Signal Viewer: Track signal emissions during gameplayeditor/debugger/script_editor_debugger.h (1)
309-311: Consider renaming_put_msgwhen exposing publicly.The
_prefix conventionally indicates internal/private methods in Godot. Since this is now part of the public API for SignalizeDock integration, consider renaming toput_msgorsend_debugger_messagefor clarity.♻️ Suggested rename
- // Send message to game process - made public for SignalizeDock - void _put_msg(const String &p_message, const Array &p_data, uint64_t p_thread_id = Thread::MAIN_ID); + // Send message to game process - public for SignalizeDock integration + void put_msg(const String &p_message, const Array &p_data, uint64_t p_thread_id = Thread::MAIN_ID);Note: This would require updating all call sites.
editor/debugger/script_editor_debugger.cpp (1)
992-1013: Gate signal_viewer debug logs behind verbosity/DEBUG.
print_line()on every signal_viewer message (and the node-data log) will flood logs and can impact editor performance. Preferprint_verbose()or wrap with a debug/verbosity setting.🛠️ Example: use print_verbose
- if (p_msg.contains("signal_viewer")) { - print_line(vformat("[ScriptEditorDebugger] Received message: '%s'", p_msg)); - } + if (p_msg.contains("signal_viewer")) { + print_verbose(vformat("[ScriptEditorDebugger] Received message: '%s'", p_msg)); + } ... - print_line(vformat("[ScriptEditorDebugger] Found handler for: '%s'", p_msg)); + print_verbose(vformat("[ScriptEditorDebugger] Found handler for: '%s'", p_msg)); ... - print_line(vformat("[ScriptEditorDebugger] No handler found for: '%s', trying plugins_capture", p_msg)); + print_verbose(vformat("[ScriptEditorDebugger] No handler found for: '%s', trying plugins_capture", p_msg)); ... - print_line(vformat("[ScriptEditorDebugger] plugins_capture returned: %s", parsed ? "TRUE" : "FALSE")); + print_verbose(vformat("[ScriptEditorDebugger] plugins_capture returned: %s", parsed ? "TRUE" : "FALSE"));
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@editor/debugger/script_editor_debugger.cpp`:
- Around line 519-535: The debug prints in _msg_signal_viewer_node_signal_data
(and similarly in _parse_message and the handlers around lines 989-1013) use
print_line() which floods logs during high-frequency signal events; change those
calls to print_verbose() (or wrap them behind an is_debug or verbosity check) so
they only emit when verbose logging is enabled, ensuring you update the calls to
SignalizeDock logging locations and any related print_line() in _parse_message
to use verbose logging or conditional verbosity checks instead.
- Around line 493-508: Update _msg_signal_viewer_signal_emitted to accept both
5- and 6-element payloads: detect p_data.size() == 5 and map elements
accordingly (emitter_id = p_data[0], node_name = p_data[1], node_class =
p_data[2], signal_name = p_data[3], connections = p_data[4]) and set count to
connections.size(); otherwise keep the existing 6-element mapping (using count =
p_data[4] and connections = p_data[5]); ensure you validate types when
extracting ObjectID/String/Array and only early-return on sizes < 5 so
backward-compatible payloads aren't dropped.
In `@scene/debugger/scene_debugger.cpp`:
- Around line 3045-3052: The current node-class filter in the scene debugger
(uses variables emitter_node and node_class and the condition checking
node_class.contains("Editor") || node_class.contains("MenuBar") ||
node_class.contains("Window") || node_class.contains("Popup")) is too broad and
hides legitimate runtime UI; narrow the check by removing generic checks for
"Window" and "Popup" and instead only filter known editor-only class names
(e.g., match exact or prefix like "Editor", "EditorPlugin", "EditorInterface",
"EditorNode", "EditorSelection", "EditorMenuBar") or otherwise restrict to a
whitelist of explicit editor classes, updating the if condition accordingly so
runtime UI nodes are not excluded.
- Around line 3081-3199: Reduce noisy logging by replacing high-frequency
print_line calls with verbose-level logging or gated logs: change print_line to
print_verbose (or wrap calls with a signal_viewer_verbose check) inside
_msg_signal_viewer_start_tracking, _msg_signal_viewer_stop_tracking,
_msg_signal_viewer_request_node_data, and
_msg_signal_viewer_request_node_data_by_path; ensure critical errors still use
print_line while routine status/info uses print_verbose or is skipped when a new
signal_viewer_verbose flag is false, and add/initialize that flag where
SignalViewerRuntime is managed if you choose the gated approach.
🧹 Nitpick comments (4)
core/object/object.cpp (1)
1230-1254: Consider avoiding redundant atomic load.The callback pointer is loaded twice: once at line 1251 (for the boolean check) and again at line 1253. This could be simplified by loading once and reusing.
♻️ Suggested optimization
// STEP 1: Signal emission hook - Declare variables for callback capture // This allows editor tools like the Signal Viewer to track signal emissions globally // We capture the callback while holding the lock, but call it after releasing to prevent deadlocks Object::SignalEmissionCallback emission_callback = nullptr; bool should_call_callback = false; { OBJ_SIGNAL_LOCK SignalData *s = signal_map.getptr(p_name); if (!s) { // ... existing code ... } - // Capture callback state under lock (thread-safe atomic load) - should_call_callback = signal_emission_callback_enabled.load(std::memory_order_acquire) && signal_emission_callback.load(std::memory_order_acquire); - if (should_call_callback) { - emission_callback = signal_emission_callback.load(std::memory_order_acquire); - } + // Capture callback state under lock (thread-safe atomic load) + if (signal_emission_callback_enabled.load(std::memory_order_acquire)) { + emission_callback = signal_emission_callback.load(std::memory_order_acquire); + should_call_callback = (emission_callback != nullptr); + }scene/debugger/signal_viewer_runtime.cpp (3)
145-145: Remove or guard debug print statements.This
print_lineis called for every tracked signal emission, which could significantly impact performance and flood the output even in regular debug builds. Consider removing these print statements or guarding them with a verbosity check, since the PR description mentions "Verbosity Control for logging."♻️ Suggested fix
- // DEBUG: Only print after filtering (much less spam) - print_line(vformat("[Signal Viewer Runtime] Tracking: %s.%s", node_name, signal_name)); + // Verbose logging can be re-enabled for deep debugging if needed + // print_line(vformat("[Signal Viewer Runtime] Tracking: %s.%s", node_name, signal_name));Or introduce a verbosity flag:
if (runtime->verbose_logging) { print_line(vformat("[Signal Viewer Runtime] Tracking: %s.%s", node_name, signal_name)); }
224-224: Consider removing or making debug print statements conditional.Multiple
print_linecalls throughout this file will produce significant output during normal operation. For a debug/visualization tool, this level of logging can be distracting and impact performance. Consider:
- Removing these print statements entirely
- Adding a verbosity setting (the PR mentions "Verbosity Control")
- Using a macro that can be disabled
Also applies to: 236-236, 258-258, 288-288, 300-300, 321-321, 389-390, 403-403, 406-406
130-142: Hardcoded class filtering may need maintenance.This extensive list of hardcoded class names for filtering (VScrollBar, HScrollBar, Label, Button, etc.) could become outdated as new UI classes are added. Consider using class inheritance checks (e.g.,
Controlcast) or a configurable filter list. However, the commented-out Control filter at lines 101-104 suggests this was intentional for demo support.
Comment on lines
+493
to
+508
| void ScriptEditorDebugger::_msg_signal_viewer_signal_emitted(uint64_t p_thread_id, const Array &p_data) { | ||
| // Forward signal emission data to SignalizeDock | ||
| // Data format: [emitter_id, node_name, node_class, signal_name, count, connections_array] | ||
|
|
||
| if (p_data.size() < 6) { | ||
| print_line("[ScriptEditorDebugger] WARNING: Invalid signal_viewer data, expected 6 elements"); | ||
| return; | ||
| } | ||
|
|
||
| // Extract the data | ||
| ObjectID emitter_id = p_data[0]; | ||
| String node_name = p_data[1]; | ||
| String node_class = p_data[2]; | ||
| String signal_name = p_data[3]; | ||
| int count = p_data[4]; | ||
| Array connections = p_data[5]; |
There was a problem hiding this comment.
Align signal_viewer payload schema to avoid dropping emissions.
_msg_signal_viewer_signal_emitted() requires 6 elements (including count), but the runtime sender currently builds a 5‑element payload (no count). This makes the handler return early and the dock never updates. Consider supporting both formats or update the sender.
🛠️ Backward‑compatible handler fix
- if (p_data.size() < 6) {
- print_line("[ScriptEditorDebugger] WARNING: Invalid signal_viewer data, expected 6 elements");
+ if (p_data.size() < 5) {
+ print_line("[ScriptEditorDebugger] WARNING: Invalid signal_viewer data, expected 5+ elements");
return;
}
// Extract the data
ObjectID emitter_id = p_data[0];
String node_name = p_data[1];
String node_class = p_data[2];
String signal_name = p_data[3];
- int count = p_data[4];
- Array connections = p_data[5];
+ int count = 1;
+ Array connections;
+ if (p_data.size() == 5) {
+ connections = p_data[4];
+ } else {
+ count = p_data[4];
+ connections = p_data[5];
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void ScriptEditorDebugger::_msg_signal_viewer_signal_emitted(uint64_t p_thread_id, const Array &p_data) { | |
| // Forward signal emission data to SignalizeDock | |
| // Data format: [emitter_id, node_name, node_class, signal_name, count, connections_array] | |
| if (p_data.size() < 6) { | |
| print_line("[ScriptEditorDebugger] WARNING: Invalid signal_viewer data, expected 6 elements"); | |
| return; | |
| } | |
| // Extract the data | |
| ObjectID emitter_id = p_data[0]; | |
| String node_name = p_data[1]; | |
| String node_class = p_data[2]; | |
| String signal_name = p_data[3]; | |
| int count = p_data[4]; | |
| Array connections = p_data[5]; | |
| void ScriptEditorDebugger::_msg_signal_viewer_signal_emitted(uint64_t p_thread_id, const Array &p_data) { | |
| // Forward signal emission data to SignalizeDock | |
| // Data format: [emitter_id, node_name, node_class, signal_name, count, connections_array] | |
| if (p_data.size() < 5) { | |
| print_line("[ScriptEditorDebugger] WARNING: Invalid signal_viewer data, expected 5+ elements"); | |
| return; | |
| } | |
| // Extract the data | |
| ObjectID emitter_id = p_data[0]; | |
| String node_name = p_data[1]; | |
| String node_class = p_data[2]; | |
| String signal_name = p_data[3]; | |
| int count = 1; | |
| Array connections; | |
| if (p_data.size() == 5) { | |
| connections = p_data[4]; | |
| } else { | |
| count = p_data[4]; | |
| connections = p_data[5]; | |
| } |
🤖 Prompt for AI Agents
In `@editor/debugger/script_editor_debugger.cpp` around lines 493 - 508, Update
_msg_signal_viewer_signal_emitted to accept both 5- and 6-element payloads:
detect p_data.size() == 5 and map elements accordingly (emitter_id = p_data[0],
node_name = p_data[1], node_class = p_data[2], signal_name = p_data[3],
connections = p_data[4]) and set count to connections.size(); otherwise keep the
existing 6-element mapping (using count = p_data[4] and connections =
p_data[5]); ensure you validate types when extracting ObjectID/String/Array and
only early-return on sizes < 5 so backward-compatible payloads aren't dropped.
Comment on lines
+3045
to
+3052
| // Filter out editor UI noise | ||
| String node_class = emitter_node->get_class(); | ||
| String node_name = emitter_node->get_name(); | ||
|
|
||
| // Skip common editor UI classes (though these shouldn't appear in gameplay) | ||
| if (node_class.contains("Editor") || node_class.contains("MenuBar") || | ||
| node_class.contains("Window") || node_class.contains("Popup")) { | ||
| return; |
There was a problem hiding this comment.
Class-name filter is too broad and can hide legitimate runtime UI signals.
Filtering out "Window"/"Popup" will exclude common game UI nodes, reducing the tool’s usefulness. Consider limiting this to known editor-only classes or removing the filter entirely.
✂️ Example narrowing
- if (node_class.contains("Editor") || node_class.contains("MenuBar") ||
- node_class.contains("Window") || node_class.contains("Popup")) {
+ if (node_class.contains("Editor") || node_class.contains("MenuBar")) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Filter out editor UI noise | |
| String node_class = emitter_node->get_class(); | |
| String node_name = emitter_node->get_name(); | |
| // Skip common editor UI classes (though these shouldn't appear in gameplay) | |
| if (node_class.contains("Editor") || node_class.contains("MenuBar") || | |
| node_class.contains("Window") || node_class.contains("Popup")) { | |
| return; | |
| // Filter out editor UI noise | |
| String node_class = emitter_node->get_class(); | |
| String node_name = emitter_node->get_name(); | |
| // Skip common editor UI classes (though these shouldn't appear in gameplay) | |
| if (node_class.contains("Editor") || node_class.contains("MenuBar")) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@scene/debugger/scene_debugger.cpp` around lines 3045 - 3052, The current
node-class filter in the scene debugger (uses variables emitter_node and
node_class and the condition checking node_class.contains("Editor") ||
node_class.contains("MenuBar") || node_class.contains("Window") ||
node_class.contains("Popup")) is too broad and hides legitimate runtime UI;
narrow the check by removing generic checks for "Window" and "Popup" and instead
only filter known editor-only class names (e.g., match exact or prefix like
"Editor", "EditorPlugin", "EditorInterface", "EditorNode", "EditorSelection",
"EditorMenuBar") or otherwise restrict to a whitelist of explicit editor
classes, updating the if condition accordingly so runtime UI nodes are not
excluded.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scene/debugger/signal_viewer_runtime.cpp`:
- Around line 180-208: The per-signal send can emit zero-count updates
immediately after runtime->_send_batch_updates() clears counts; fix by checking
whether a batch send just occurred and skip the per-signal send when the count
is zero. Concretely: after computing current_time and the batch-send block (the
call to runtime->_send_batch_updates() and setting
runtime->last_batch_update_time = current_time), read count =
runtime->signal_emission_counts[key] and then only call
runtime->_send_signal_update(...) if should_send && count > 0 (or skip when
runtime->last_batch_update_time == current_time && count == 0). Update
signal_last_sent_time and reset signal_emission_counts[key] only when you
actually send.
🧹 Nitpick comments (1)
scene/debugger/signal_viewer_runtime.cpp (1)
279-301: Clear tracking state when stopping.
If tracking is toggled on/off, stale counts and connections can persist. Consider clearing maps and timestamps on stop to keep sessions clean.♻️ Suggested cleanup
void SignalViewerRuntime::stop_tracking() { if (!tracking_enabled) { return; // Already not tracking } // Unregister the callback Object::set_signal_emission_callback(nullptr); tracking_enabled = false; + + signal_emission_counts.clear(); + signal_connections.clear(); + signal_last_sent_time.clear(); + last_batch_update_time = 0; print_line("[Signal Viewer Runtime] Signal tracking disabled"); }
Comment on lines
+180
to
+208
| // Get current time | ||
| uint64_t current_time = OS::get_singleton()->get_ticks_msec(); | ||
|
|
||
| // Check if we should send an update (rate limiting) | ||
| bool should_send = false; | ||
| if (!runtime->signal_last_sent_time.has(key)) { | ||
| // First time seeing this signal - send immediately | ||
| should_send = true; | ||
| } else { | ||
| uint64_t last_sent = runtime->signal_last_sent_time[key]; | ||
| if (current_time - last_sent >= RATE_LIMIT_MS) { | ||
| should_send = true; | ||
| } | ||
| } | ||
|
|
||
| // Also check if we need to do a batch update | ||
| if (current_time - runtime->last_batch_update_time >= BATCH_UPDATE_INTERVAL_MS) { | ||
| runtime->_send_batch_updates(); | ||
| runtime->last_batch_update_time = current_time; | ||
| } | ||
|
|
||
| // Send update if rate limit allows | ||
| if (should_send) { | ||
| int count = runtime->signal_emission_counts[key]; | ||
| Array connections = runtime->signal_connections.has(key) ? runtime->signal_connections[key] : Array(); | ||
| runtime->_send_signal_update(key, emitter_node->get_instance_id(), node_name, node_class, signal_name, count, connections); | ||
| runtime->signal_last_sent_time[key] = current_time; | ||
| runtime->signal_emission_counts[key] = 0; // Reset count after sending | ||
| } |
There was a problem hiding this comment.
Avoid zero-count updates right after batch sends.
When Line 196 triggers _send_batch_updates(), counts are cleared, but the per-signal send still runs and can emit count = 0 (e.g., first signal after the batch interval). Gate the per-signal send when a batch send has just occurred.
🔧 Suggested fix
- // Also check if we need to do a batch update
- if (current_time - runtime->last_batch_update_time >= BATCH_UPDATE_INTERVAL_MS) {
- runtime->_send_batch_updates();
- runtime->last_batch_update_time = current_time;
- }
+ // Also check if we need to do a batch update
+ bool did_batch_send = false;
+ if (current_time - runtime->last_batch_update_time >= BATCH_UPDATE_INTERVAL_MS) {
+ runtime->_send_batch_updates();
+ runtime->last_batch_update_time = current_time;
+ did_batch_send = true;
+ }
// Send update if rate limit allows
- if (should_send) {
+ if (should_send && !did_batch_send) {
int count = runtime->signal_emission_counts[key];
Array connections = runtime->signal_connections.has(key) ? runtime->signal_connections[key] : Array();
runtime->_send_signal_update(key, emitter_node->get_instance_id(), node_name, node_class, signal_name, count, connections);
runtime->signal_last_sent_time[key] = current_time;
runtime->signal_emission_counts[key] = 0; // Reset count after sending
}🤖 Prompt for AI Agents
In `@scene/debugger/signal_viewer_runtime.cpp` around lines 180 - 208, The
per-signal send can emit zero-count updates immediately after
runtime->_send_batch_updates() clears counts; fix by checking whether a batch
send just occurred and skip the per-signal send when the count is zero.
Concretely: after computing current_time and the batch-send block (the call to
runtime->_send_batch_updates() and setting runtime->last_batch_update_time =
current_time), read count = runtime->signal_emission_counts[key] and then only
call runtime->_send_signal_update(...) if should_send && count > 0 (or skip when
runtime->last_batch_update_time == current_time && count == 0). Update
signal_last_sent_time and reset signal_emission_counts[key] only when you
actually send.
Arctis-Fireblight
previously requested changes
Mar 8, 2026
Contributor
Arctis-Fireblight
left a comment
Arctis-Fireblight
left a comment
There was a problem hiding this comment.
@tindrew We probably need to modify the logging for Signalize. As is, it is logging all signals into the log output even when I am not using the signal debugger. This spams the logs as it is and kinda makes them unusable.
I see several code rabbit suggestions about using verbose logging, but I am thinking we probably need to add controls for this, to turn logging on and off.
In addition, we will probably need to start working on comprehensive tutorials on how to use this.
…sabled the Build Graph Button for now.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/docks/signalize_dock.h (1)
286-339:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict the inspector plugin to actual remote-node inspections.
can_handle()now returnstruefor everyNode, andparse_begin()always calls_on_node_inspected_in_remote_tree(...). Selecting a regular local scene node will therefore drive the dock down the remote-inspection path too. The comment on Line 297 says this will be filtered later, but that guard never happens here.🔧 Minimal guard
virtual bool can_handle(Object *p_object) override { if (!signal_viewer_dock) { return false; } // Only handle Node objects Node *node = Object::cast_to<Node>(p_object); - if (!node) { + if (!node || !signal_viewer_dock->tracking_runtime_scene) { return false; } - // Check if this node has a "Node/path" property (indicates it's from Remote tree) - // Actually, let's just handle ALL nodes and check in parse_begin return true; }scene/debugger/signal_viewer_runtime.cpp (1)
143-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh cached connection metadata on every emission.
get_signal_connection_list()already gives you the current runtime connections here, butsignal_connections[key]is only populated the first time a signal is seen. Any laterconnect()/disconnect()leaves both the immediate update path and the batch path rendering stale targets and methods for that signal.🔧 Minimal fix
if (!runtime->signal_emission_counts.has(key)) { runtime->signal_emission_counts[key] = 0; + } - // First time seeing this signal - collect and store connection info - Array connections_array; - for (const Object::Connection &conn : conns) { - Array conn_data; - Object *target = conn.callable.get_object(); - if (target) { - conn_data.append(target->get_instance_id()); - - // Get the target node's name if it's a Node, otherwise use class - Node *target_node = Object::cast_to<Node>(target); - if (target_node) { - conn_data.append(target_node->get_name()); // Use actual node name - conn_data.append(target_node->get_class()); - } else { - conn_data.append(target->get_class()); // Use class as name for non-Node objects - conn_data.append(target->get_class()); - } - - conn_data.append(String(conn.callable.get_method())); - connections_array.append(conn_data); - } - } - runtime->signal_connections[key] = connections_array; + // Rebuild connection metadata from the current connection list. + Array connections_array; + for (const Object::Connection &conn : conns) { + Array conn_data; + Object *target = conn.callable.get_object(); + if (!target) { + continue; + } + + conn_data.append(target->get_instance_id()); + if (Node *target_node = Object::cast_to<Node>(target)) { + conn_data.append(target_node->get_name()); + conn_data.append(target_node->get_class()); + } else { + conn_data.append(target->get_class()); + conn_data.append(target->get_class()); + } + conn_data.append(String(conn.callable.get_method())); + connections_array.append(conn_data); } + runtime->signal_connections[key] = connections_array;Also applies to: 196-197, 267-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/debugger/signal_viewer_runtime.cpp` around lines 143 - 169, The code currently populates runtime->signal_connections[key] only the first time (guarded by runtime->signal_emission_counts.has(key)), causing stale connection metadata after connect()/disconnect(); change the logic in the emission handling to always (re)build and assign connections_array from the current conns returned by get_signal_connection_list() for the given key (i.e. remove or bypass the one-time-only branch and ensure runtime->signal_connections[key] = connections_array on every emission), and apply the same fix to the other similar blocks that populate signal_connections (the blocks referenced by the reviewer around the other occurrences).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scene/debugger/signal_viewer_runtime.cpp`:
- Around line 143-169: The code currently populates
runtime->signal_connections[key] only the first time (guarded by
runtime->signal_emission_counts.has(key)), causing stale connection metadata
after connect()/disconnect(); change the logic in the emission handling to
always (re)build and assign connections_array from the current conns returned by
get_signal_connection_list() for the given key (i.e. remove or bypass the
one-time-only branch and ensure runtime->signal_connections[key] =
connections_array on every emission), and apply the same fix to the other
similar blocks that populate signal_connections (the blocks referenced by the
reviewer around the other occurrences).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff879b36-1aa6-48e6-a6e4-c38202b73f88
📒 Files selected for processing (7)
editor/debugger/script_editor_debugger.cppeditor/docks/signalize_dock.cppeditor/docks/signalize_dock.hscene/debugger/scene_debugger.cppscene/debugger/scene_debugger.hscene/debugger/signal_viewer_runtime.cppscene/debugger/signal_viewer_runtime.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds SIgnalize, a visual tool for viewing and understanding signal connections between nodes in your Redot scenes.
Key Features:
- Shows static signal connections defined in your scene
- Can refresh/rebuild the graph with the "Build Graph" button
- Tracks signal emissions in real-time
- Shows which signals are actually being fired
- Can inspect individual nodes from the remote tree
- Search/Filter: Filter which nodes to display
- Customization: Change connection line colors
- Floating Window: Can detach from editor as a floating window
- Verbosity Control: Adjust how much logging is shown
- Per-Node Inspection: Click a node in the remote tree to see its specific signals
Use Cases:
Screen.Recording480.2026-01-25.at.4.06.34.AM.mov
Summary by CodeRabbit
New Features
Refactor
Documentation