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

Make debug behaviour consistent for all tools #512

Open
jasperges opened this issue Jan 16, 2020 · 5 comments
Open

Make debug behaviour consistent for all tools #512

jasperges opened this issue Jan 16, 2020 · 5 comments

Comments

@jasperges
Copy link
Contributor

jasperges commented Jan 16, 2020

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:

  • Add the debug variable to the show() function of all the tools.
  • Remove anything that changes the context/session, for example the current asset, project, etc. That can be used when running tests, but should never happen when debugging. It actually makes debugging a lot harder!
  • Include the following (see snippet below) when 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).

if debug:
    import traceback
    sys.excepthook = lambda typ, val, tb: traceback.print_last()
@BigRoy
Copy link
Collaborator

BigRoy commented Jan 16, 2020

Consistency sounds good! Perfect.

sys.excepthook = lambda typ, val, tb: traceback.print_last()

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.

@jasperges
Copy link
Contributor Author

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.

@jasperges
Copy link
Contributor Author

@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.

@mottosso
Copy link
Contributor

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 uninstall() version of install(). Which would be resetting it to whatever the previous value was.

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.

  1. Overwrite it per default
  2. In the install() of the Blender integration (not per tool, unless I'm missing some reason for that being done multiple times, once for each tool?)
  3. But preserve whatever handler was there (even if there is none currently, one may be added at any time)
  4. Let TDs disable our overwriting, with an environment variable, such that it is configurable at a studio-global, machine level.
  5. Make it obvious in the docs for Blender that's what we're doing.

At the end of the day, causing hard-to-debug issues with overwriting excepthook is bad, but better than insta-killing Blender.

@jasperges
Copy link
Contributor Author

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).

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

No branches or pull requests

3 participants