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

Fix: [NoAI] don't notify caught exceptions #8331

Merged
merged 1 commit into from Dec 2, 2020
Merged

Fix: [NoAI] don't notify caught exceptions #8331

merged 1 commit into from Dec 2, 2020

Conversation

rptr
Copy link
Contributor

@rptr rptr commented Oct 18, 2020

Make _notifyallexceptions and error raising behave as expected
(1 LOC backport from https://github.com/albertodemichelis/squirrel trunk)
Set _notifyallexceptions to false to only call Squirrel::_RunError on uncaught exceptions

@rptr rptr closed this Oct 18, 2020
@rptr rptr changed the title Fix: don't notify caught exceptions Fix: [NoAI] don't notify caught exceptions Oct 18, 2020
@rptr rptr reopened this Oct 18, 2020
@LordAro
Copy link
Member

@LordAro LordAro commented Oct 18, 2020

Presumably this does still output for uncaught exceptions?
For a possible bonus feature - still output all exceptions if in running with -d script=<n>

@rptr
Copy link
Contributor Author

@rptr rptr commented Oct 18, 2020

https://i.imgur.com/dHSqlon.png

AI code used:
function ThrowAI::Start () { AILog.Info("AI booting up..."); AILog.Info("\"throw 1\""); try { throw 1; } catch (e) { AILog.Info("Caught it"); } AILog.Info("\"throw 1\""); throw 1;

@rptr
Copy link
Contributor Author

@rptr rptr commented Oct 18, 2020

The -d script= possibility occured to me, but ideally it wouldn't show on the lowest level, since it's used by AI developers. At least I use that option. Not sure exactly what is printed for each level.

I guess an alternative solution is/ was to catch all exceptions like before but filter out the call stacks somewhere in RunError/ ScriptController::Print. If the AI/Game Script Debug window was made more interactive at some point, this information could be folded/ hidden and interacted with.

@rptr
Copy link
Contributor Author

@rptr rptr commented Oct 18, 2020

Also the reason I suggested AIError:PrintCallStack was to allow developers to still print the stack for debugging.

@LordAro
Copy link
Member

@LordAro LordAro commented Oct 18, 2020

I wouldn't expect it to be used by AI developers, tbh. It's all mostly just for internal debugging stuff. But you can just arbitrarily pick a level to display it at - there's no consistency between anything :p

@rptr
Copy link
Contributor Author

@rptr rptr commented Oct 18, 2020

It's useful. I am coding an AI now (hence this patch), and it would be useful. Perhaps what I really want is a debugger...

@rptr
Copy link
Contributor Author

@rptr rptr commented Oct 18, 2020

Ah, but it still prints the stack in the case of errors. Maybe it wouldn't be needed then. Disregard what I just said.

@rptr
Copy link
Contributor Author

@rptr rptr commented Oct 18, 2020

So apparently that 1 line of code in sqvm.cpp wasn't needed after all.

SQBool notify_all = SQFalse;
if (_debug_script_level > 2) notify_all = SQTrue;
sq_notifyallexceptions(this->vm, notify_all);
Copy link
Member

@LordAro LordAro Oct 18, 2020

Choose a reason for hiding this comment

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

some odd indentation going on here. Besides, perhaps:

Suggested change
SQBool notify_all = SQFalse;
if (_debug_script_level > 2) notify_all = SQTrue;
sq_notifyallexceptions(this->vm, notify_all);
sq_notifyallexceptions(this->vm, _debug_script_level > 2);

?

@LordAro LordAro merged commit 0b2dd2c into OpenTTD:master Dec 2, 2020
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants