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

Implement conditional, printing, and enabled/disabled breakpoints #100516

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ydeltastar
Copy link
Contributor

@ydeltastar ydeltastar commented Dec 17, 2024

Adds advanced breakpoints and related editor features to work with them.

image

Features

  • Conditional breakpoints using Expression evaluation.
  • Print breakpoints using string interpolation. Supports member and stack variables Node {name} : {event}.
  • Can disable suspending execution. Useful for print-only breakpoints.
  • Enable/disable or edit through the code editor or the breakpoint tree's context menu.
  • Support for passing settings by command line. -b res://script.gd:55|print=Print:%20{event}

I didn't make relevant changes in this PR for the DAP support which is used by the godot-vscode-plugin but the groundwork is done should someone want to implement it.

@ydeltastar ydeltastar requested review from a team as code owners December 17, 2024 16:51
@AThousandShips AThousandShips added this to the 4.x milestone Dec 17, 2024
@ydeltastar ydeltastar force-pushed the adv-break branch 4 times, most recently from 36c6ad5 to 4d08122 Compare December 24, 2024 13:45
@ydeltastar ydeltastar requested review from a team as code owners December 24, 2024 13:45
@ydeltastar ydeltastar force-pushed the adv-break branch 6 times, most recently from 9f7db04 to 95a220a Compare December 24, 2024 19:04
@ckaiser
Copy link
Contributor

ckaiser commented Jan 13, 2025

Tested, works as expected.

The conditions are pretty powerful, I'm a fan. It does let you shoot yourself in the foot by calling functions with side effects, but I'm not opposed to that being a thing that's allowed.

One thing I could see being improved is the text for the "Print" placeholder, I had to resize the window to finish reading it and find out it's with {var_name}, it's a minor thing but the text could be condensed or the sizing adjusted.

Checking the validity of the expression within the dialog, if possible, would be great as well, so you can see if you've made a syntax error (I accidentally tried to use || instead of or for example).

Overall though this is great, very useful and I'd love to see it get merged. Did not look at the code or documentation, just functionality.

@ydeltastar ydeltastar force-pushed the adv-break branch 2 times, most recently from b96ed57 to 1f4a516 Compare January 23, 2025 16:54
@ydeltastar
Copy link
Contributor Author

I modified the print hint text.

Checking the validity of the expression within the dialog, if possible, would be great as well, so you can see if you've made a syntax error (I accidentally tried to use || instead of or for example).

|| is a valid operator in GDScript.
I added an expression parsing error label but note that it can only catch basic syntax errors.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

The idea of having non-suspending breakpoints is a great one; finally I may no longer have to rely on print() statement debugging so much 🙂

image

Some feedback:

  • Using an unknown variable name (e.g. counters >= 10 instead of counter >= 10 in the above example) will fail silently. This should probably print an error when the line is first reached (but without breaking).
  • Using an unknown print name (e.g. Value of counter: {counters} instead of Value of counter: {counter}) won't replace the placeholder and no error is printed. I'd expect it to print an error (and leave the placeholder as-is).
  • Prints could make use of print_line_rich() instead of print_line() so you can use BBCode formatting within them. Right now, BBCode is printed as-is ([color=red]{counter}[/color] won't show up in red).
  • When a breakpoint has Suspend Execution disabled, it should have a different icon to distinguish it from breakpoints that don't suspend execution. This could be an outlined red circle, for instance. For conditional non-suspending breakpoints, the = sign could be present in red in the middle.
  • Since we have some space, we could rename Suspend Execution to Suspend Execution if Condition is Met just to be clear.
  • Changes are occasionally lost after I close and reopen the Edit Breakpoint dialog. It's inconsistent and I can't determine a pattern as to why it's occurring.
  • The error message that occurs when you use an invalid expression (I used : for my testing) isn't very clear. While the editor does point back to the script line where it occurred, it doesn't tell you that it's coming from an invalid conditional breakpoint. Ideally, it should do that, but also print the expression that was attempted to be parsed. (Printing the expression could also be done at the Expression class level.)

image

(I accidentally tried to use || instead of or for example).

It's strange that this is invalid in Expression, considering it's valid in GDScript (even if the preferred syntax is or as per the style guide).

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 conditional/printing breakpoints
4 participants