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

Add Blender 2.80 support (this will resolve #361) #466

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

jasperges
Copy link
Contributor

So, this is a bigger one. :) Adding Blender 2.80+ as host to Avalon.

Some notes up front:

  • It is tested, but not thoroughly yet and only on Linux. I hope to have some artists who will start using it soon (mainly on Windows).
  • Because Blender 2.80 uses Python 3.7 there is some specific Python 3 stuff in there, for example the type annotations. How do you feel about this?
  • To play around with it, a config that supports Blender is needed. You can try this one. It's basically the Colorbleed config with a few Blender things. I expect to add extra things to this configuration in the next weeks, which will also enable me to better test the integration itself.

That being said, let's have a discussion how we can work towards adding this to Avalon. Do you see issues, is there missing functionality, did you encounter bugs, etc.?

Looking forward to your comments. :)

avalon/blender/pipeline.py Outdated Show resolved Hide resolved
avalon/blender/pipeline.py Outdated Show resolved Hide resolved
avalon/blender/pipeline.py Outdated Show resolved Hide resolved
avalon/blender/pipeline.py Outdated Show resolved Hide resolved
avalon/blender/pipeline.py Outdated Show resolved Hide resolved
avalon/blender/ops.py Outdated Show resolved Hide resolved
avalon/blender/ops.py Outdated Show resolved Hide resolved
avalon/blender/lib.py Outdated Show resolved Hide resolved
avalon/blender/lib.py Outdated Show resolved Hide resolved
avalon/blender/lib.py Outdated Show resolved Hide resolved
@jasperges
Copy link
Contributor Author

Thank you hound... :)

@jasperges jasperges mentioned this pull request Oct 30, 2019
@jasperges
Copy link
Contributor Author

I think I've fixed the main issues for now. The Travis build failing is because of Python 3 specific syntax in the Blender part (as I already mentioned in my first comment).

@mottosso
Copy link
Contributor

The Travis build failing is because of Python 3 specific syntax in the Blender part

Could you have a look to see whether you can fence Python 3-specific tests behind a if PYTHON_MAJOR == 3 block (using whichever naming convention we've got for that currently, if any)? Otherwise we could have a look at a dedicated CI pass for Python 3, but that would be a little unfortunate.

for example the type annotations. How do you feel about this?

Good question. I would like us to use them, but it will be a good few years before anyone in the Maya-camp can actually join in, as it would mean stripping support for Python 2 altogether. They would make reading and understanding more challenging for anyone until that time, but I also don't think it's a good idea to hold Blender back due to a slowly moving remainder of the VFX industry.

We'll have to pay attention to what is exclusive to Blender, such as utility functions that may or may not be useful elsewhere, but it shouldn't be too hard considering we've got the avalon.blender namespace. So my vote is "Go for it", but everyone should weigh in on this, as it would affect their ability to understand, contribute and debug this aspect of the codebase as well.

For completeness, not sure this works from a comment, but to trigger the GitHub automation gods, this PR fixes #361 (which should automatically close that upon merging this PR).

Finally, merging this may be tricky, for the same reason #438 is challenging; testing it is hard. Blender + Avalon is still a niche, so this will be likely be your baby, at least initially. I'm happy delegating responsibility for this section of the codebase - ensuring code consistency and that it works - to you, if you'd be happy with that.

Finally again, can you post screenshots? :) I think we're all eager to see it in action, Blender user or no.

And finally, great work!! Wonder if this would make us eligable for an announcement on one of the many Blender forums? Following the latest convention, I suspect people are in an open frame of mind with regards to switching things around and trying new things.

@jasperges
Copy link
Contributor Author

For completeness, not sure this works from a comment, but to trigger the GitHub automation gods, this PR fixes #361 (which should automatically close that upon merging this PR).
That's why I have 'this will resolve #361' in the title. I believe that should work... :)

Screenshots etc.: yes, I should definitely show them somewhere and probably make some documentation... Where would you like for screenshots/documentation to go?

I will get back later to your other points.

@BigRoy
Copy link
Collaborator

BigRoy commented Oct 30, 2019

