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

Enhanced debugger for godot 3.0 #11940

Merged
merged 4 commits into from
Nov 20, 2017
Merged

Enhanced debugger for godot 3.0 #11940

merged 4 commits into from
Nov 20, 2017

Conversation

Geequlim
Copy link
Contributor

@Geequlim Geequlim commented Oct 8, 2017

Some improvements for the debugger of godot 3.0. As disscused with @reduz in #10891.

The PR will take the enhanced debugger like the cuurent debugger 2.1.4 to the master branch.

  • Make it possiable to access members and constants from all kind of script languages.
  • Send script varialbes and constants when inspector remote objects.
  • Improved property editor to interact with remote objects.
  • Merge debugger inspector with normal inspector.
  • Merge live scene tree panel with normal scene tree.
  • Implement a better dictionary editor than the 2.1 version.

@groud groud added this to the 3.0 milestone Oct 8, 2017
@reduz
Copy link
Member

reduz commented Oct 9, 2017

Did you see my proposal in the 2.1 issue? Seems like it from your two last checkboxes, but making sure :P
I would also add that live editing should probably be enabled by default too.

@Geequlim
Copy link
Contributor Author

2017-10-15 00-38-34
@reduz I have implemented the inspector to show and edit remote objects now but the live scene tree still stays here.

@reduz
Copy link
Member

reduz commented Oct 14, 2017

looking great!

@Geequlim
Copy link
Contributor Author

2017-10-15 00-51-40
Why I didn't move the Live Scene Tree to the normal Scene Tree dock is I didn't find a good way to manage the tree with the debugger but it is contained in another place.

That would make the code more mess 😕

@reduz
Copy link
Member

reduz commented Oct 14, 2017 via email

@Geequlim
Copy link
Contributor Author

Geequlim commented Oct 17, 2017

The commits of this PR is not event to my origin branch: debugger after I git push --force to it.
Could you please give me a help about this?

@Geequlim
Copy link
Contributor Author

Seems the github delayed to synchronize the commit from the origin branch.

@Geequlim
Copy link
Contributor Author

@reduz Are you going to have a review and mege this before alpha2 ?

@reduz
Copy link
Member

reduz commented Nov 13, 2017

Hi @Geequlim I was planning this week to add the tools so you can properly implement:
"Merge live scene tree panel with normal scene tree.", in hopes of having it implemented before beta. It seems you completed the work for everything else, so will try to have this working ASAP

@Geequlim
Copy link
Contributor Author

@reduz Great!
I hope we can finish this work within this week so it can be merge before the 3.0 BETA . Then I have enough time to fix bugs before the 3.0 stable.

@MarianoGnu
Copy link
Contributor

why not adding the remote scene tree to a dock in the same slot as Scene and make it active when debugging? (or add a setting to allow enable it automatically)
Other way (here comes a big complain, because of extreme and unneded complexity) is adding a "remote scene" to scene tabs, with the X button not enabled, then you can see the scene in realtime while the game is runing (you have a duplicate of the nodes with the same property values, but scripts disabled)

@reduz
Copy link
Member

reduz commented Nov 16, 2017

@Geequlim alright, added the support you were missing. You can acess SceneTreeDock by using

EditorNode::get_singleton()->get_scene_tree_dock()

so, basically add your remote tree editor with

EditorNode::get_singleton()->get_scene_tree_dock()->add_remote_tree_editor(editor)

When debug begins, show it by doing:

EditorNode::get_singleton()->get_scene_tree_dock()->show_remote_tree()

and when debug ends, hide it by doing:

EditorNode::get_singleton()->get_scene_tree_dock()->hide_remote_tree()

and the remote debug menu can go away!

@Geequlim Geequlim changed the title [WIP] Enhanced debugger for godot 3.0 Enhanced debugger for godot 3.0 Nov 16, 2017
@Geequlim
Copy link
Contributor Author

All features are implemented now.
It is ready for merging.

@akien-mga
Copy link
Member

Some screenshots to build up the hype? :D

@Geequlim
Copy link
Contributor Author

@akien-mga Here we go!

  • Explore remote member variables with dictionary support in breakpoint stacks.
    remote member variables

  • Live explore the remote scene tree
    Live explore the remote scene tree

  • Edit remote objects from the inspector directly
     Edit remote objects

@SirPigeonz
Copy link
Contributor

This is excelsior!

@@ -532,56 +553,77 @@ void ScriptDebuggerRemote::_send_object_id(ObjectID p_id) {
if (!obj)
return;

List<PropertyInfo> pinfo;
Copy link
Member

Choose a reason for hiding this comment

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

I actually like my code for handling requesting objects better. It's newer than yours, fixes a few bugs, sends Object IDs and can handle when an object is too big to send.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember you have changed the limit to 8MB it should be enough for most objects. I will check your code again and make it more reliable with this. But it is necessary to change something to send more usable informations like script constant values and member variables of script instance.

Copy link
Member

Choose a reason for hiding this comment

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

member variables of script instance should already be obtainable with get_property_list to the object, they have the usage flag: PROPERTY_USAGE_SCRIPT_VARIABLE , so no extra code is needed.

@@ -231,11 +252,11 @@ void ScriptDebuggerRemote::debug(ScriptLanguage *p_script, bool p_can_continue)
}
}

{ //locals
packet_peer_stream->put_var(locals.size());
{ //globals
Copy link
Member

Choose a reason for hiding this comment

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

The idea of sending globals is fine, but in GDScript, globals are constants and there are a ton of them. Sending them every time seems like too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Infact only autoloads will be sent in GDScript.

inspected_object->prop_list.clear();
inspected_object->prop_values.clear();
bool is_new_object = false;
if (remote_objects.has(id)) {
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I like this, makes the history work.

}

for (int i = 0; i < prop_count; i++) {
for (int i = 0; i < properties.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Please check my code for sending objects, it fixes a few bugs since 2.1 regarding objects being too big that may crash the debugger, so you may want to use it, or part of it.

@akien-mga
Copy link
Member

Sorry @Geequlim, I did a slight refactoring of the gdscript module in #12969 so you'll need to rebase this PR.

Ignore all script constants in the global section of the breakpoint stack.
Check property size before send to avoid too large of data be sent.
Fix crash while clear the remote objects from the debugger.
@Geequlim
Copy link
Contributor Author

Geequlim commented Nov 17, 2017

@akien-mga Rebased to latest master.

@reduz I try to use your code to send properties but I got some problems

So I keep the code of mine for send objects with your size checking logic for properties.

@reduz
Copy link
Member

reduz commented Nov 17, 2017

@Geequlim If you want to keep your code but use the size checking it's fine

@akien-mga akien-mga merged commit 6065b2d into godotengine:master Nov 20, 2017
@reduz
Copy link
Member

reduz commented Nov 21, 2017

@Geequlim I don't mind, but please make sure to use my code to check that no buffer overrides exist, otherwise the editor will crash.

@reduz
Copy link
Member

reduz commented Nov 21, 2017

@Geequlim also, cant seem to drag values in the inspector

@Geequlim
Copy link
Contributor Author

@reduz I have take the size checking code inside the PR before merged. But I’m not sure it is safe enough as the size checking for each property to the max size of the stream peer as it did before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants