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

Expression Evaluator (REPL support with inspector sequent results) #60134

Closed
wants to merge 0 commits into from

Conversation

rohanrhu
Copy link
Contributor

@rohanrhu rohanrhu commented Apr 11, 2022

Meoowww... I made an expression evaluator.

Looks like this:
image

I overloaded

void GDScriptLanguage::debug_get_stack_level_locals(int p_level, List<String> *p_locals, List<Variant> *p_values, int p_max_subitems, int p_max_depth)

definitions with

void GDScriptLanguage::debug_get_stack_level_locals(int p_level, Vector<String> *p_locals, Array *p_values, int p_max_subitems, int p_max_depth)

because Expression methods accepts Vector<String> and Array soo I didn't copy List<String> and List<Variant> datas into Vector<String> and Array.

I connected EditorDebuggerInspector's object_selected to inspect objects from evaluater result to inspector bar.

Fixes godotengine/godot-proposals#4473

@Calinou
Copy link
Member

Calinou commented Apr 11, 2022

Thanks for opening a pull request!

Please amend the commit to use a human-readable commit message (git commit --amend). This way, people reading the Git log can easily figure out what's going on 🙂

PS: It's "evaluator", not "evaluater".

@rohanrhu
Copy link
Contributor Author

PS: It's "evaluator", not "evaluater".

Hiiii @Calinou I refactored the typo.

@Chaosus
Copy link
Member

Chaosus commented Apr 12, 2022

Please squash commits together as required by our pipeline (https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html).

@rohanrhu rohanrhu force-pushed the expression-evaluater branch from addf7ec to 964c281 Compare April 12, 2022 13:06
@rohanrhu
Copy link
Contributor Author

Hiiii @Chaosus I squashed commits.

@rohanrhu rohanrhu requested a review from a team as a code owner April 13, 2022 11:09
@rohanrhu rohanrhu force-pushed the expression-evaluater branch from 967aaf3 to 770c22f Compare April 13, 2022 11:17
@rohanrhu
Copy link
Contributor Author

Hiii, I added missing overloading of virtual method for C# bindings, trimmed trailing spaces and squashed commits.

@rohanrhu rohanrhu force-pushed the expression-evaluater branch from 8f40fa2 to a96a8f4 Compare April 13, 2022 13:29
@rohanrhu
Copy link
Contributor Author

I ordered includes for format checker and squashed commits again. 🙀

@rohanrhu
Copy link
Contributor Author

Meow.. @Chaosus @Calinou clang-format is acting interesting. I ran clang_format.sh in my local and it ordered the includes but it still wants a different order in Github workflow. 🙀

@neikeq
Copy link
Contributor

neikeq commented Apr 13, 2022

@rohanrhu Make sure your clang-format version matches the one we're using. As of this comment, that would be version 13:

sudo apt-get install -qq dos2unix recode clang-format-13 libxml2-utils

@rohanrhu rohanrhu force-pushed the expression-evaluater branch from d7ac6eb to a30a617 Compare April 13, 2022 18:39
@fire
Copy link
Member

fire commented Apr 13, 2022

Can someone help @rohanrhu write a proposal for this so we can all meow.

@rohanrhu
Copy link
Contributor Author

Can someone help @rohanrhu write a proposal for this so we can all meow.

Yesss I want to meoowww.

@rohanrhu
Copy link
Contributor Author

I meowed a proposal:
godotengine/godot-proposals#4473

@rohanrhu
Copy link
Contributor Author

rohanrhu commented May 4, 2022

Meoowwww can anyone review this PR?? 🙀

@Faless
Copy link
Collaborator

Faless commented Feb 24, 2024

I think your commit ed617c96eefd2b057bdcd9e57407611cc9c7702d is a partial change I was discussing and that I incorrectly marked as suggestion in GH, sorry about that.

You can probably revert that completely, since it's not really blocking the PR.