Because Blender 2.80 uses Python 3.7 there is some specific Python 3 stuff in there, for example the type annotations. How do you feel about this?

Agreed. Go for it, as long as it's only inside the code areas that are supposed to only have be Python 3+ where it has support for it, e.g. the avalon.blender space.

Finally again, can you post screenshots? :) I

Moving footage of it working, like publishing and loading/update/removing content would be amazing!

Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Quickly checked the code itself. Looks very clean! 🚀 I put down two comments regarding code style - consider it a start to a discussion at this stage.

Nice work!

avalon/blender/lib.py Show resolved Hide resolved
avalon/blender/ops.py Outdated Show resolved Hide resolved
@mkolar
Copy link
Member

mkolar commented Oct 31, 2019

We actually recently started using blender for certain tasks, so we'll be able to test it out fairly quickly I think. I'll try to find time today to pull it and give it a spin. Looking forward

@jasperges
Copy link
Contributor Author

Could you have a look to see whether you can fence Python 3-specific tests behind a if PYTHON_MAJOR == 3 block (using whichever naming convention we've got for that currently, if any)? Otherwise we could have a look at a dedicated CI pass for Python 3, but that would be a little unfortunate.

Yes, I can definitely have a look at that. Can you point me in the right direction where I can find the relevant code/tests? I had a look at the test things, but couldn't quite figure out what is exactly run on Travis.
Also looking at this, maybe it would make sense to make some tests for Blender, like there is for Maya. Because it's open source we could easily create a docker with Blender to run some tests. How do you feel about that?

Finally, merging this may be tricky, for the same reason #438 is challenging; testing it is hard. Blender + Avalon is still a niche, so this will be likely be your baby, at least initially. I'm happy delegating responsibility for this section of the codebase - ensuring code consistency and that it works - to you, if you'd be happy with that.

I completely understand it's tricky to merge this. If I could chop it into smaller pieces, I would, but I don't see any way to do that.
I would be happy to take the responsibility for the blender part. Personally I think this can be considered alpha or maybe beta stage. It will mature over time as we use it more and hopefully other people join the party.

Finally again, can you post screenshots? :) I think we're all eager to see it in action, Blender user or no.

In due time, have patience... 😛
What would be a good place to put some documentation for Blender part? Would it be okay, to put an extra README.md in avalon/blender? Or would you prefer it to be in the docs repo?

And finally, great work!! Wonder if this would make us eligable for an announcement on one of the many Blender forums? Following the latest convention, I suspect people are in an open frame of mind with regards to switching things around and trying new things.

Thanks! Before posting it I would prefer let it mature a bit. Also the configuration for the Blender part needs more, so there is actually something to show.

@jasperges
Copy link
Contributor Author

Because Blender 2.80 uses Python 3.7 there is some specific Python 3 stuff in there, for example the type annotations. How do you feel about this?

Agreed. Go for it, as long as it's only inside the code areas that are supposed to only have be Python 3+ where it has support for it, e.g. the avalon.blender space.

Cool. And yes, it will only be in avalon.blender and setup/blender.

Moving footage of it working, like publishing and loading/update/removing content would be amazing!

Yes, yes... I will dig up some nice looking assets, possibly from one of the Bender movies so I have more to show then just some cubes and monkeys.

@jasperges
Copy link
Contributor Author

We actually recently started using blender for certain tasks, so we'll be able to test it out fairly quickly I think. I'll try to find time today to pull it and give it a spin. Looking forward

Cool! Looking forward to hear how it works for you.

@BigRoy BigRoy requested a review from mkolar November 4, 2019 08:54
@mkolar
Copy link
Member

mkolar commented Nov 19, 2019

So I have a few comments / questions.

I've managed to make it work in pype config (just copying the blender parts). I'm running it on windows and run into some issues. Creator and Workfiles are not loading. I didn't dig into code details before checking that on linux all the key parts are working for you.

What I'm currently struggling with is Creator that crashes with this

Traceback (most recent call last):
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\blender\ops.py", line 248, in execute
    return super().execute(context)
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\blender\ops.py", line 212, in execute
    self._window.show(*args, **kwargs)
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\tools\creator\app.py", line 630, in show
    window = Window(parent)
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\tools\creator\app.py", line 142, in __init__
    name = SubsetNameLineEdit()
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\tools\creator\app.py", line 73, in __init__
    anim.setPropertyName("status_color")
TypeError: setPropertyName(self, Union[QByteArray, bytes, bytearray]): argument 1 has unexpected type 'str'

location: <unknown location>:-1

And workio that can't see registered host that I'm able to trace back to api.install(blender) that throws this

Exception ignored in: <function ImagePreviewCollection.__del__ at 0x000001961F2FFF28>
Traceback (most recent call last):
  File "C:\Program Files\Blender Foundation\Blender\2.80\scripts\modules\bpy\utils\previews.py", line 79, in __del__
    f"{self!r}: left open, remove with "
ResourceWarning: <ImagePreviewCollection id=0x1961f77fe60[1], <super: <class 'ImagePreviewCollection'>, <ImagePreviewCollection object>>>: left open, remove with 'bpy.utils.previews.remove()'
Traceback (most recent call last):
  File "<blender_console>", line 1, in <module>
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\pipeline.py", line 81, in install
    host.install(config)
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\blender\pipeline.py", line 118, in install
    ops.register()
  File "C:\code\pype-setup-2.0\repos\avalon-core\avalon\blender\ops.py", line 395, in register
    bpy.utils.register_class(cls)
ValueError: register_class(...): already registered as a subclass

I'm assuming this is not happening to you @jasperges

@BigRoy
Copy link
Collaborator

BigRoy commented Nov 20, 2019

What I'm currently struggling with is Creator that crashes

Should be fixed with #469

The second one sounds like you are running install multiple times on the same code. Not sure...

@jasperges
Copy link
Contributor Author

jasperges commented Nov 20, 2019

@mkolar Nope, that doesn't happen to me.

As @BigRoy mentions, the first error should be fixed if you update Avalon.

The second issue also feels to me like Roy mentions. This kind of thing mostly happens to me if the register function in Blender fails and you try to do it a second time (for example reloading a broken add-on).
Also I'm not sure if this part:

Exception ignored in: <function ImagePreviewCollection.__del__ at 0x000001961F2FFF28>
Traceback (most recent call last):
  File "C:\Program Files\Blender Foundation\Blender\2.80\scripts\modules\bpy\utils\previews.py", line 79, in __del__
    f"{self!r}: left open, remove with "
ResourceWarning: <ImagePreviewCollection id=0x1961f77fe60[1], <super: <class 'ImagePreviewCollection'>, <ImagePreviewCollection object>>>: left open, remove with 'bpy.utils.previews.remove()'

is related to the Avalon Blender integration. It could be because I 'register' the Pyblish icon as a custom icon. If it is, something is definitely wrong and an earlier host.install probably failed causing these errors running the install a second time.

Today I have some Windows testing planned. So let's see how that will work out.

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 12, 2019

@mkolar are you able to give this another go shortly? :)

@jasperges
Copy link
Contributor Author

Could you have a look to see whether you can fence Python 3-specific tests behind a if PYTHON_MAJOR == 3 block (using whichever naming convention we've got for that currently, if any)? Otherwise we could have a look at a dedicated CI pass for Python 3, but that would be a little unfortunate.

Yes, I can definitely have a look at that. Can you point me in the right direction where I can find the relevant code/tests? I had a look at the test things, but couldn't quite figure out what is exactly run on Travis.
Also looking at this, maybe it would make sense to make some tests for Blender, like there is for Maya. Because it's open source we could easily create a docker with Blender to run some tests. How do you feel about that?

@mottosso Can you shed some light on this? ^^ I would like to fix the tests on Travis, but couldn't figure out how it works.

@jasperges
Copy link
Contributor Author

jasperges commented Jan 8, 2020

If possible I would like to merge this integration in the near future. The things I would like to do before a merge:

  • Add a README.md for the Blender part.
  • Go through todo's in code (Blender part) and see if anything needs changing or fixing for now.
  • Make the tests on Travis pass.
  • Wait for Merge Context Manager + Work Files (Fix #412) #442 to be merged (optional).
  • Make some (simple) tests for the Blender part (optional).

Do you have any thoughts on this? Do you see any problems or things that need to be fixed or changed?

@iLLiCiTiT
Copy link
Contributor

I would add sys.excepthook = lambda *exc_info: traceback.print_exception(*exc_info) into blender's pipeline install method to not crash blender because of unhandled exceptions in avalon.

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 8, 2020

Good list @jasperges - hit me up if you have a simple test config, etc. and you want me to run through it once too.

For the README.md maybe these Avalon Integration notes I wrote out could be helpful. I noticed it was non-trivial to know which environment variables to set per integration so set this up as a reference.

@mottosso
Copy link
Contributor

mottosso commented Jan 8, 2020

I would add sys.excepthook = lambda

Don't!

Consider what happens if another vendor did that at the same time, or if Blender themselves at one point installed one for their own internal purposes. There can only be one of those; Avalon should be able to handle exceptions internally, to carry its own weight.

If not, then we need to give the user an option to install it, explaining the risks (such as random breakage of anything else that depend on it, including Blender).

I had a look at the test things, but couldn't quite figure out what is exactly run on Travis.

I think we solved this quite well in the Qt.py project, see here for example.

    if PYTHON == 2:
        def test_sip_api_already_set():
            """Raise ImportError with sip was set to 1 with no hint, default"""
            __import__("PyQt4.QtCore")  # Bypass linter warning
            import sip

If would simply exclude that block of code in anything that isn't Python 2. The more complicated option would be to have separate modules, and run the Python 2 interpreter for one, and Python 3 for another. We would need to modify the CI file for that which can get complicated fast.

In general, still strive to write code that run on both Python's so that you won't need such an if-block. It'll still be necessary to support both in parallel for at least another few years.

@mottosso
Copy link
Contributor

mottosso commented Jan 8, 2020

Oh my, tests are running in Python 2.6? :O I don't see how, it should be using mayapy (which is 2.7).

Unsure about that one.

Maya and Houdini are also excluded, so it seems safe and the sensible
thing to do.
@jasperges
Copy link
Contributor Author

jasperges commented Jan 8, 2020

@mottosso The fix was actually very simple. Nose was failing during the discovery of tests, no code was actually tested. Maya and Houdini were already excluded, now I added Blender (here and here)
and the problem is gone.

Oh my, tests are running in Python 2.6? :O I don't see how, it should be using mayapy (which is 2.7).

It seems that either mayapy -m pip install nose installs it globally (for the system Python 2.6) or Nose is already installed system wide and that one is picked up by mayapy. Would have to inspect the Docker images to fully understand what's happening. Weird...

@jasperges
Copy link
Contributor Author

I would add sys.excepthook = lambda

Don't!

Consider what happens if another vendor did that at the same time, or if Blender themselves at one point installed one for their own internal purposes. There can only be one of those; Avalon should be able to handle exceptions internally, to carry its own weight.

If not, then we need to give the user an option to install it, explaining the risks (such as random breakage of anything else that depend on it, including Blender).

Given these considerations I see 2 options:

  1. 'Hide' it behind an environment variable or something similar.
  2. Add an explanation and instructions how to add the sys.excepthook to README.md for the Blender integration (which I'm writing now). You can easily add it in core/setup/blender/startup/avalon_setup.py or add your own startup script for Blender that does it.

Personally I would prefer the second option.

@jasperges
Copy link
Contributor Author

Good list @jasperges - hit me up if you have a simple test config, etc. and you want me to run through it once too.

Thanks, will do.

For the README.md maybe these Avalon Integration notes I wrote out could be helpful. I noticed it was non-trivial to know which environment variables to set per integration so set this up as a reference.

That is exactly what I was doing. 😄

Nice resource by the way. At some point I quickly wanted to test Avalon with Houdini (Apprentice) but didn't have a clue how to set it up.

avalon/tools/workfiles/app.py Show resolved Hide resolved
avalon/tools/workfiles/app.py Show resolved Hide resolved
avalon/blender/ops.py Show resolved Hide resolved
avalon/blender/lib.py Show resolved Hide resolved
@iLLiCiTiT
Copy link
Contributor

@jasperges any updates about blender?

  • I've tested your current branch and almost tools are working until they are re-opened. Reason (I think) is that all tools call module.window.close() if module.window is not None (or workfiles call it on open button press).
  • close method trigger ACCESS VIOLATION error in blender (blender terminates is less than 100ms). Didn't found real reason but it happens to me only when close is called or widget is deleted (and it is totally random sometimes does sometimes not). I think that blenders garbage collector with combination of operators and Qt's processEvents is deadly combination to memory access.
  • I've tried different access: Inherit QAppliction where I've stored widget objects of tools by their id directly in blender ops which means I had to skip implemented show of each tool module. That works nice but It is totally different approach than in other hosts...

Few questions:
1.) Is same issue happening (had happened) to you?
2.) Are you solving that or have you solved that?
3.) If not what do you (and others) think about that different approach? (I can share branch if necessary)

