Encapsulation in Instancer

[danieldresser-ie]

This is very early, but it looks like it's going to work fine. There's some cleanup required for how affects() is handled, and I should probably do a bit more testing, but after switching to a method where I don't use contexts, this is looking reasonably good.

The main thing I could use feedback on is the Version 1 and Version 2 PRs. Originally, I did what we talked about: adding internal versions of [compute/hash]Branch[ChildNames/Object/Set], which take a boolean for whether they are being evaluated outPlug() or capsuleScenePlug(), and when appropriate, calling them with inCapsule=true from an overridden hashObject when evaluating the capsuleScene plug. But when it came time to implement computeSet, I encountered a problem: I would have to reimplement all the logic from BranchCreator::computeSet, which is kind of complicated, and sounds like it may change in the future to increase parallelism.

Then I realized that I got just turn this logic inside out: the logic for computeSet on outPlug when encapsulating is trivial ( no sets are added to until you expand the capsules, so the input sets can just be passed through ( Oh wait, I guess I do still need to check if the set exists in the input, and return an empty set if it doesn't? ) ). This meant that I could just have the overridden computeSet handle the special easy case of computing the sets when encapsulating, instead of using any of the default mechanism, and then computeBranchSet can be set up to always do the more complicated default behaviour without checking the encapsulate plug.

For sets, this is both cleaner and simpler - but it's a bit annoying that the structure is now quite different for object/childNames and for sets. In version 2, I apply the same transform to object/childNames, which I think also feels a bit cleaner, but it costs an extra call to parentAndBranchPaths, which is probably not worth it.

So, I still need to make a decision on this, and do some cleanup, and it might make sense to add support for the prototype encapsulation as well before putting this up for merging. But it all feels like it's going reasonably smoothly so far.

[johnhaddon]

Remind me to talk to you about this later - I think we can do better than invalidate the capsule every time the out plug changes. It should be sufficient to depend only on the poor man's hash of prototypesPlug() and the exact hash of the points object. The goal is not to invalidate the capsule for any old change in the main input scene.

[danieldresser-ie]

Quick update at end of day: I thought this was pretty well ready to go, then did some interactive testing, and discovered a problem:

You suggested that we could improve performance by using the actual hash of the points object, because the Capsules don't change if the hash of the points object doesn't change. This is true ... the problem is that the capsules are invalidated based on the dirtyCount, and the dirtying of the points object feeds into that. I could just hash in the dirtyCount of the points object, but it would be nice if reusing the points object did reuse the capsules, since they are still correct, we just don't know that they're valid.

I got as far as adding a test showing the problem, but didn't get any further. There may be simple solution that I will realize as soon as I think about it on Monday.

[danieldresser-ie]

Haven't really concluded anything other than our conversation this morning, but I did confirm that everything appears to work fine if I just comment out throwIfExpired.

[danieldresser-ie]

After rebasing onto the new verison of the other PR, this now appears to be working.

[danieldresser-ie]

Checked and noticed the failing test ... was just the UI failing, so I quick added some documentation ... and noticed the plug name is too long to fit in the Node Editor.

I guess that probably needs fixing? I would accept basically any proposal other than "encapsulateInstances", which I still feel describes precisely the wrong thing. If we're not doing "encapsulatePrototypes" for now, maybe we just call it "encapsulate"?

[danieldresser-ie]

Resolved my last comment by setting label. Ready for review

[johnhaddon]

Thanks Daniel, this is looking good. I've made quite a few little nitpicky comments inline, but overall I'm really pleased with how simple you've managed to make it look - nice one.

I did a bit of testing here with a just over a million instances, and things are looking good. This seems to add around 1% overhead from the additional checks when we're not taking advantage of encapsulation, which I don't think is a big deal. In return, the set generation is greatly improved as we had hoped. Testing with a prototype with 3 sets, encapsulated set generation is down to 0.002s vs 10s for a downstream Encapsulate node. Memory usage also drops more than a 1G. I think this will be a significant improvement for the production scenes we're targeting.

Thanks!
John

[johnhaddon]

Can you explain how this checks against expired Capsules?

[johnhaddon]

This method doesn't exist at the moment, so we should probably say talk about ignoring dirtiness on the globals plug rather than using capsuleDirtyCount().

[johnhaddon]

Let's elaborate a little bit here, to mention why this is better than downstream encapsulation.

[danieldresser-ie]

Addressed comments in separate commit, will need squashing after review.

Only comment I have is that in naming the UI section "Encapsulate", my original thought was that it makes the UI name match the plug name "Encapsulate" > "Instance Groups" matches "encapsulateInstanceGroups", but I don't feel strongly about this, so I've renamed to "Encapsulation" as requested.

I probably need to rebase this again onto the maintenance branch as well? We don't want to target master any more?

Requested changes made

[johnhaddon]

Thanks Daniel, I spotted a couple of typos so I've taken the liberty of fixing those and tweaking the release notes slightly at the same time as doing a final review and rebasing to 0.59. Will merge once CI is done.