-
Notifications
You must be signed in to change notification settings - Fork 49
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
Make debug behaviour consistent for all tools #512
Comments
Consistency sounds good! Perfect.
I wouldn't bother actually, but would instead mention it somewhere as a recommendation for manual debugging in case of crashes in e.g. blender. I am a bit wary as including it anywhere by default, even for debug mode. However, I am not fully against it so will leave this up the general consensus of everyone. Just giving my gut feeling. |
My gut feeling actually says the same. I also updated this point, to make it clear what I mean. The thing is that some tools already have this and some don't. It's not consistent. |
@mottosso Do you have an opinion about this? Especially about the excepthook? Should we add it to all tools or remove it from all tools? I am inclined to do the latter. |
Oh, no this shouldn't be per tool. It should be per install of Avalon. There's an install per integration, like the one we have for Maya. That'll happen once, and can take things like this into consideration. Either as an argument, or environment variable. In addition, we shouldn't overwrite it point-blank, we should try and preserve whatever was there initially. At least then we prevent half of the problems from occurring (i.e. not accounting for someone overwriting it after Avalon). previous_excepthook = sys.excepthook
def avalon_excepthook():
previous_excepthook()
# Our business here
sys.excepthook = avalon_excepthook We should also make sure to uninstall it in the This won't preserve anyone overwriting it after Avalon though, so it isn't entirely safe.. It's unfortunate the excepthook doesn't facilitate more folks working with it. Especially since it seems the only way to keep Qt from killing Blender. We certainly don't want Avalon to be a reason for Blender dying, and it's not reasonable to expect Avalon will never throw an exception, even though that would be the ideal case. I think the reasonable thing to do, despite having said the opposite not too long ago, is to handle it per default. Don't let Avalon be the cause of people losing work in Blender. But make it very clear that it's doing it. One approach I somewhat liked was how aTools did it. They prompt the user on the first run about when it's doing things that may affect other functionality in Maya. Just a popup that you can't miss, and need to agree to (or not, instead affecting the stability of aTools). The problem though is that the users of Avalon aren't the ones to make this choice, it's the TDs setting it up. For TDs, the documentation is the popup. So my two cents.
At the end of the day, causing hard-to-debug issues with overwriting excepthook is bad, but better than insta-killing Blender. |
Thanks @mottosso. So as part of the linked PR (#513) I will remove the excepthook code from all the tools that currently have this. Later I will do a separate PR, to make it simple to override the excepthook (so future integrations that don't have Qt) can make use of it easily. Or make it part of the Blender PR (#466). |
When doing #470 I missed some similar calls in the other tools. I would like to make it consistent across all the tools and avoid confusing behaviour.
Things I would like to do:
debug
variable to theshow()
function of all the tools.debug=True
. That way hosts who don't rely on Qt themselves don't crash when there is an error in one of the tools. As mentioned here this might actually be quite dangerous. So the other option would be to remove this from all tools. The most important thing is to make it consistent (meaning include this for every tool or for none of them, now some have it and some don't).The text was updated successfully, but these errors were encountered: