Tree View for Nuke Gizmo

https://github.com/SpinVFX/Cryptomatte/tree/crypto_tree_view

Hey Guys. I started working on a tree view for the cryptomatte gizmo in Nuke. It opened up a few points:

first I did a round of pylinting on the code, and it lit up with a few possible errors in the current code, mostly in the tests (some function calls with invalid arguments) https://github.com/SpinVFX/Cryptomatte/commit/adaf4d7eb7143945f85ea989686ef8b16e04248c?diff=split Possible errors at line 64, 68. The rest was just pep8 formatting recommendations.
I then added the tree view. So far it's QT based, though I would like to try the built in nuke SceneView knob to see if it performs any better. The challenge there is getting the cryptomatte data for the whole frame range so that the tree view stays consistent. So far I rely on an external function that makes a set out of multiple sidecar manifests into a new JSON file and use that as my tree. Could not find a quick way to extract it out of EXRs, though Ben Dickson mentions he's done something similar in another thread. This is my main issue so far, getting good data to put in the tree. Once it's in there, it works quite well. One thing I was thinking about was to implement some sort of disk caching for cryptomatte manifests of the whole scene. It's easy enough to implement, the tricky part being re-caching it if the frames get overwritten. Other challenge is some manifests represent paths as /path/to/object, while some others use path.to.object.
Once I had my tree working, I found the methods used in the gizmo to add or remove from the matteList to be really not optimized for running in a loop, trying to bulk add a few thousand objects would get extremely slow. I have modified the functions so that the current matteList gets converted to a set of floats, and the list being added or subtracted being also converted to a set of floats, do a set union or a set subtraction, then convert back to stings/ based on the manifest data. That didn't seem to affect performance much while picking one by one (it may even have improved it, to be confirmed) but it does a huge difference for large sets.

Just wanted to put this up there, as I'd love some feedback from you guys. Our Lighters didn't want to use cryptomatte because it's too manual to pick a few hundred objects manually, and I think we should put our brains together to see how we can best implement that as part of cryptomatte.

Cheers Erwan

Sweet!

I'll take a look into it since it's something I was writing my self.

Congrats!

Cheers, Diogo

On Thu, Jun 21, 2018 at 1:27 PM Erwan Leroy notifications@github.com wrote:

https://github.com/SpinVFX/Cryptomatte/tree/crypto_tree_view

Hey Guys. I started working on a tree view for the cryptomatte gizmo in Nuke. It opened up a few points:

-

first I did a round of pylinting on the code, and it lit up with a few possible errors in the current code, mostly in the tests (some function calls with invalid arguments) SpinVFX/Cryptomatte@adaf4d7?diff=split https://github.com/SpinVFX/Cryptomatte/commit/adaf4d7eb7143945f85ea989686ef8b16e04248c?diff=split Possible errors at line 64, 68. The rest was just pep8 formatting recommendations.

I then added the tree view. So far it's QT based, though I would like to try the built in nuke SceneView knob to see if it performs any better. The challenge there is getting the cryptomatte data for the whole frame range so that the tree view stays consistent. So far I rely on an external function that makes a set out of multiple sidecar manifests into a new JSON file and use that as my tree. Could not find a quick way to extract it out of EXRs, though Ben Dickson mentions he's done something similar in another thread. This is my main issue so far, getting good data to put in the tree. Once it's in there, it works quite well. One thing I was thinking about was to implement some sort of disk caching for cryptomatte manifests of the whole scene. It's easy enough to implement, the tricky part being re-caching it if the frames get overwritten. Other challenge is some manifests represent paths as /path/to/object, while some others use path.to.object.

Once I had my tree working, I found the methods used in the gizmo to add or remove from the matteList to be really not optimized for running in a loop, trying to bulk add a few thousand objects would get extremely slow. I have modified the functions so that the current matteList gets converted to a set of floats, and the list being added or subtracted being also converted to a set of floats, do a set union or a set subtraction, then convert back to stings/ based on the manifest data. That didn't seem to affect performance much while picking one by one (it may even have improved it, to be confirmed) but it does a huge difference for large sets.

Just wanted to put this up there, as I'd love some feedback from you guys. Our Lighters didn't want to use cryptomatte because it's too manual to pick a few hundred objects manually, and I think we should put our brains together to see how we can best implement that as part of cryptomatte.

Cheers Erwan

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Psyop/Cryptomatte/issues/79, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfs4ifWrekjJVlZ-V8fc8fGL5zCf_Vlks5t-8lzgaJpZM4UyZDr .

Hi Erwan (@herronelou) , thanks for bringing us this. I really appreciate the contributions and want to encourage this kind of thing. I’m making a point of saying this because I’m in the position of having to be a stickler for certain things for the sake of the integrity of the project, so please don’t take any of the criticism as lack of interest in your work - encouraging contributions is important to us.

Tree view:

I’m definitely into the idea of a tree view. Loading the manifest on every frame is not great and a faster way to do this would be nice. It could also be loaded on demand, when the tab is accessed.

In the Cryptomatte spec, these are not meant to be paths, so there is no guarantee that the names actually represent a hierarchy or that one can be extracted from them (and in fact it usually can’t be). However in certain cases, such as Katana-like paths, a hierarchy could be extracted, and you could do it automatically in these cases. Also worth noting - there is also no standard for what the path delimiter is, in the case that the identifier is a path.

Set manipulations:

I’m definitely open to accepting changes like that. (Subject to everything continuing to work properly- see below). There’s a wrinkle here which makes it not totally straightforward - there may be things selected by names which are not in the manifest. That drove the complexity of the existing code. However, it could absolutely be improved, it just needs to be done in a way that doesn’t introduce subtle bugs. (See the part about tests below).

One word of caution about picking hundreds of objects. Currently we use an expression node for extraction, so there are potential performance issues here with large selections. We find the most commonly used Cryptomattes are the material ones, since those are already conglomerations of items and you don’t need to select hundreds of things, and this maps well to what people actually use mattes for most of the time, changing material properties. One thing we could use is a C++ node for performing the extraction more efficiently than an expression node for such a use case.

PyLinting:

There are some issues here.

The pylinting broke the tests. It removed @classmethod decorators, which caused most of the tests not to work. I’m not sure why it thought that was a safe change to make.

Second, the linting making tons of formatting changes, which is an issue if I want to merge your changes, or if you want to merge updates from mine. The issue is that your branch now contains small changes to hundreds of lines, which will be merge conflicts. I think it’s best to do large scale reformatting when introducing major version changes that are likely to cause merge conflicts anyway. As for the errors that it found, thanks for that and we’ll look into them.

Failed tests:

Once the tests are repaired, there are 5 failed tests, once of which I think is an actual new bug. It is no longer possible to remove objects which are selected by name but not in the manifest. The tests should be fixed to accommodate the changes.

Tests aren’t just a way for me to nit-pick other people’s work, they protect you from introducing subtle bugs in your own branch as well. So I highly recommend using them!

Failed: CSVParsingNuke.test_big_csv_through_gizmo
AssertionError: Round trip failed: Came out as: <-1.82577004544e+11>, <-2.74633926288e+29>, <-5.90858871874e-06>, <1.34596486673e-20>, <1.92957949132e+34>, <123.449996948>, <16866966528.0>, <2.59943444547e-08>, <2.82940902496e-31>, <5.10401209831e-23>
---------
Failed: CSVParsingNuke.test_csv_through_gizmo
AssertionError: Round trip failed: Came out as: <-2.54616133732e-10>

I suspect these two are a red herring, based on a way in which you’ve reorganized the code.

---------
Failed: CryptomatteNukeTests.test_matte_list_numeric
AssertionError: Removal of name by number failed.. ("<7.36562399642e+18>, heroflower" vs "<2.07262543558e+26>, <7.36562399642e+18>")

This is a new bug. This means that if you have something selected by name, but that name is not in the manifest, you can’t remove the item.

---------
Failed: CryptomatteNukeTests.test_output_preview
AssertionError: Preview image did not light up: (red) 0.0199350118637 vs 1.0

---------
Failed: CryptomatteNukeTests.test_output_preview_multi
AssertionError: Bunny pixels are wrong.: (red) 0.0199350118637 vs 1.0
---------
TESTING FAILED: 5 failed, 0 errors. (47 test cases.)

These two are also only failing because you changed the preview color from yellow to green. (Not a big deal of course, just a good idea to update the tests when you change a behavior).

Hey Jonah. Thanks for the detailed feedback. I'll try to do an answer as detailed. The current Branch was targeted for our in house use, and the idea to re-share it with the wolrd came later, and I had to get approval from the company to let the work out... more on this later.

Tree View.

By looking at multiple manifests, I did notice that there was no standards, and that is wasn't necessarily a tree structure. Even for those, however, it turned out to be quite helpful to have a list of objects sorted by name. (I have run the tree view on the rabbit test image, and it works pretty well, the issues come when I pick one of the few objects that do have children, as they have about 10k children, my processor goes crazy for a minute, though it does success eventually. I think it might be a dirty implementation of my recursive selection though.)

Set manipulation:

I'll cover the possible bug later on. About the performance issues when picking hundreds of objects, I did notice, though even at a few hundreds it processed really fast (my test asset in house is a spaceship with 152 visible pieces in the test frame) and that renders pretty much instantaneously at 2k. I tried on the Rabbit test image from you guys (with 32k objects). I selected all, and that did take like a good minute to render a frame.

Pylinting:

I'll be honest there, I didn't actually run the tests. I knew I should, but I got lazy and didn't feel like trying to figure out how to run them. I removed the @classmesthod decorators because they seemed wrong to me. Normally a classmethod must have the first parameter be 'cls', and not have self use. The classes you had here had self use and no cls argument, so I assumed they were regular methods. Now looking at it more, it made sense to have classmethods for unittests, and it runs in your case because you replaces cls by self both in the method definition and in the code. A proper fix from me would have been to replace all the 'self' by 'cls'.

About all the small format changes, I did that before thinking about trying to push back, so I didn't think this would even get shared outside of here. I realized it after I made it, but I still think it's a good idea to respect formatting standards. If you want to wait for a major version to do them that's cool. I tend to do them early because I use pycharm and it lights up all over the place when there are badly formatted pieces of code.

Failed Tests:

I think it should still be possible to remove an object by name even if it's not in the manifest. The difference probably comes from the fact that I regenerate the list on each picking. For example if you had 2 objects in the matteList as names, but not loaded in the manifest, on picking (let's say remove) they would be reconverted to floats (I do all my sets on floats), the one that matches the picked color would get removed, and the other one would make it back to the list, but because it's not in the manifest anymore, it would make it as a string instead of the original name. Seems like this is what is happening there: "<7.36562399642e+18>, heroflower" vs "<2.07262543558e+26>, <7.36562399642e+18>", heroflower is most likely not in the manifest anymore, so it got converted back to a float. It might not be ideal but it shouldn't affect functionality. The good news is that reloading the manifest and repicking anything would turn it back into heroflower. I was hoping to find a good way to load/cache sequence manifests and that might be able to be used as the default manifest for a sequence, so that the manifest is always there, and every object is there on every frame.

For the color test, I did change the colors, though I mean to invert the two I picked so that the default color doesn't change from the real cryptomatte.

If you're able to push your version of my branch with the fixed unittests I can go from there and fix these issues.

Sorry for the slow response, a lot’s been happening and this fell off my radar. Anyway, I’ve tried it out with the sidecar manifest example, and the tree view is cool for sure. I like that the preview lights up one color when you select things, and then have +, =, - buttons, and then lights up another color when you’ve added them (though I would switch which color is which to not introduce arbitrary changes). For deriving the “tree”, I think it might be useful to use any non-alphanumeric character as a possible delimiter, so you’d see “flower” as the parent of “flower.petal”.

It also might be nice to have an indication of what's already in the matte list, perhaps as italicized entries.

Really though, I’m hung up on something here, which also applies to wild card / regex keyers: the way it is right now, selecting a few hundred or thousand items generates an expression which is unacceptably slow to evaluate. As a result I think giving users a way to do that easily or accidentally is asking for trouble. I think until there’s a C++ plugin which can do this with acceptable performance, this is probably a pretty fraught proposition. I think a way of performing the extraction with better performance should be a priority here. Maybe this would be a whole replacement of the gizmo, or maybe it should be a minimal node which lives inside the gizmo, only replacing the expression node.

Coding stardards, linting, etc:

I think it’s probably best to mostly talk about the tree view and not get hung up on this, but I will add some notes about this. I’d like to reiterate that I think reformatting all the code is a mistake even if it's going to be a private branch, because I have changes in a dev branch that you'll find impossible to rebase your changes onto without a lot of manual work. I use an auto-formatter myself sometimes, but I only run it on code I'm actively working on because I know there are people like you who are maintaining their own branches with modifications in them, who will have a lot of work to do if I reformat everything.

For running the tests, there are instructions in the readme.

Hi Jonah, No worries about the slow feedback, I haven't been able to touch this myself since last time either.

Concerning your point about selecting many items, sadly that's usually where Cryptomatte shines. We've been doing a lot of science fiction shows recently, and have spaceships made up of hundreds or thousands of sub components. Yet we do want to have the ability to select a whole escape pod or the whole cockpit at once if needed. That rudimentary tree view has helped with that already. It's still fairly responsive, and definitely faster than re-rendering CG. It gets very slow on your bunny asset that has 32k objects, but our production scenes are more in the 1500 objects range and it works nicely.

There could be a compiled node that has the same controls and behavior as the gizmo, and give people the option to install either, same as you did with the mmh3 library. This way anyone with the gizmo can open a scene containing the compiled node (but slower) and vice versa, but you maintain compatibility.

I'll remake a branch from scratch to get rid of all my reformatting eventually to make it easier to compare. I wanted to try with the tree view that nuke provides rather than relying on QT as well, which does come with these little indicators to show whether something is loaded or not.

Things are a bit crazy for me this summer though, so I'm not sure when I'll be getting back to this.

Psyop / Cryptomatte

Tree View for Nuke Gizmo #79