-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add Expression Evaluator and Script Watches to the debugger #1
base: master
Are you sure you want to change the base?
Conversation
Thanks @rxlecky! I've been testing the commit and I found the following issues:
If there is anything I can do to help please let me know. |
Thanks @jahd2602 for reporting the problems you are having, this really helps! I opened an issue for each of them respectively in my repo. When you encounter problems just open new issues. It would be also useful to mention how severely given issue impacts you to help me prioritize my work. Also, I just want to ensure I understand the second issue from your list correctly. Is the problem that you can not select and copy from the log in the evaluator? If that's the case, as you mentioned, the workaround would currently be to use arrow up to find the correct expression and then press the |
@rxlecky I will open new issues as soon as I find them.
Exactly, I am aware of the workaround but still, I think new users will not get it immediately and might get frustrated.
Ok from the 4 items I mentioned, the order of importance would be 4 (most important), 1, 3 and 2 (as you mentioned, there is currently a workaround so we can leave it last). |
@rxlecky I just tested https://github.com/JavaryGames/godot/pull/1/files/3ac33d0ea0ca8fd8acf6bc9f707f41b7a0d368c8..b85c56ff791dd28f93262cd2769c5dce38fddf70 and indeed the crash does not seem to happen anymore! Also, I was very impressed that I could watch expressions like |
@rxlecky I also noticed something: When canceling the execution of the game, the watched expressions are still there, which is great. However, when playing the game again, they stop working. I guess because the instance ids are gone. Have you thought on anyway to persist the variable references even after re-launch? |
That's a great news! So we now know where the error was coming from. The reason why I didn't do those checks was that I was certain all those variables are non-null under normal circumstances. It would be really helpful if you could let me know which of the variables are reported to be null when the
Haha, really glad to hear that! 😄 Normally IDEs don't support live update of watches, presumably for performance reasons. And the performance was also Juan's concern but he agreed that it is a really useful feature to have so he gave it a greenlight. It would be especially useful if you could test whether you can observe FPS differences between when you are using watches versus when you are not using them. There's also an option in the Editor Settings
Yes, that's exactly the reason why they are not working. There are two reasons why they are not cleared:
That said, manually reconnecting watches is really, REALLY tedious. So the plan is to eventually implement a feature to reconnect watches automatically. This obviously isn't possible for all watches but covering the most common use case would be really helpful already. Most likely the automatic reconnecting would only cover the nodes that are placed in the scene statically, not the ones spawned on runtime.
Yes, I am aware that the error messages aren't clear at all. This is one of the UX improvements that I will work on at some point. It isn't really a priority at the moment, though. |
@rxlecky yes, with autoloads it would be very useful that it auto-connects automatically. I have another idea for the automatic reconnection: Keep track of the script and line-number that the watcher started tracking a variable. Then, in a following execution, when the parser reaches that script and line-number, then reconnect the variable. It should probably exist with the same name (if the user has not changed it in between executions). It would probably cause issues when multiple instances uses the same script (it would only connect with the first instance) but at least it would attend the majority of the cases automatically. |
694e346
to
9235ea1
Compare
9235ea1
to
abc2ea0
Compare
Evaluator allows expressions to be evaluated in the current stack frame. Watches keep track of a list of expressions.
Game occasionally crashes on nullptr dereference in _evaluate_watches. Add null checks to help discover the issue.
Evaluating in watches still crashes
abc2ea0
to
093a366
Compare
Move watches next to the stack frames and the code stepping buttons
Replaced evaluete_watches mutex with atomic counter to prevent infinite recursion that caused this crash. Also moved all locking from _evaluate_watches to the inlined evaluate_watches, making _evaluate_watches thread unsafe. Fix #12
Evaluator allows expressions to be evaluated in the current stack frame.
Watches keep track of a list of expressions.
The original PR: godotengine#26219
This is still a WIP. Testing and feedback is much appreciated.
CC @jahd2602