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

[WIP] Expression evaluator and variable watches #26219

Open
wants to merge 1 commit into
base: master
from

Conversation

@rxlecky
Copy link
Contributor

rxlecky commented Feb 24, 2019

When debugger breaks execution, expressions can be evaluated in the current stack frame.

Closes #7057.

@rxlecky rxlecky requested review from bojidar-bg, reduz and vnen as code owners Feb 24, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Feb 24, 2019

20190224_034627

@Chaosus Chaosus added this to the 3.2 milestone Feb 24, 2019
@rxlecky rxlecky force-pushed the rxlecky:expression-evaluation branch 2 times, most recently from ff16365 to 5452a4d Feb 24, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Feb 24, 2019

Not quite sure what is the reason of the failing build...

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 25, 2019

Not quite sure what is the reason of the failing build...

A style issue as shown by this diff: https://travis-ci.org/godotengine/godot/jobs/497835792
Our clang-format configuration disallows having more than one consecutive newline. See Style guide.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Feb 25, 2019

Oh, I see! I knew it was coming from clang-format, just wasn't sure what the problem was. Thanks!

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Feb 25, 2019

Wait, this is strange. This is the reported error from clang-format:

*** The following differences were found between the code to commit and the clang-format rules:
--- a/core/script_debugger_remote.cpp	2019-02-24 18:09:18.317826952 +0000
+++ b/core/script_debugger_remote.cpp	2019-02-24 18:09:27.113682041 +0000
@@ -1067,7 +1067,6 @@
 	physics_frame_time = p_physics_frame_time;
 }
 
-
 ScriptDebuggerRemote::ResourceUsageFunc ScriptDebuggerRemote::resource_usage_func = NULL;
 
 ScriptDebuggerRemote::ScriptDebuggerRemote() :
*** Aborting, please fix your commit(s) with 'git commit --amend' or 'git rebase -i <hash>'

However, it doesn't seem to match the source. Not to mention that there is not double newline:

void ScriptDebuggerRemote::profiling_set_frame_times(float p_frame_time, float p_idle_time, float p_physics_time, float p_physics_frame_time) {
frame_time = p_frame_time;
idle_time = p_idle_time;
physics_time = p_physics_time;
physics_frame_time = p_physics_frame_time;
}
void ScriptDebuggerRemote::set_allow_focus_steal_pid(OS::ProcessID p_pid) {
allow_focus_steal_pid = p_pid;
}
ScriptDebuggerRemote::ResourceUsageFunc ScriptDebuggerRemote::resource_usage_func = NULL;
ScriptDebuggerRemote::ScriptDebuggerRemote() :

It seems as if clang-format couldn't see the whole set_allow_focus_steal_pid function definition.

Also, I did not touch this section of the code at all, so it's weird this error popped up now, and not when the code was created.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 25, 2019

It seems as if clang-format couldn't see the whole set_allow_focus_steal_pid function definition.

Indeed, it's because it has been removed in the master branch in 9d78274, which is not in your own branch. Travis then applies your PR on top of the master branch, and creates this style issue when merging the changes together.

So you'd have to rebase on the master branch to fix it.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Feb 25, 2019

Of course, that makes sense!

Thanks for the explanation, will remember it not to ask the same question again.

@rxlecky rxlecky force-pushed the rxlecky:expression-evaluation branch from 5452a4d to b8854c0 Feb 25, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 25, 2019

Thanks for the explanation, will remember it not to ask the same question again.

Well I don't mind your asking, that was a tricky case :D It took me a while to understand why it was showing this style error myself.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Feb 25, 2019

True, it wasn't a trivial case. :) Well, good thing it builds now without errors.

@rxlecky rxlecky force-pushed the rxlecky:expression-evaluation branch 2 times, most recently from 2ddfafb to f3c560c Mar 3, 2019
@rxlecky rxlecky force-pushed the rxlecky:expression-evaluation branch from f3c560c to 8245460 Mar 22, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Mar 22, 2019

Updated the previous version: Added script watches allowing tracking results of multiple expressions. A watch can also be locked or unlocked. When locked, the variables in the expression are locked to reference the variables in that they reference at the moment of locking. If not locked, the watch is always evaluated in the context of the current stack frame.

