Page MenuHome

Cycles: Implement render passes that contain all lighting from lights in a specified group
Needs ReviewPublic

Authored by Lukas Stockner (lukasstockner97) on Aug 15 2018, 2:33 AM.
Tokens
"Love" token, awarded by Alaska."Love" token, awarded by erei."Love" token, awarded by Shimoon."Love" token, awarded by pablovazquez."Love" token, awarded by HevertonFS."Love" token, awarded by leonumerique."Love" token, awarded by bintang."Love" token, awarded by wo262."Love" token, awarded by aliasguru."Love" token, awarded by navarroblaya."Love" token, awarded by bnzs."Love" token, awarded by juang3d.

Details

Summary

This can currently be done using renderlayers, but doing it in one render can be faster.

These passes are useful because they make it easy to tweak the lighting in scenes after rendering has finished.

There still are some ToDos left - the biggest limitation is the maximum of 8 light groups, but this is a tradeoff between number of passes and kernel memory usage.

Another question is how to handle denoising for these layers.

Depends on D3538 since it also has multiple named passes of the same type.

Diff Detail

Repository
rB Blender
Branch
custom-aov
Build Status
Buildable 5985
Build 5985: arc lint + arc unit

Event Timeline

Update to current master and changed from Groups to Collections.

Also adds some features, e.g. including the world background in a group.

Juan Gea (juang3d) added a comment.EditedAug 24 2019, 4:07 PM

Great feature.

I've been testing it, right now I find two things:

1.- It does not work well with adaptive sampling (I imagine AS needs some kind of tuning to support this)

2.- Clamping does not affect light group passes, so I'm getting fireflies out of it.

3.- Less important for a first implementation in master IMO, but somewhat important, it does not recognise emitter shaders as lights in the light groups

It seems also that this patch introduced a bug that when a light is inside a collection, and you enable compositing and save the file the collection with the light looses their users and disappears from the scene, the light it's still in the blend file, but since there is no collection it's not instanced in the scene.

@Martin Felke (scorpion81) did a small workwround for the time being for our build, but the problem seems to be bigger, he can explain what he did to fix this for the time being.

Also I assume this was introduced by this patch because we did not had this problem before, but maybe it's was a coincidence with a bug in master?

@Lukas Stockner (lukasstockner97) can you try to fix this and the clamping thing when you have some time please?

Hi @Lukas Stockner (lukasstockner97)

After a while with the patch working I think it works fine, but it lacks one important thing that I mentioned, without that thing the patch is less useful, but because we get very different pictures from the final rendered image and the light groups render layers.

The most important thing I think is the clamp, in complex interior scenes, where this is the most useful, to turn on/off and modulate ligths, we get a lot of fireflies.

Can you add the clamping to this please?

Other than that it is super useful and works great, thanks!!!!

Update to latest master and add several features and improvements:

  • Clamping is now applied per-contribution so that it can also be applied to lightgroups.
  • Lightgroups are written directly from path_radiance_accum_* to avoid the additional memory in PathRadiance (also allows to bump the lightgroup limit to 32)
  • Objects with emissive shaders are now also added to lightgroups
  • Some optimizations and fixes

There is one serious problem though: Something in the reference counting code goes wrong for the Collection property, leading to the Collection counter underflowing eventually.
This can break blendfiles since the Collections will be skipped on saving when the counter is zero.
Since this code doesn't touch any reference counts directly, this must be a general problem with the ID Property implementation here.
I'll try to reproduce the issue from a standalone addon.

@Juan Gea (juang3d) This update should cover all missing features. For the disappearing collections, I'll need to dig deeper.

Also, here's an image showing light groups in the Pavillon scene:

Hey @Lukas Mandok (Lukas)

@Juan Gea (juang3d) This update should cover all missing features. For the disappearing collections, I'll need to dig deeper.

Also, here's an image showing light groups in the Pavillon scene:

We have that problem solved!

@Martin Felke (scorpion81) solved it, the fix must be in our github, I’ll check the commit, it’s there somewhere :)

This is a general problem, not limited to this patch. It's good if there's a workaround, but it should be fixed properly.
I've created a bug report for it at T71250.

This is a general problem, not limited to this patch. It's good if there's a workaround, but it should be fixed properly.
I've created a bug report for it at T71250.

Well, just in case your fix fixes the more generic problem, here you have the commits in one of our branches, I'm not sure if what @Martin Felke (scorpion81) did was a workaround or the actual fix of the probelm, I'll try your script in our buiuld and see if it generates a problem too. :)

https://github.com/juangea/boneMaster/commit/ecf4d166dad1bde1bfe67d861e7e17c4af21ec04

