Navigation Menu

Skip to content
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
Jump to bottom

Huge Debugger/EditorDebugger refactor. #36244

Merged
merged 1 commit into from Feb 21, 2020
Merged

Huge Debugger/EditorDebugger refactor. #36244

merged 1 commit into from Feb 21, 2020

Conversation

Faless
Copy link
Collaborator

Changes both script debugger and editor debugger.

What this PR does:

Core

  • ScriptDebuggerRemote is back to core/. Most of the serialization/parsing is done via classes/structs.
  • All messages are now an Array, containing 2 elements, a String (the command name) and another array (the data for that message, could be empty, but must be present).
  • Most messages are sanitezed (for size, and some for type)
  • Live editing/inspection code is now disabled in release builds (with the exception of stack vars, but not for objects. We could change that too.).
  • The code to handle live editing/inspection, is now in scene/debugger/scene_debugger.cpp (let me know if you prefer it to just be scene/scene_debugger.cpp). Most of the live-edit code from SceneTree has also been moved to that file.
  • Current ProcessID is sent to debugger if one is attached.

Editor

  • Multiple debug session support
  • Splitted into ScriptEditorDebugger (should be EditorDebuggerSession), EditorDebuggerNode (handles multiple sessions).
  • Added EditorDebuggerTree and EditorDebuggerInspector to handle remote tree view, and stack var inspector (plus remote view inspector cache).
  • Code is moved to editor/debugger/ (profilers should also be moved there too).
  • The debugger has now it's own plugin, added by SceneTree directly and not by ScriptDebugger (it is also a singleton, but it could be exposed via EditorNode::get_debugger_node() instead). Do we want to keep or drop the various singletons in editor?
  • Scene/script changes are synced to all attached sessions
  • Most of the stuff the worked should still work, possibly better, report any issue you might find. *Note: it seems to me that memory usage is disabled for now in the vulkan branch :( *
  • EditorRun now handles multiple process, debugger read each connected ProcessID from network, but only allows focus steal and process kill to actually started child processes (for security).
  • Added a new radio to select one or two instances to run.

multi_small

debugger_showoff

What this PR does not do (but should be done!):

  • Fully abstract network functions, optimize network usage, use thread (that will be done when I port WebSocket profiler for HTML5 platform. #33925 to all this new changes).
  • Normalize message format like [prefix]:[message] (core, scripts, scene, servers, etc.)
  • A generic CoreDebugger on which we can register captures and profilers:
    • Captures to allow parsing messages with a given prefix. (script debug is a capture, live debug is a capture, memory usage is a capture, etc)
    • Profilers to allow callbacks on profiling enable, disable, and tick (network profiler, visual profiler, servers profiler, ecc).
    • All this could even be exposed to scripts.

@Faless Faless added this to the 4.0 milestone Feb 15, 2020
@Faless Faless requested a review from neikeq as a code owner February 15, 2020 13:12
@Zireael07
Copy link
Contributor

Yay!

Hopefully after this lands, the remote tree issues I keep spotting will get fixed.

How does the sanitizer work? If some value is mangled/nonsensical, does it get quietly dropped or is there an error message shown? Does it crash? The "crash without any errors" behavior is still the most annoying part of #29533 ...

@Faless
Copy link
Collaborator Author

If some value is mangled/nonsensical, does it get quietly dropped or is there an error message shown?

Dropped, and the editor should show an error ERR_FAIL_COND.

Does it crash?

It should not, it should never, but it still might if I left some bug in it :-P

@Faless Faless force-pushed the debugger/big_refactor_squash branch from 2e60f85 to eec29ca Compare February 15, 2020 14:55
@Faless Faless mentioned this pull request Feb 15, 2020
Closed
@Razoric480 Razoric480 mentioned this pull request Feb 16, 2020
Closed
@Faless Faless mentioned this pull request Feb 17, 2020
Closed
@Faless Faless force-pushed the debugger/big_refactor_squash branch 2 times, most recently from 6bb0666 to 816d122 Compare February 20, 2020 18:04
@Faless Faless force-pushed the debugger/big_refactor_squash branch from 816d122 to cbc450c Compare February 21, 2020 10:12
@akien-mga akien-mga merged commit a24aafc into godotengine:master Feb 21, 2020
@akien-mga
Copy link
Member

Thanks a ton!

@YeldhamDev
Copy link
Member

This doesn't look right:
Screenshot_20200221_092248

@YeldhamDev
Copy link
Member

YeldhamDev commented Feb 21, 2020 •
edited

"Debugger" should be "Session 1", but session 2 is the one with that label.

nathanfranke
nathanfranke reviewed Feb 24, 2020
View reviewed changes
editor/debugger/script_editor_debugger.cpp
@@ -2508,6 +1702,7 @@ ScriptEditorDebugger::ScriptEditorDebugger(EditorNode *p_editor) {
visual_profiler->set_name(TTR("Visual Profiler"));
tabs->add_child(visual_profiler);
visual_profiler->connect_compat("enable_profiling", this, "_visual_profiler_activate");
visual_profiler->connect_compat("break_request", this, "_profiler_seeked");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rebase seemed to add this line back #36340 (See the diff)

This line causes an error since the signal break_request does not exist.

Edit: Quickly made this PR #36502

nathanfranke added a commit to nathanfranke/godot that referenced this pull request Feb 24, 2020
@nathanfranke
Remove this signal call that was mistakenly added in godotengine#36244
e0bb28c 
The original change was in godotengine#36340
akien-mga added a commit that referenced this pull request Feb 25, 2020
@akien-mga
Merge pull request #36502 from nathanwfranke/re-remove-invalid-signal…
a4125bb 
…-connect

Re-Remove this signal call that was mistakenly added in #36244
@Faless Faless mentioned this pull request Feb 25, 2020
Closed
@Faless Faless mentioned this pull request Mar 3, 2020
Merged
@Calinou Calinou mentioned this pull request Mar 6, 2020
Closed
@Faless Faless mentioned this pull request Mar 9, 2020
Merged
@Razoric480 Razoric480 mentioned this pull request Apr 23, 2020
Merged
@YuriSizov YuriSizov mentioned this pull request Jul 6, 2020
Merged
@raulsntos raulsntos mentioned this pull request Jan 5, 2022
Closed
reduz added a commit to reduz/godot that referenced this pull request Jan 7, 2023
@reduz
Error when removing a phycics node during a physics callback
398e73c 
* This behavior is not allowed, the error text suggests using call_deferred().
* Added a check in Node::remove_child to prevent future crashes of this type.
* Fixed a performance regression introduced by godotengine#36244.

Fixes godotengine#63718, probably other crashes too.
@reduz reduz mentioned this pull request Jan 7, 2023
Merged
the-brickster pushed a commit to the-brickster/godot that referenced this pull request Feb 9, 2023
@reduz @the-brickster
Error when removing a phycics node during a physics callback
c067190 
* This behavior is not allowed, the error text suggests using call_deferred().
* Added a check in Node::remove_child to prevent future crashes of this type.
* Fixed a performance regression introduced by godotengine#36244.

Fixes godotengine#63718, probably other crashes too.
Streq pushed a commit to Streq/godot that referenced this pull request Feb 9, 2023
@reduz @Streq
Error when removing a phycics node during a physics callback
4737f76 
* This behavior is not allowed, the error text suggests using call_deferred().
* Added a check in Node::remove_child to prevent future crashes of this type.
* Fixed a performance regression introduced by godotengine#36244.

Fixes godotengine#63718, probably other crashes too.
@eskandrej eskandrej mentioned this pull request Jun 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanfranke nathanfranke nathanfranke left review comments

@neikeq neikeq Awaiting requested review from neikeq

Assignees
No one assigned
Labels
enhancement topic:editor topic:network usability
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Faless @Zireael07 @akien-mga @YeldhamDev @nathanfranke