Ensure light collections are dirtied on creation

[aloysbaillet]

Description of Change(s)

We found that when undoing the deletion of a light with an active collection, the re-creation of that light was not dirtying the content of the collection.

It is unfortunately quite tricky to provide a test for this use case, but we have one internally in Omniverse...
Our test goes like this:

• create 2 cubes
• create a light
• set the light's lightLink collection to refer to cube2
• check in viewport with a light-collection-enabled renderer that things are OK
• delete the light
• undo that deletion
• check in viewport with a light-collection-enabled renderer that things are OK again (it wasn't without this fix)

[spitzak]

We ran into this as well, but it was pretty simple to have the renderDelegate update things due to the creation of the light and the calling of Sync() on it, so I am not absolutely certain this dirtying is needed

[aloysbaillet]

Thanks Bill, I'm still discovering the Hydra world... but in our render delegate the Light::Sync was indeed called, but each linked geometry rprim wasn't and we need them called to re-register them with the light link. I'm curious how you made it work?

[spitzak]

This is lame but in Light::Sync() it checks if the categories changed and if so it calls sceneDelegate->GetRenderIndex().GetChangeTracker().MarkAllRprimsDirty(pxr::HdChangeTracker::DirtyCategories); which calls Sync() on all the Geometry.

…

On Tue, Oct 12, 2021 at 4:34 PM Aloys Baillet ***@***.***> wrote: Thanks Bill, I'm still discovering the Hydra world... but in our render delegate the Light::Sync was indeed called, but each linked geometry rprim wasn't and we need them called to re-register them with the light link. I'm curious how you made it work? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1653 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMCEOOMZ3JDAUF377LBYTUGTAY5ANCNFSM5FZXXWVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[aloysbaillet]

Fair enough :)
I wanted to avoid marking all prims dirty though, the proposed change only marks the collection members dirty...

[jilliene]

Filed as internal issue #USD-6961

[tcauchois]

Hey Aloys,

Like I mentioned on your other PR, I'm a bit hesitant to have special case code in delegate.cpp since it becomes hard to maintain. There's also a few bits you're missing. This will be a bit involved to fix but I'll try to lay out the pieces and answer any questions.

First up, we have an object UsdImaging_CollectionCache that actually answers all of the queries, both GetLightParamValue(lightLink) and GetCategories(rprim). Here's the class: https://github.com/PixarAnimationStudios/USD/blob/release/pxr/usdImaging/usdImaging/collectionCache.h ... and here's where we populate it: https://github.com/PixarAnimationStudios/USD/blob/release/pxr/usdImaging/usdImaging/lightAdapter.cpp#L131 ... In order to keep the collection cache up to date with any changes, I'd recommend updating lightAdapter.cpp and lightFilterAdapter.cpp to call UpdateCollection in TrackVariability (which is already there); call UpdateCollection in ProcessPropertyChange if the collection changes (and return HdLight::DirtyParams); and call RemoveCollection in _RemovePrim. This way the results of any calls into the collection cache will always be up-to-date.

The next piece you need is hydra dirty bit tracking. Ideally, you could do this when you call UpdateCollection; the collection cache removes a bunch of gprims from the collection list, and adds a bunch of other ones, and you could just produce a list of paths to mark with DirtyCategories. However, TrackVariability (for example) is called multi-threaded and can't call the hydra change-tracker, which isn't threadsafe. And neither TrackVariability nor ProcessPropertyChange have a UsdImagingIndexProxy, which is the class needed to call MarkDirty.

The way out of this that I see is for the collection cache to keep a dirty gprim list. Every time you call RemoveCollection or UpdateCollection, add hydra paths to an SdfPathSet (or Vector) on the collection cache itself. At the end of UsdImagingDelegate::ApplyPendingUpdates(), which is our function that applies USD edits to imaging, loop through this set calling MarkDirty(DirtyCategories); I'm imagining putting that loop here https://github.com/PixarAnimationStudios/USD/blob/release/pxr/usdImaging/usdImaging/delegate.cpp#L1040 ... right after _ExecuteWorkForVariabilityUpdate. Or the other option, of course, is Bill's approach of marking all rprims dirty with DirtyCategories whenever the collections change. That's faster if your light collections tend to be big. (Note that the trivial collection of "/" avoids all of these codepaths entirely).

That should do a much better job of handling the category change-tracking.

Hopefully this makes sense! Any work to fix this up is super appreciated, and I'm happy to answer whatever questions you have about the code.

Thanks,
Tom

[aloysbaillet]

Thanks Tom, I'll look into this!

[tcauchois]

Can skip the else here.

[tcauchois]

Actually, can just call _UpdateCollectionsChanged. I don't think collections are allowed to be time-varying? And if you did want to check that, instead of seeing if the collection has new contents, you'd want to ask whether the attributes are time varying.

[aloysbaillet]

I removed the call altogether as the attributes cannot be animated... I'm guessing that if timesamples are added on the includeRoot bool that could cause some issues though?

[tcauchois]

I feel like there's some missing bits here... If _queryForId[id] before you update it != query, also _MarkContentDirty(stage, _queryForId[id])?

[aloysbaillet]

The RemoveCollection call at the beginning of UpdateCollection will mark the previous collection content dirty, wouldn't this cover it?

[tcauchois]

Even if the query has exclude paths, I'd still expect UsdComputeIncludedPathsFromCollection to come up with the right list. That should return the set of paths that pass both the include & exclude rules, right?

[aloysbaillet]

Indeed, but prims that were previously excluded from the collection would not be returned in the include set, and these also need to be marked dirty...

[aloysbaillet]

Actually as the RemoveCollection call will catch all paths from the outgoing collection, the UpdateCollection should be able to safely ignore the excluded paths... Will test that further!

[aloysbaillet]

I've cleaned and simplified the code further, also fixed some formatting inconsistencies.
The UsdImagingLightAdapter::ProcessPropertyChange method now returns HdLight::DirtyBits::DirtyCollection when it detects a collection change (using hashes for comparison, but without having to store them).

I'm still having one refresh issue that I'm chasing but I'm confident it is a problem in our render delegate (I had to change it quite a lot to handle HdLight::DirtyCollection properly).
I'm quite confident the prman render delegate will need updating to take advantage of the now-working DirtyCollection flag, but I'm not sure if I'll be able to do that (I think I would need a license first ;) ).

[aloysbaillet]

I've fixed a compilation error (UsdLuxLight->UsdLuxLightAPI as I was testing on an older USD version sorry!) and @sirpalee kindly tested these changes with success using the Arnold render delegate.