One more feature that is planned, but currently disabled is tracking watches - making watch tracking makes it break execution when the result of the watch changes. However, this requires small change in how local variables are queried and that's something I'd first have to discuss with the author of the code.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Mar 22, 2019

@bojidar-bg @vnen I was talking to Juan a while back and he told me either of you guys would hopefully find time to skim over my code and give me some feedback.

@rxlecky rxlecky force-pushed the rxlecky:expression-evaluation branch from 8245460 to 336f6e3 Mar 22, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Apr 1, 2019

Hey @vnen I was looking into GDScript debug_get_stack_level_locals function that gives me some trouble. It returns all local variables in the function that the given stack frame is in - including variables that were not yet declared or initialized. For those, it returns Variant::NULL as their value. I believe the correct behaviour would be not to return the variables that do not exist yet.

Looking into the function that this function relies on, I found this code:

int oc = 0;
Map<StringName, _GDFKC> sdmap;
for (const List<StackDebug>::Element *E = stack_debug.front(); E; E = E->next()) {
const StackDebug &sd = E->get();
if (sd.line > p_line)
break;

Was the line if (sd.line > p_line) break; supposed to skip the variables that are not yet declared and is just failing somewhere or am I misunderstanding the code? Thanks for the help!

return watches[p_index].expression->get_error_text();
}

bool ScriptDebugger::evaluate_watches(int p_stack_level, int p_watch) {

This comment has been minimized.

Copy link
@reduz

reduz Apr 1, 2019

Member

Some feedback:

  1. This function will be called for every line of script code being run, and it will make execution considerably slower so make sure nothing is run if no watches exist by using some sort of inline static bool to check watches.
  2. This function can be called from multiple threads, make sure to add a mutex to it if it actually runs
  3. You will need to check if waches use stack variables from the current function, if so they will need to be erased once function exits.
@@ -36,6 +36,12 @@
class Expression : public Reference {
GDCLASS(Expression, Reference)
public:
struct Context {

This comment has been minimized.

Copy link
@reduz

reduz Apr 1, 2019

Member

This is added here and then unused, it should probably be added somewhere else.
Also, dont store a pointer, you should instead store an ObjectID via object->get_instance_id() and then resolve to pointer using ObjectDB::get_instance(). This way if the object was erased, it will fail and you can remove the watch.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Apr 30, 2019

@SeleckyErik Do you know how to edit the PR to take @reduz's feedback into account, or do you need more details?

@Reneator

This comment has been minimized.

Copy link

Reneator commented May 22, 2019

Really looking forward to this! hypetrain

@akien-mga akien-mga requested a review from reduz May 28, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented May 28, 2019

Needs a rebase to solve a merge conflict, otherwise it should be good to review in a PR meeting.

@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented May 28, 2019

Similar to other places where source code is written, it would make sense to use the monospace/fixed-width font for the Expression output and LineEdit 🙂

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented May 28, 2019

@akien-mga When's the PR meeting taking place? There are a still some changes I want to make before this is merged.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented May 28, 2019

@Calinou Thanks, I will change that. :)

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jun 20, 2019

@SeleckyErik What's the status on this PR? Are there still changes you want to make before it's merged?

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Jun 20, 2019

@akien-mga I'm sorry, I was only in touch with Juan and didn't update you. Currently I was working on finishing #27742. I will push the final iteration on it this week. This PR will have to wait, though, because next week I have final school week and I have to focus on finishing up all formalities. I will resume working on this PR in July. If that means that it won't make it into 3.2, I am sorry for the inconvenience, but I am pressed by deadlines right now.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jun 20, 2019

No worry, take your time :)

There's no planned date for 3.2's feature freeze for now, but it will likely be towards the end of July, so you should have time to finish this one.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Jun 20, 2019

Nice, that should be enough time indeed. I will keep you updated in case of any changes.

@rxlecky rxlecky force-pushed the rxlecky:expression-evaluation branch 2 times, most recently from f193f21 to 58af21e Aug 14, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Aug 14, 2019

Worked on the Evaluator part and addressed mainly UI/UX-related feedback:

  • Updated font to monospace (thanks for a reminder @Calinou 🙂)
  • Enabled scroll following on the expression log RichTextLabel
  • Added colors to distinguish normal results from errors
  • Implemented expression history, allowing access to previously evaluated expressions
  • Added a way to clear expression log (UI is WIP)

2019-08-15_00-35-04
GIF showing UI/UXchanges

Also, evaluator can now be used regardless of whether debugger is running or not or whether a breakpoint was hit or not; It can evaluate GDScript public functions and constants without a debugger context. So besides debugging, it can also double for a pocket calculator. 🙂

@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Aug 14, 2019

@rxlecky Judging by the GIF, the expression log and its input field still seem to use the default font rather than the code font. Or is the GIF outdated?

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Aug 15, 2019

@Calinou No, the GIF shows the current. Honestly, that's why I tagged you, because I find it hard to spot the differences between the fonts so I was hoping you'd give me a hand. Is this the correct way to set the code font?

Ref<Font> mono_font = get_font("source", "EditorFonts");
set_h_size_flags(SIZE_EXPAND_FILL);
line = memnew(LineEdit);
add_child(line);
line->set_anchors_and_margins_preset(PRESET_WIDE);
line->set_context_menu_enabled(false);
line->connect("text_entered", this, "_text_entered");
line->add_font_override("font", mono_font);

Also, I did a small comparison, I put two LineEdits next to each other. The left one is the current LineEdit from this PR and the right one is a default LineEdit. There seems to be a small difference but still I am not sure whether the left one uses the correct code font.

obrázok

@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Aug 15, 2019

@rxlecky I got the font setting to work as expected 🙂

diff --git a/editor/expression_evaluator.cpp b/editor/expression_evaluator.cpp
index 16df7d7b4..df63360cc 100644
--- a/editor/expression_evaluator.cpp
+++ b/editor/expression_evaluator.cpp
@@ -33,6 +33,7 @@
 #include "core/math/expression.h"
 #include "core/os/input.h"
 #include "core/os/keyboard.h"
+#include "editor_node.h"
 #include "editor/editor_settings.h"
 #include "scene/gui/button.h"
 #include "scene/gui/control.h"
@@ -126,7 +127,7 @@ HistoryLineEdit::HistoryLineEdit() {
        history.index = 0;
        set_process_input(true);
 
-       Ref<Font> mono_font = get_font("source", "EditorFonts");
+       Ref<Font> mono_font = EditorNode::get_singleton()->get_gui_base()->get_font("source", "EditorFonts");
 
        set_h_size_flags(SIZE_EXPAND_FILL);
 
@@ -238,7 +239,7 @@ void ExpressionEvaluator::set_result(const String &p_result, bool p_error) {
 ExpressionEvaluator::ExpressionEvaluator() {
        expression = memnew(Expression);
 
-       Ref<Font> mono_font = get_font("source", "EditorFonts");
+       Ref<Font> mono_font = EditorNode::get_singleton()->get_gui_base()->get_font("source", "EditorFonts");
 
        set_name("Expressions");
        set_focus_mode(FOCUS_NONE);

I don't know why it solves the issue, but I noticed the same thing is done in FindInFilesPanel:

_search_text_label->add_font_override("font", EditorNode::get_singleton()->get_gui_base()->get_font("source", "EditorFonts"));

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Aug 15, 2019

Ahhh, I see, thanks for the help! I'll fix it. :)

Evaluator allows  expressions to be evaluated in the current stack frame. Watches keep track of multiple
expressions.
@rxlecky rxlecky force-pushed the rxlecky:expression-evaluation branch from 58af21e to 3fb350b Aug 16, 2019
@akien-mga akien-mga dismissed reduz’s stale review Aug 28, 2019

Changes done.

@akien-mga akien-mga removed the request for review from karroffel Aug 28, 2019
@akien-mga akien-mga mentioned this pull request Aug 28, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Aug 28, 2019

Waiting for #27742 to be merged as it will conflict with this PR, which should then be rebased (debugger moving from core to scene).

@akien-mga akien-mga changed the title Added Expression tab in the debugger [WIP] Added Expression tab in the debugger Aug 30, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Oct 4, 2019

Moving to 4.0 milestone as it still needs some work so it won't be ready to merge for the imminent 3.2 release.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Oct 4, 2019
@rxlecky rxlecky changed the title [WIP] Added Expression tab in the debugger [WIP] Expression evaluator and variable watches Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.