https://github.com/juangea/boneMaster/commit/64caa97f563aaf362af8387c8b544c2e6682d338

https://github.com/juangea/boneMaster/commit/e32d31fbc2bb83eb9eea6fd91d22d36b3d3803bf

Ah, okay, that first commit looks like it should fix the underlying problem. Great, thanks for linking it!

The other two are already included in the new version of this patch.

Great!

It's not a public patch I think, maybe you or @Martin Felke (scorpion81) should make the patch public to the bug you reported :)

Rebase to latest master to include the Collection user fix.

Hey @Lukas Stockner (lukasstockner97) fantastic patch!

It is working mostly here, but I've found a bug with light groups here on the GafferCycles side but I'm not sure if you are experiencing it on the Blender-side but I'd imagine so...

When adding passes there's a sort function in there to sort based on component size:
https://developer.blender.org/diffusion/B/browse/master/intern/cycles/render/film.cpp$175

The problem here is that it changes the light group order so that the grouping is all mixed up, funnily enough the ordering isn't affected when you only make 9 light groups (I am using the string name of "lightgroup01" to "lightgroup32" for the name of the lightgroup passes).

Perhaps the sorting isn't affected with the particular name that Blender is using? What is the naming format on the Blender side?

Cheers!

Just a follow-up on a potential fix:

static bool compare_lightgroup_order(const Pass &a, const Pass &b)
{
  if ((a.type == PASS_LIGHTGROUP) && (b.type == PASS_LIGHTGROUP)) {
    string str = a.name;
    int numA = stoi(str.replace(str.begin(),str.end()-2,""));
    str = b.name;
    int numB = stoi(str.replace(str.begin(),str.end()-2,""));
    return (numA < numB );
  }
  return (a.type < b.type);
}
/* order from by components, to ensure alignment so passes with size 4
 * come first and then passes with size 1 */
sort(&passes[0], &passes[0] + passes.size(), compare_pass_order);
/* ensure lightgroups are in-order so that they match lightgroup masks */
for (size_t i = 0; i < passes.size(); i++)
{
  if (passes[i].type == PASS_LIGHTGROUP)
  {
    sort(&passes[i], &passes[0] + passes.size(), compare_lightgroup_order);
    break;
  }
}

This implies the last 2 characters in the string are the numbers eg. lightgroup01 to lightgroup32

Let me know what you think, cheers!

I'm reviewing this and doing some fixes to get this ready for 2.82, along with D4837.

The main thing I would like to change is the "LG " prefix. Ideally in the future we could support also other passes than combined for light groups (using LPEs perhaps), and in general I think it's good to indicate that this is a "Combined" pass.

I'm thinking of naming like this for OpenEXR, though I'm not entirely sure about the preferred order:

RenderLayer.Combined.LightGroup
RenderLayer.LightGroup.Combined

In the compositor and image editor this should then display as Combined.LightGroup or LightGroup.Combined as the pass name, or maybe even later it can explicitly understand these as light groups.

Does that make sense? Any preferences on the order?

I'm reviewing this and doing some fixes to get this ready for 2.82, along with D4837.

The main thing I would like to change is the "LG " prefix. Ideally in the future we could support also other passes than combined for light groups (using LPEs perhaps), and in general I think it's good to indicate that this is a "Combined" pass.

I'm thinking of naming like this for OpenEXR, though I'm not entirely sure about the preferred order:

RenderLayer.Combined.LightGroup
RenderLayer.LightGroup.Combined

In the compositor and image editor this should then display as Combined.LightGroup or LightGroup.Combined as the pass name, or maybe even later it can explicitly understand these as light groups.

Does that make sense? Any preferences on the order?

I think RenderLayer.Combined.LightGroup makes more sense, just because we could have other Combined types in the future, so Combined is a type of pass, and LightGroup is the specific type of combined pass.

I think RenderLayer.Combined.LightGroup makes more sense, just because we could have other Combined types in the future, so Combined is a type of pass, and LightGroup is the specific type of combined pass.

That seems like a strange interpretation to me, I would not consider LightGroup a "type of pass" but more a "type of layer".

Actually, reading the OpenEXR specs it would be the other way around. They actually have an example where they consider light groups a type of layer, that would then go in front.

So I think I'll go with LightGroup.Combined.

One thing that's bothering me in testing is that in a more complex scene it's not easy to create a correct set of light groups that when combined together, actually get you the proper combined image. You have to be careful to ensure lights are only included in one light group, and there a few cases like volume emission which are not part of any light group currently.

