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

[2.1] Some improvements for debugger #10891

Merged
merged 3 commits into from
Sep 12, 2017
Merged

[2.1] Some improvements for debugger #10891

merged 3 commits into from
Sep 12, 2017

Conversation

Geequlim
Copy link
Contributor

@Geequlim Geequlim commented Sep 2, 2017

  • Fix continous update of the remote properties
  • Remove useless debugger property panel
  • Rename Remote Inspector to Live Scene Tree
  • Editor crashes when edit remote object instances from inspector edit history after debugging finshed.
  • Fix the continous update of the remote properties push tons of objects into edit history.
    Improve data serialization/desrialization with the debug remote and client.

Remove useless debugger property panel
Rename Remote Inspector to Live Scene Tree
@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

peek 2017-09-02 22-26

@Zireael07
Copy link
Contributor

Zireael07 commented Sep 2, 2017

@Geequlim In your gif, the editor reacts to you clicking on the nodes instantly. However, in my projects (other than the main scene which is tiny, similar to your gif, 4 nodes total) I need to wait a couple of seconds (with CPU usage noticeably increasing) before I see the properties in the window.

I suspect some serialization is done on demand instead of earlier? That would explain the CPU usage increase.

My project is 3d and consists of quite a lot of nodes (one main scene, which has 10 or 15 children, most of which are scenes themselves, and sometimes contain several child scenes too).

Also going back to the game results in CPU usage rampup and several seconds of waiting, during which Windows behaves as though the game was hung up.

EDIT: Monitor says its circa 3000 nodes.

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

DO NOT MERGE YET !

Two new problems found here:
1. Editor crashes when edit remote object instances from inspector edit history after debugging finshed.
2. The continous update of the remote properties push a ton of objects into edit history.

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

@Zireael07 Did you copmpiled with commit of this PR?

@Zireael07
Copy link
Contributor

Zireael07 commented Sep 2, 2017

Nope, I'm using 2.1.4. Why do you ask? Did you make some improvements on performance?

Also I've removed a scene I don't really need (which was made of a ton of modular meshes) and now with ~2900 nodes the delay is nearly gone. So it's definitely related to number of nodes (not scripts, the scene I removed had no scripts, just a ton of meshes).

Is the debugger fetching the data for everything or some such? I am scratching my head here wondering why the additional 200 mesh instances should affect debugger so much.

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

@Zireael07
No improvements with data serialization. But the continous update with this pr will make that worse as it send serialized data from remote debugger more frequently.

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

@Zireael07 I didn't try any thing with 3D with godot yet. Could you please send me an example project to reproduct that?

@Zireael07
Copy link
Contributor

Zireael07 commented Sep 2, 2017

Dropbox'd my whole project so that you can take a look: https://www.dropbox.com/s/8bn3lxhtm8latrx/FreeRoamRacer.zip?dl=0

This is a working copy of my project (what I have on github is missing some functionality), so I often plop stuff straight into the navigation.tscn for testing purposes. You'll notice there's quite a lot of free space around, so a more finished version of the game could easily have double the number of nodes.

If you instance the bridge scene, you'll probably see much worse debugger performance (as I said, 200 mesh instances).

EDIT: Also I can't seem to go back to game after looking in the debugger, it freezes. Sometimes it unfreezes after a lengthy delay, but more commonly it doesn't, making live edits pointless.

@Zireael07
Copy link
Contributor

PS. Anyway my comments aren't meant to hold up this PR, I just thought it'd be good to get the slowdown/inability to go back to game looked at.

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

@Zireael07
I run the res://objects/bridge.tscn and I can't reproduct it on my laptop.

@Zireael07
Copy link
Contributor

I didn't say 'run the bridge scene', I said 'instance the bridge scene'. Try running the project, and then instancing (chain icon over scene tree panel) the bridge scene in navigation.tscn (i.e. the main scene of the project), and running again.

@reduz
Copy link
Member

reduz commented Sep 2, 2017

Hmm, so basically what you suggest is using the regular property editor for everything? Does it still work for modifying the running game? I'm not against it, but we could probably move the Live Scene Tree to the regular scene tree and add some sort of "Live" option (that is on by default when you run the game) to switch between the controls so you can switch between them. Also it could be interesting to mark the inspector so it's clear that it's editing something remote. I have to add a small panel already to inspector to warn you when you are editing a resource from another scene (so you know you can't edit it), so we might reuse it for this.. Give me some time to make a proposal (or feel free to make one yourself too)

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

@reduz
It work for modifying the running game
peek 2017-09-02 23-48

Yes my suggestion is using the inspector to edit properties for both local instances and remote objects. And mege the live scene tree and the regular tree editor is also a good advice.

I'm glad to see the proposal you are making.

@reduz
Copy link
Member

reduz commented Sep 2, 2017

@Geequlim My idea for 3.1 (or 3.0 if you have time) could be something more akin to this:
image

Which of course needs a nicer design.. but the idea is that the remote scene appears automatically in the inspector when debug begins (and you can switch between local and remote). I think a lot of people ignores it right now because it can be kind of a hassle.

@reduz
Copy link
Member

reduz commented Sep 2, 2017

And the remote/local tabs or buttons of the scene tree disappear when not debugging.

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 2, 2017

That is more similar with what Unity does. 😄 But I think it is the right way.

@toger5
Copy link
Contributor

toger5 commented Sep 2, 2017

I really like the idea!!
making both toggle options be more consistent (instead of tab and toggle)
so each dock: inspector + scene_tree would change to the remove version as soon as the project is running. Than the user can press disconnect/(show original) to see the original tree (color would chage). pressing again shows the remote version. selecting something in the remote tree toggles the inspector to connected. and so on...

It's exactly the same than reduz proposes except using toggle buttons for both ui elements

@Geequlim
Copy link
Contributor Author

Geequlim commented Sep 3, 2017

@Zireael07 I can reproduct your problem. And it may be fixed by anothor PR in the future.
@akien-mga Time to have a review :)

@Zireael07
Copy link
Contributor

@Geequlim: Good to know it's reproducible, I was starting to think I was doing something wrong in the project :)

if (inspect_scene_tree->is_visible())
_scene_tree_request();

bool need_query_instance = inspected_object_id != 0;
Copy link
Member

Choose a reason for hiding this comment

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

I guess you could simplify this a bit as:

bool need_query_instance =
		inspected_object_id != 0
		&& editor->get_property_editor()->is_visible()
		&& editor->get_property_editor()->obj != NULL
		&& editor->get_property_editor()->obj->is_type("ScriptEditorDebuggerInspectedObject")
		&& (ObjectID)editor->get_property_editor()->obj->call("get_remote_object_id") != 0;

Or whatever format clang-format will allow for line breaks, it's a bit touchy with those. Worst case, everything on one line is still more readable than 5 successive assignments IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with clang-format :)

…history

Fix error inspector capitalization with regular objects after insepct remote objects
@Geequlim
Copy link
Contributor Author

@Dillybob92 Do you mean the 3.0 version?

@Geequlim
Copy link
Contributor Author

@Dillybob92 I have pull the request #13181 for that just now :)

@Geequlim Geequlim deleted the pr-debugger-2.1.4 branch January 11, 2018 03:03
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.

5 participants