@rohanrhu rohanrhu force-pushed the expression-evaluater branch from a971a44 to 7645c17 Compare February 24, 2024 13:57
@KoBeWi
Copy link
Member

KoBeWi commented Feb 25, 2024

The LineEdit already has a clear button, so the extra Clear is unnecessary.
image

EDIT:
Although maybe it makes sense if LineEdit can be disabled? 🤔 idk

@KoBeWi
Copy link
Member

KoBeWi commented Feb 25, 2024

The variable overwrite bug is still not fixed.

@rohanrhu
Copy link
Contributor Author

The variable overwrite bug is still not fixed.

Oh I think you didn't see my previous message; that bug is coming from the internal behavior of the inspector component. I think that feature which is for updating the variables when you switch to another stack, is implemented as the only behavior in the inspector. If you want I can fix that too by implementing it outside the stack variables component and update them from the inspector part in this PR or a new PR.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 25, 2024

Well unless it's a pre-existing bug, it's better to fix it here.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Seems like all my previous suggestion were "resolved" but none applied.

@rohanrhu
Copy link
Contributor Author

Seems like all my previous suggestion were "resolved" but none applied.

Oh I remember I did them but there is only one I asked about this:

I think it is good because there are other things too that the evaluator checks with the ScriptEditorDebugger instance https://github.com/rohanrhu/godot/blob/a971a441dc3e0c5f08a3774e3048fda550ed54bd/editor/debugger/editor_expression_evaluator.cpp#L56

I can change all of those things and make it work with signals but I think this way is more efficient in terms of optimization.

What do you want me to do?

@Faless
Copy link
Collaborator

Faless commented Feb 29, 2024

What do you want me to do?

I've gone through them, and "unresolved" the ones that still need to be applied.

@Faless
Copy link
Collaborator

Faless commented Feb 29, 2024

I think it is good because there are other things too that the evaluator checks with the ScriptEditorDebugger instance https://github.com/rohanrhu/godot/blob/a971a441dc3e0c5f08a3774e3048fda550ed54bd/editor/debugger/editor_expression_evaluator.cpp#L56

Note that the check is_session_active() is not needed there, because send_message does that for you.

@mathrick
Copy link

Hi, what's the status of this PR? Not having any ability to inspect complex expressions in the debugger really hobbles it, so I'd really like to see this merged.

@AThousandShips
Copy link
Member

@mathrick see the comments right above yours, this needs further work by the OP

@dalexeev dalexeev modified the milestones: 4.3, 4.x May 31, 2024
@rohanrhu
Copy link
Contributor Author

The LineEdit already has a clear button, so the extra Clear is unnecessary. image

EDIT: Although maybe it makes sense if LineEdit can be disabled? 🤔 idk

Hello I'm taking care of the PR. The "Clear" button is for clearing evaluator results, not for the input so I think it is good to have 😊

@jdozubko
Copy link

How close are we to adding this feature? It seems really close and I'd love to see it in. It's an essential debugging feature IMO

@KoBeWi
Copy link
Member

KoBeWi commented Sep 30, 2024

Someone more active would need to take over to finish this. There is still some unaddressed feedback and merge conflicts.

@jdozubko
Copy link

I really wish I knew how as I'd happily push this over the finish line, but I'm out of my depth for this level of coding. The feature seems so close. Anybody out there able to help?

@RyanCross
Copy link

RyanCross commented Oct 1, 2024

I've followed this one for a couple years now. There's about 100 comments in here and the complexity of picking up and sorting through this mess may be higher than someone starting from scratch. I personally think it would be best that this be closed so would be contributors aren't under the impression this is in progress or claimed.

EDIT: when I say mess I mean the issue itself, I can't speak to the quality of the code.

@fire
Copy link
Member

fire commented Oct 1, 2024

#97647 is a recreation of this pull request.

Typically a new pr supersedes the older one. The older one then is archived.

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.

Add an expression evaluator (REPL support with inspector sequent results)