Fix #119692: Account for mesh lights with a light tree root index of 0 #119770

Alaska · 2024-03-22T06:02:33+01:00

Alaska commented

2024-03-22 06:02:33 +01:00

When using light linking with the light tree, the root index of a
mesh light subtree can be 0. The current code assumed this wasn't
possible, and as such it caused rendering issues, specifically the
incorrect computation of the PDF of certain mesh lights during
forward path tracing.

So we adjust the code to allow mesh light subtree root node
indexs of 0.

This was worked on by @Alaska, @Sergey, and @weizhen

When using light linking with the light tree, the root index of a mesh light subtree can be 0. The current code assumed this wasn't possible, and as such it caused rendering issues, specifically the incorrect computation of the PDF of certain mesh lights during forward path tracing. So we adjust the code to allow mesh light subtree root node indexs of 0. This was worked on by @Alaska, @Sergey, and @weizhen

Alaska added the

Module

Render & Cycles

label 2024-03-22 06:03:21 +01:00

Alaska changed title from ~~WIP: Fix #119692: Ensure light tree node 0 can't be a leaf node.~~ to Draft: Fix #119692: Ensure light tree node 0 can't be a leaf node.

2024-03-22 07:22:33 +01:00

Sergey Sharybin commented

2024-03-22 14:46:18 +01:00

I've spent some time looking into the results of the flattening code, but i could not see anything obviously wrong. So then I've started looking into the light_tree_sample and light_tree_pdf, while keeping in mind your findings.

I think what actually is happening is the light_tree_pdf which uses root_index != 0 as some sort of indication that leaf node is a mesh light, and need to fetch its subtree. When light linking is not used root_index = 0 indicates the scene tree, and anything else indicates some other subtree. With light linking enabled it is possible to have a subtree's root_index which is not 0.

So I think the fix as as easy as making it root_index = -1 to indicate that subtree is not needed to be traversed. Attached a fix for it.

I've spent some time looking into the results of the flattening code, but i could not see anything obviously wrong. So then I've started looking into the `light_tree_sample` and `light_tree_pdf`, while keeping in mind your findings. I think what actually is happening is the `light_tree_pdf` which uses `root_index != 0` as some sort of indication that leaf node is a mesh light, and need to fetch its subtree. When light linking is not used `root_index = 0` indicates the scene tree, and anything else indicates some other subtree. With light linking enabled it is possible to have a subtree's `root_index` which is not 0. So I think the fix as as easy as making it `root_index = -1` to indicate that subtree is not needed to be traversed. Attached a fix for it.

light_tree_pdf.diff

935 B

Alaska force-pushed fix-119692-tree-flatten from a2b3eb9995 to 1cd5c1e758

2024-03-22 15:48:21 +01:00

Compare

Alaska changed title from ~~Draft: Fix #119692: Ensure light tree node 0 can't be a leaf node.~~ to Fix #119692: Account for mesh lights with a light tree root index of 0

2024-03-22 15:48:32 +01:00

Alaska requested review from Sergey Sharybin 2024-03-22 15:48:46 +01:00

Alaska requested review from Weizhen Huang 2024-03-22 15:48:54 +01:00

Alaska requested review from Brecht Van Lommel 2024-03-22 15:49:00 +01:00

Alaska commented

2024-03-22 15:49:48 +01:00

I'm currently targeting the main branch of Blender for this pull request. Feel free to rebase against blender-v4.1-release.

Weizhen Huang commented

2024-03-22 16:04:00 +01:00

@Sergey I am not sure if you misunderstood something or it was just typo, but without light linking 0 is the index of root node of the main tree, and the mesh lights have subtrees with non-zero root_index. Not sure about the logic with light linking.

@Sergey I am not sure if you misunderstood something or it was just typo, but without light linking `0` is the index of root node of the main tree, and the mesh lights have subtrees with non-zero `root_index`. Not sure about the logic with light linking.

Alaska commented

2024-03-22 16:07:40 +01:00

The code change proposed by Sergey (and is now in the current iteration of this pull request) is fine by me. I made the same fix (but with a different method) earlier with 1caa6716d9 (In my local repo only). Although the explaintion Sergey provided seems to be a bit backwards in some areas based on my understanding of the code.

I still find it odd that node_index 0 can't be a leaf node with light linking disabled, but it's possible with light linking enabled. And that's what a2b3eb9995 (From the earlier iteration of this pull request) attempted to fix. But it's up to the Cycles developers at this point to decide which approach they want to take.

The code change proposed by Sergey (and is now in the current iteration of this pull request) is fine by me. I made the same fix (but with a different method) earlier with https://projects.blender.org/Alaska/blender/commit/1caa6716d9f37d53d34715b25bed9f843c556ff1 (In my local repo only). Although the explaintion Sergey provided seems to be a bit backwards in some areas based on my understanding of the code. I still find it odd that node_index 0 can't be a leaf node with light linking disabled, but it's possible with light linking enabled. And that's what https://projects.blender.org/blender/blender/commit/a2b3eb99955b91589597ee00554ab2c8c7c300b3 (From the earlier iteration of this pull request) attempted to fix. But it's up to the Cycles developers at this point to decide which approach they want to take.

Weizhen Huang commented

2024-03-22 16:12:33 +01:00

@Alaska without light linking, the right child of the root node is always reserved for distant/backgrounds lights, even if they are not present in the scene, so 0 can't be the leaf.
I do not remember the exact reason I did that, maybe it was just simpler for the implementation.

@Alaska without light linking, the right child of the root node is always reserved for distant/backgrounds lights, even if they are not present in the scene, so `0` can't be the leaf. I do not remember the exact reason I did that, maybe it was just simpler for the implementation.

Sergey Sharybin commented

2024-03-22 16:32:07 +01:00

With the light linking we had to get rid of some assumptions about the tree topology, and store things in a more flexible manner, and some parts of the trees are filled in first. So while the statement about right child reserved for a distant for scene tree is still expected to be valid, you can not come to a conclusion about what node with index 0 is. It might or night not be root.

Although the explaintion Sergey provided seems to be a bit backwards in some areas based on my understanding of the code.

Yeah, I think i've mixed the = 0 and != 0 in the explanation. Corrected it now.

With the light linking we had to get rid of some assumptions about the tree topology, and store things in a more flexible manner, and some parts of the trees are filled in first. So while the statement about right child reserved for a distant for scene tree is still expected to be valid, you can not come to a conclusion about what node with index 0 is. It might or night not be root. > Although the explaintion Sergey provided seems to be a bit backwards in some areas based on my understanding of the code. Yeah, I think i've mixed the `= 0` and `!= 0` in the explanation. Corrected it now.

Sergey Sharybin approved these changes 2024-03-22 16:33:23 +01:00

Sergey Sharybin left a comment

Attaching file from the report which would be handy to add to our regression suit.

light_link_mesh_tree.blend

938 KiB

Weizhen Huang commented

2024-03-22 17:01:23 +01:00

If we can not make the assumption anymore that the root index of the main tree is 0, and root_index is only used for traversing subtree here, I would rename it to subtree_index, or subtree_root. This way it's much less confusing, because assigning root_index = -1 has no concrete meaning to me, while subtree_index = -1 seems clear to indicate a subtree does not exist.

If we can not make the assumption anymore that the root index of the main tree is 0, and `root_index` is only used for traversing subtree here, I would rename it to `subtree_index`, or `subtree_root`. This way it's much less confusing, because assigning `root_index = -1` has no concrete meaning to me, while `subtree_index = -1` seems clear to indicate a subtree does not exist.

Alaska added 1 commit 2024-03-23 01:04:06 +01:00