Cycles: add instancing support in light tree #106683
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#106683
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "weizhen:light_tree_instance"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Build a subtree for each unique mesh light.
Implementation details:
!SD_OBJECT_TRANSFORM_APPLIED
, so the cost is comparable with the previous implementation where we needed to transform three vertices of the triangle when computing the importance.While therecursive_build()
function is parallelized for meshes with many triangles, the process of iterating through mesh lights and building subtrees is not. This means that if there are numerous small unique mesh lights, the building process could be slower than before. However, it's possible to parallelize by splitting the loop into two.Performance
Tested with Apple M1
1.60.34s after, with 99% of the time being spent on computingLightTreeMeasure
, since the instances have non-uniform scalings, resulting in non-trivial transformations. Some side notes about this transformation are in the comment (light_tree.cpp:340
).Ladder.blend
from #77889 (comment), with 205 mesh lights, 5 of which are unique. There are slight differences in the noise present in the two renderings.The bistro scene is 1.7x slower when buiding the light tree, probably because there are 79 mesh lights with less than 4096 triangles (and 12 mesh lights with 10824 triangles). As mentioned before, the process of iterating through the mesh lights and building subtrees is not yet parallelized.After implementing parallelization, the time required for building the light tree in the bistro scene is now half of the time in main.@blender-bot build
Cycles: speed up light tree build with many instanced mesh lightsto Cycles: add instancing support in light treeThis sounds like an impressive speedup!
Only did a quick pass so far. Didn't spot obvious logic mistakes, just some stlying/naming comments.
@ -64,3 +64,3 @@
KERNEL_DATA_ARRAY(KernelLightTreeNode, light_tree_nodes)
KERNEL_DATA_ARRAY(KernelLightTreeEmitter, light_tree_emitters)
KERNEL_DATA_ARRAY(uint, light_to_tree)
KERNEL_DATA_ARRAY(uint, lamp_to_tree)
What is the difference between lamp and light?
This is something confusing in the current state of kernel. Is it possible to stick to
light
? Or somewhere have a comment about whet the difference is?I think we should get rid of the term "lamp". It was used in Blender before and renamed to light in 2.8, but some usage of lamp stuck around in Cycles.
Maybe best to use "light" for area/spot/sun/point, and then "emitter" for lights and emissive triangles. Though not as part of this patch of course. I don't mind it being called lamp as long as that is still used in other places.
@ -1370,6 +1370,13 @@ using BoundingCone = struct BoundingCone {
float theta_e;
};
using LightTreeNodeType = enum LightTreeNodeType {
Not sure why such syntax is needed.
For being C-style (which matches the legacy roots of the OpenCL days in the file) it is
typedef LightTreeNodeType { ... } LightTreeNodeType;
.For the C++ you can simply do
enum LightTreeNodeType{ ... };
@ -1404,3 +1413,2 @@
/* The location in the lights or triangles array. */
int prim_id;
union {
Such things I always get wrong. It is easy to run into an extension created by MSVC. It seems fine here, but worth double-checking it does not generate a warning about it (in the case I've missed something).
@ -156,3 +190,1 @@
it is an inner node. */
int first_emitter_index; /* Leaf nodes contain an index to first emitter. */
unique_ptr<LightTreeNode> children[2]; /* Inner node has two children. */
int type = LIGHT_TREE_INNER;
Add a note that this is a bitmask of
LightTreeNodeType
.@ -64,3 +64,3 @@
KERNEL_DATA_ARRAY(KernelLightTreeNode, light_tree_nodes)
KERNEL_DATA_ARRAY(KernelLightTreeEmitter, light_tree_emitters)
KERNEL_DATA_ARRAY(uint, light_to_tree)
KERNEL_DATA_ARRAY(uint, lamp_to_tree)
This array stores indices of the build-in lights (area light, spot light, sun light, point light), these lights are often called "lamps" throughout the code. Besides that we have mesh lights, which are kept in a separate array called
mesh_to_light
. Because both types are called "lights", I thought it would be less confusing to rename the other one tolamp_to_tree
.I can add a comment explaining this.
Looks generally fine, only minor comments.
Would be nice to parallelize building of multiple mesh lights, but also not a blocker for landing this.
@ -1385,3 +1390,1 @@
* Otherwise, it's an index to the node's second child. */
int child_index;
int num_emitters; /* leaf nodes need to know the number of emitters stored. */
LightTreeNodeType type;
We don't use enum types in struct like this and instead use an integer type, because we don't know that the size is the same on the CPU and GPU.
Maybe we can start using them with an explicit size in the declaration like
enum LightTreeNodeType : int {
orenum LightTreeNodeType : uint8_t {
? Would need to be tested on the buildbot, not sure it works for all GPU compilers.@ -1388,0 +1395,4 @@
union {
int first_emitter; /* Leaf node stores the index of the first emitter. */
int right_child; /* Inner node stores the index of the right child. */
int reference; /* Instance node refers to the node with the subtree. */
I think it would be a bit more clear like this:
@ -1407,0 +1422,4 @@
struct {
int object_id;
int node_id;
}; /* Mesh Light. */
Same suggestion as for
KernelLightTreeNode
, could be more explicit about which emitter type the data belongs to.I forgot to publish the comments.
@ -64,3 +64,3 @@
KERNEL_DATA_ARRAY(KernelLightTreeNode, light_tree_nodes)
KERNEL_DATA_ARRAY(KernelLightTreeEmitter, light_tree_emitters)
KERNEL_DATA_ARRAY(uint, light_to_tree)
KERNEL_DATA_ARRAY(uint, lamp_to_tree)
The code indeed often refers to the light sources as lamps. And this is exactly what causes quite some confusion.
Like,
KernelLight *klight = &kernel_data_fetch(lights, lamp)
. So, is the lamp different from light, how can they be used interchangeably.We can probably also ignore the problem until the lights are actually a real objects in Cycles, and then everything is just an object.
@ -65,2 +65,3 @@
KERNEL_DATA_ARRAY(KernelLightTreeEmitter, light_tree_emitters)
KERNEL_DATA_ARRAY(uint, light_to_tree)
KERNEL_DATA_ARRAY(uint, lamp_to_tree)
KERNEL_DATA_ARRAY(uint, mesh_to_tree)
Isn't it more like
object_to_tree
, as it is indexed by object?Also, maybe
object_to_light_tree
?`@blender-bot package
Package build started. Download here when ready.
Code looks fine, but not sure what is going on with those crashes on the buildbot on Linux and Windows. I guess a debug + asan build on macOS might crash too.
The crash is caused by use of non-POD in an union without its proper construction.
It is possible to ensure the expected lifetime of those unique pointers by manually initializing them, but it is going to be fragile. Doing so is highly discouraged in the modern C++, and I would not suggest going that route.
Alternatives are:
std::variant
which is a type-safe alternative of the unions, which also works for non-POD. Using it will free you from doing all the object life-time management, in exchange of slightly bigger memory usage (the variant stores type of the value it is holding).Attached the PoC of the
std::variant
for the reference. I did not have time to build it to scale or paint it. Surely some of the boiler plate could be reduced somewhat.union
tovariant
490f41856b@blender-bot package
Package build started. Download here when ready.
#include <variant>
@blender-bot package
Package build started. Download here when ready.
Thanks for the patch. The two proposed alternatives require the same amount of memory (16 bytes more compared to the version with
union
), but withvariant
the intention of the code is clearer and looks safer, so I usedvariant
in the end.committed as
bfd1836861
Pull request closed