Improve GDExtension Tools Integration with Editor Debug Tooling

[Naros]

This PR aims to address several issues around GDExtension and Debug Tooling within the Editor.

• Bugsquad edit, closes: Improve GDExtension Tooling with Editor Debugger Tools godot-proposals#8777

[AThousandShips]

Please open a proposal to track this and discuss the specifics of this feature

[Naros]

@AThousandShips done - godotengine/godot-proposals#8777
Will take a look at the formatting issues now.

[Naros]

Hi, would someone me able to tell me whether the failure is related to my change or something that pre-exists in main?

[dsnopek]

Thanks!

At a high-level, I think this is a great addition, since it's making something that's possible via Godot modules also be possible via GDExtension (and addons).

I'm not really familiar with the debugger API, but for the most part, the code here looks like a thin layer over ScriptDebugger from core/debugger/script_debugger.h, which seems good to me. But it should probably be reviewed by someone more familiar with this API (@Faless?) to see if the API that's being exposed really makes sense.

[Naros]

I'd also add that if there are other bits from SccriptDebugger that make sense to include, let me know. I basically grabbed just what I needed for the short-term but perhaps there are others that may warrant inclusion.

[vnen]

I'm not the most familiar with the debugger code, but this is mostly exposing what's there for extensions. Overall it looks fine to me.

[akien-mga]

Could you squash the commits? See PR workflow for instructions.

[Faless]

Do we really want to expose ScriptDebugger as a separate class?

I feel that class exists in core mostly for historical reasons.

Does it make sense to move the exposed functions directly to EngineDebugger?

E.g.:

core_bind::EngineDebugger::insert_breakpoint

[dsnopek]

@Faless:

Do we really want to expose ScriptDebugger as a separate class?

I feel that class exists in core mostly for historical reasons.

Does it make sense to move the exposed functions directly to EngineDebugger?

Unfortunately, you may be the person most qualified to answer the questions you pose. :-)

[Naros]

Do we really want to expose ScriptDebugger as a separate class?

I feel that class exists in core mostly for historical reasons.

Does it make sense to move the exposed functions directly to EngineDebugger?

I have to agree with David, really entirely your call, I can certainly align those there if you'd prefer.

[Faless]

Unfortunately, you may be the person most qualified to answer the questions you pose. :-)

I have to agree with David, really entirely your call, I can certainly align those there if you'd prefer.

Given there doesn't seem to be any objection, I will say let's do that.

It's already confusing enough to have EngineDebugger and EditorDebugger, adding a ScriptDebugger will make it even worse.

I already had plans to move more of the debugger default features to the plugin interface (while still shipping them as part of Godot), so I will take a chance there to see if we can get rid of the whole ScriptDebugger concept internally too.

[Naros]

Sounds good, then I'll amend the PR over the weekend and ping you when its ready for a review from your side on the particulars to make sure that the direction and goals are aligned to what you have in mind 👍 @Faless

[Naros]

Hi @Faless, I have a few questions about this work.

I was reviewing and testing my changes in a local build to make sure I hadn't missed any additions needed for this to work well for our script language plug-in, and I came across a hack I had used to integrate with the goto_script_line signal.

TypedArray<Node> nodes = editor_node->find_children("*", "EditorDebuggerNode", true, false);
if (nodes.size() == 1) {
  // Wire this signal so when user selects a frame we can handle that here.
  Node* editor_debugger_node = Object::cast_to<Node>(nodes[0]);
  editor_debugger_node->connect("goto_script_line", callable_mp(this, &OrchestratorScriptView::_goto_script_line));
}

The ScriptEditor hooks into this directly into the engine to advance the text editor position in the script view to the correct line based on the chosen breakpoint; however, as our plug-in isn't based on the ScriptEditor classes, we cannot take advantage of that callback. I want to find a reasonable way to expose this signal.

What I was considering was adding a new method on EditorDebuggerPlugin, i.e.:

void goto_script_line(const Ref<Script>& p_script, int p_line);

Then, inside EditorDebuggerNode::_error_selected and _text_editor_stack_goto where, we emit the goto_script_line signal that we iterate the registered debugger plugins and dispatch to this new method, allowing the plug-in an opportunity to react. There may be other alternatives for this, but it seemed like the most likely candidate.

The last task I have open is to check how GDScript integrates with the Breakpoints bottom panel. I want my script tooling to add the breakpoints to that panel, much like how the gutter breakpoints work in GDScript. If you have any pointers, I'd be grateful for the advice. I should have that worked out tomorrow with any last questions before this is ready.

So it seems EditorDebuggerSession::set_breakpoint is what I want to add/remove breakpoints. I have a question about usage. Is it safe then for my EditorDebuggerPlugin to cache the EditorDebuggerSession or its ID (as I think there should only be one active), and then from my code, I can call into the debugger plugin, use the session ID to set/unset the breakpoint then, right? I tested this and it seems to work, but just unsure about there being only one session.

In addition, I also need a way to react to breakpoint_set_in_tree and breakpoints_cleared_in_tree, which are signals that are also emitted by the EditorDebuggerNode, when the user uses the context-menu on the breakpoints in the panel. Again, delegate these to the Plugin or should we consider a different way?

[Naros]

The build failures seem to be unrelated as master seems to be unstable atm.

[akien-mga]

This should be fixed after rebasing.

[Naros]

@akien-mga @Faless is it possible to still get this into 4.3? It would be super helpful for us to have debugger support in the editor and this is a blocker for that.

[AThousandShips]

It's marked for 4.3 so unless that is changed it's still planned for 4.3, wait and see

[Faless]

Fantastic job! 🏆 , and sorry for the long delay between reviews.

I was worried of the potential misuse of set_lines/set_depth which can potentially break debugging but I couldn't figure out a better solution, and it's better to have this than not in 4.3.

So I've approved this, but also suggested marking those methods as experimental.

Thank you for your patience and the great work.

[Faless]

Is it safe then for my EditorDebuggerPlugin to cache the EditorDebuggerSession or its ID (as I think there should only be one active),

The ID is a bit of an implementation detail, but it's indeed the "tab number".
In general, it should be fine caching between then signals EditorDebuggerSession.started and EditorDebuggerSession.stopped.

I also need a way to react to breakpoint_set_in_tree and breakpoints_cleared_in_tree, which are signals that are also emitted by the EditorDebuggerNode, when the user uses the context-menu on the breakpoints in the panel. Again, delegate these to the Plugin or should we consider a different way?

I think indeed those signals could be exposed via the EditorDebuggerPlugin given they are "global" (i.e. are relevant to all sessions).

Maybe goto_script_line could also be exposed this way?

[Naros]

Thanks for the feedback @Faless, I've addressed your recommendations with the documentation and I've added the 3 methods as virtual methods on the EditorDebuggerPlugin interface. Assuming the build succeeds, let me know if there is anything else you would like to see for this.

[akien-mga]

Thanks!

[Faless]

I've added the 3 methods as virtual methods on the EditorDebuggerPlugin interface.

Oh no... as I mentioned in my comment, those should probably have been signals not virtual methods :(

[Naros]

I've added the 3 methods as virtual methods on the EditorDebuggerPlugin interface.

Oh no... as I mentioned in my comment, those should probably have been signals not virtual methods :(

No worries, I can send another PR to follow-up on this if that's acceptable @Faless @akien-mga, just lmk.

[Faless]

@Naros I'll let you know in case, it's not the end of the world indeed, just a bit weird, we usually prefer virtual methods for things that needs overriding and either are frequently called (they're faster than signals), or when we need to receive a return (which is not the case for those function).

At the same time, the plugin aims to be "self-contained", so the virtual method can make sense, and might not be worth changing.

Sorry for the noise, I think in the end we'll leave it as it is now 😅