I'm thinking that we should make it so lights and objects can only be in a single light group at a time. This is more restrictive, but I think it's quite helpful and also simplifies the Cycles API. Some other renderers also work the same way. Everything that is not in a specified specific light group would then be available in a light group that contains the remainder of all lights that weren't included elsewhere. So then it's easy to get light groups that exactly reproduce the combined pass.

The tricky bit is how to handle this on the Blender side, since lights can be in multiple collections. In practice you'd probably organize your scene in a way that this is not so much an issue, and we could make the first light group in the list with that object win.

Rebased on latest master with custom shader AOVs. I also split the clamping
and light groups changes into two separate commits, though that won't be
visible in this diff.

Main difference is the naming of the render passes, but I'm looking into
further changes as described in the previous comment.

Why not let the user to deal with this manually? As much give a warning, but let the user decide

For example to create light states, you could want to have the same light in two different light groups. If you sum all the light groups you won’t get the same result of the actual render, but that’s not the target of light groups, the target is to be able to modify the scene lighting in compositing and for that you may want to have the same light repeated in different light groups.

Hi Brecht/Lukas,

Agreed that a light should only ever be associated with 1 light group.

Also to note, were you able to test and see if the light groups weren't getting re-ordered based on the component sort made in film? My patch seemed to solve this issue for me to ensure correct ordering, but it was only if the light groups went past like >9.
https://developer.blender.org/diffusion/B/browse/master/intern/cycles/render/film.cpp$181

Something else I want to bring up but it can wait for a follow-up because it's probably a Blender thing and not necessarily about light groups, now that a background light can be applied to a light group, could we open up the possibility for multiple background lights with each potentially having their own light group?

Cheers.

Rebase on master after clamping was committed.

@Juan Gea (juang3d), this is how things work in Renderman and Arnold (and for what it's worth, I'm the one who implemented LPEs and light groups in Arnold).. The group name is a property of the light there. It also fits better with LPEs if every light only has a single group, and will have better performance.

Even if lights are only in a single group, it's still possible to sum light groups together, but if it takes more setup time then. In my opinion we should make prioritize the typical use case.

@Alex Fuller (mistaed), I think your fix indeed works, but it might be obsolete once I've made further code changes here, not sure yet.

Some further code review revealed a few issues which I think should really be addressed for committing, and then this probably won't make it in time for 2.82. We better commit it in a few weeks when the window for 2.83 opens, when it's in a better state.

Mainly the way membership of a collection is determined is problematic, it is based on name. This works fine in simpler cases, but gives issues when using linked datablocks with the same name (not too hard to solve), but also when using instancing (harder). You want the light group to cover all lights under a given collection in the view layer, and determining if an object is made visible by a given collection is not as simple, maybe needs some additional Blender RNA API.

The integration with Blender OpenEXR saving and loading is also tricky. It displays ok in the unsaved render result, but then when you load it's not recognizing the pass names well. The way it works with denoising is also not ideal, to with the new OpenImageDenoise node it's better.

So overall I'd rather take a few more days to get this in a better state.

@Juan Gea (juang3d), this is how things work in Renderman and Arnold (and for what it's worth, I'm the one who implemented LPEs and light groups in Arnold).. The group name is a property of the light there. It also fits better with LPEs if every light only has a single group, and will have better performance.

Even if lights are only in a single group, it's still possible to sum light groups together, but if it takes more setup time then. In my opinion we should make prioritize the typical use case.

@Alex Fuller (mistaed), I think your fix indeed works, but it might be obsolete once I've made further code changes here, not sure yet.

Agree with the priorities.

Then I have a question, why not create a property in the lights and emission shaders to put the ligth group name (or to select from a list of created light groups)?

The thing is that if we are not going to be able to put the same light in different groups, then using collections don't make much sense I think, and using a property like the one you described is more logical, and easy to use, also don't mess with the sceen organization.

IMO the power of collections is precisely that, to be able to organize things with freedom.

I understand this could be more complex, and if in the future is something to take into account, leaving as it is right now makes sense (with the changes you mention I mean), but if that's something that won't be possible, then probably using collections is a bit too complex for the simple behaviour.

Also with unlimited lightgroups there is really no need to have the light repeated in several groups.

Keep in mind that I prefer to use collections, I'm just thinking about what you said, the use cases I can think about amd the UX of the feature.

In intern/cycles/blender/blender_object.cpp, in line 529 there is a typo:

LIGHGROUPS_NONE should be LIGHTGROUPS_NONE

Notice the lacking T in LIGHTGROUPS word :)

This feature will be on Blender 2.83? I could not find it on the latest builds.