@jasperges
Copy link
Contributor Author

@iLLiCiTiT A quick answer for now. Will check more thoroughly on Monday.

  1. Is this on Windows? This could potentially be something that happens only Windows. I mainly tested this on Linux and I don't recall having this problem. At least not frequently. In the future maybe we should use bqt. Then probably things like this can be fixed over there and everyone benefits. But that would mean getting support for Linux and macOS first.
  2. Like with 1, I can't recall having seen this problem. Will carefully check again, especially on Windows.
  3. If possible I would like to avoid by-passing the show function of each tool module. But we have to see if that is possible.

@jasperges
Copy link
Contributor Author

jasperges commented Feb 17, 2020

@iLLiCiTiT I double checked and and do not have the problem you describe in 1 and 2. In Blender I do not keep track of the module.window directly. There is one odd thing though: because the Loader handles the window a bit differently I have put in some extra code there. This seems to prevent the error you are having.
I also have this PR which should resolve that issue.

Could it be that you call the tools differently then I do or have a different approach of getting the QApplication instance? It also seems related to this: #514

@jasperges
Copy link
Contributor Author

@iLLiCiTiT I'm now hitting the same ACCESS VIOLATION error as you did. I have the error when trying to open a file with the new workfiles app. It seems it somehow is caused when running Pyblish. So far this is my 'recipe for disaster': open Blender > open workfiles app, opening file and closing workfiles app > open Pyblish and close it > open new file > open workfiles app, open file >> crash.

I don't have time to further investigate the issue right now, but I will get back about this. I really don't know if it is in any way related, but Pyblish uses a 'modal operator'. I will re-work it so it uses the new bpy.app.timer and see what happens then. If that doesn't change anything, I will check your solution.

To be honest I'm stumbling in the dark here, but we'll see...

@jasperges
Copy link
Contributor Author

jasperges commented Feb 26, 2020

@iLLiCiTiT Finally had some time today to dive deeper into the issue. Apart from the Access Violation error in Windows I kept getting 'Segmentation Faults' when opening files with the Workfiles tool in Linux.
After lots of digging around and trying stuff out, I figured out what the problem is. If you open a .blend file from within a 'timed' function, Blender crashes. So, this will crash Blender:

import bpy

count = 0

def timed_function():
    global count
    count += 1
    print(f"Crashing in {count}...")
    if count > 2:
        bpy.ops.wm.open_mainfile(filepath=<blendfilepath>)
    return 1  # run every second

bpy.app.timers.register(timed_function, persistent=True)

On Windows it might take a second try. But after it has crashed it will crash consistently afterwards unless you log out in back in again (in my tests).

Now to find a solution for this...


EDIT: this might be related to these:

EDIT 2:
It seems that at the moment you can't run operators from within timed functions. For some you might be able to override the context, but I haven't been able to do this for bpy.ops.wm.open_mainfile.
As a (temporary) solution I switched back to using a modal operator. To prevent windows getting stuck when the modal operator stops (for example when opening a new file) I simply close all Qt windows that are managed by the current QApplication.instance().
It's not great, but at least it makes it workable again...

You can check this out in this branch.

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.

6 participants