-
Notifications
You must be signed in to change notification settings - Fork 43
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
Redscript Compilation Error-checking #23
base: master
Are you sure you want to change the base?
Redscript Compilation Error-checking #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Again a few remarks.
src/dll/Hooks/ExecuteProcess.cpp
Outdated
switch (waitResult) { | ||
case WAIT_TIMEOUT: | ||
spdlog::error("Redscript compilation timed-out - exiting"); | ||
SHOW_LAST_ERROR_MESSAGE_AND_EXIT_FILE_LINE("Redscript compilation timed-out"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the debate I wanted to raise :) if we've added scripts, and it's the first try at compiling, I think we should. Otherwise, it could be a hot-reload, which should probably be handled differently/passed to another part of that plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it, @psiberx what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied the condition I described to the exit later on - we're still exiting for the cases here, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only exit on timeout, but not if compilation fails.
Exiting only on game start is compatible with hot reload indeed. Although redscript shows the message "the game will start without scripts" when compilation fails, which can be a bit confusing.
Prints the status of the redscript compilation & exits if the command fails (assume game will not run if our redscript files were not compiled) - we might want to also show an error, but redscript will have already shown one. Also handles the error results for
WaitForSingleObject
, but I was not able to trigger these conditions to test.Based on #22