Cycles: build Light Tree in parallel #105862

Merged
Weizhen Huang merged 4 commits from weizhen/blender:parallel_light_tree into main 2023-03-20 18:02:23 +01:00
3 changed files with 29 additions and 30 deletions
Showing only changes of commit 2050a0a615 - Show all commits

View File

@ -655,8 +655,8 @@ void LightManager::device_update_tree(Device *,
else {
/* Fill in the stacks. */
left_index_stack[stack_id] = index;
right_node_stack[stack_id] = node->children[right];
node = node->children[left];
right_node_stack[stack_id] = node->children[right].get();
node = node->children[left].get();
stack_id++;
}
}

View File

@ -210,7 +210,7 @@ LightTree::LightTree(vector<LightTreePrimitive> &prims,
root = create_node(BoundBox::empty, OrientationBounds::empty, 0.0f, 0);
/* All local lights are grouped to the left child as an inner node. */
recursive_build<left>(root, 0, num_local_lights, &prims, 0, 1);
recursive_build(left, root.get(), 0, num_local_lights, &prims, 0, 1);
task_pool.wait_work();
OrientationBounds bcone = OrientationBounds::empty;
@ -221,14 +221,13 @@ LightTree::LightTree(vector<LightTreePrimitive> &prims,
bcone = merge(bcone, prim.bcone);
energy_total += prim.energy;
}
LightTreeNode *distant_node = create_node(BoundBox::empty, bcone, energy_total, 1);
distant_node->make_leaf(num_local_lights, num_distant_lights);
root->children[right] = distant_node;
root->children[right] = create_node(BoundBox::empty, bcone, energy_total, 1);
root->children[right]->make_leaf(num_local_lights, num_distant_lights);
}
template<Child child>
void LightTree::recursive_build(LightTreeNode *parent,
void LightTree::recursive_build(const LightTreeChild child,
LightTreeNode *parent,
brecht marked this conversation as resolved Outdated

Is there a reason this is a template argument instead of a regular function argument?

It's only used for parent->children[child], not worth specializing the function for as far as I can see.

Is there a reason this is a template argument instead of a regular function argument? It's only used for `parent->children[child]`, not worth specializing the function for as far as I can see.

It's also used for recursive_build<left>(current_node, start, middle, prims, bit_trail, depth + 1);, I think it looks better than recursive_build(0, current_node, start, middle, prims, bit_trail, depth + 1);

It's also used for `recursive_build<left>(current_node, start, middle, prims, bit_trail, depth + 1);`, I think it looks better than `recursive_build(0, current_node, start, middle, prims, bit_trail, depth + 1);`

I think this would look ok too?

recursive_build(left, current_node, start, middle, prims, bit_trail, depth + 1);

Generally there should be a performance reason for using templates like this. Not a big deal for any individual function, but still would rather not do it here.

I think this would look ok too? ``` recursive_build(left, current_node, start, middle, prims, bit_trail, depth + 1); ``` Generally there should be a performance reason for using templates like this. Not a big deal for any individual function, but still would rather not do it here.
const int start,
const int end,
vector<LightTreePrimitive> *prims,
@ -250,8 +249,8 @@ void LightTree::recursive_build(LightTreeNode *parent,
energy_total += prim.energy;
}
LightTreeNode *current_node = create_node(bbox, bcone, energy_total, bit_trail);
parent->children[child] = current_node;
parent->children[child] = create_node(bbox, bcone, energy_total, bit_trail);
LightTreeNode *current_node = parent->children[child].get();
const bool try_splitting = num_prims > 1 && len(centroid_bounds.size()) > 0.0f;
int split_dim = -1, split_bucket = 0, num_left_prims = 0;
@ -285,23 +284,23 @@ void LightTree::recursive_build(LightTreeNode *parent,
/* Recursively build the left branch. */
if (middle - start > MIN_PRIMS_PER_THREAD) {
task_pool.push([=] {
recursive_build<left>(current_node, start, middle, prims, bit_trail, depth + 1);
recursive_build(left, current_node, start, middle, prims, bit_trail, depth + 1);
});
}
else {
recursive_build<left>(current_node, start, middle, prims, bit_trail, depth + 1);
recursive_build(left, current_node, start, middle, prims, bit_trail, depth + 1);
}
/* Recursively build the right branch. */
if (end - middle > MIN_PRIMS_PER_THREAD) {
task_pool.push([=] {
recursive_build<right>(
current_node, middle, end, prims, bit_trail | (1u << depth), depth + 1);
recursive_build(
right, current_node, middle, end, prims, bit_trail | (1u << depth), depth + 1);
});
}
else {
recursive_build<right>(
current_node, middle, end, prims, bit_trail | (1u << depth), depth + 1);
recursive_build(
right, current_node, middle, end, prims, bit_trail | (1u << depth), depth + 1);
}
}
else {

View File

@ -54,7 +54,7 @@ OrientationBounds merge(const OrientationBounds &cone_a, const OrientationBounds
*/
/* Left or right child of an inner node. */
enum Child {
enum LightTreeChild {
brecht marked this conversation as resolved Outdated

This name is too generic to put in the global Cycles namespace. Should be either defined inside the LightTree class, or named LightTreeChild.

This name is too generic to put in the global Cycles namespace. Should be either defined inside the `LightTree` class, or named `LightTreeChild`.
left = 0,
right = 1,
};
@ -102,10 +102,10 @@ struct LightTreeNode {
OrientationBounds bcone;
float energy;
uint bit_trail;
int num_prims = -1; /* The number of primitives a leaf node stores. A negative
number indicates it is an inner node. */
int first_prim_index; /* Leaf nodes contain an index to first primitive. */
LightTreeNode *children[2]; /* Inner node. */
int num_prims = -1; /* The number of primitives a leaf node stores. A negative
number indicates it is an inner node. */
int first_prim_index; /* Leaf nodes contain an index to first primitive. */
unique_ptr<LightTreeNode> children[2]; /* Inner node. */
LightTreeNode() = default;
@ -134,7 +134,7 @@ struct LightTreeNode {
* BVH-like data structure that keeps track of lights
* and considers additional orientation and energy information */
class LightTree {
LightTreeNode *root;
unique_ptr<LightTreeNode> root;
atomic<int> num_nodes = 0;
uint max_lights_in_leaf_;
@ -150,17 +150,17 @@ class LightTree {
LightTreeNode *get_root() const
{
return root;
return root.get();
};
/* NOTE: Always use this function to create a new node so the number of nodes is in sync. */
LightTreeNode *create_node(const BoundBox &bbox,
const OrientationBounds &bcone,
const float &energy,
const uint &bit_trial)
unique_ptr<LightTreeNode> create_node(const BoundBox &bbox,
const OrientationBounds &bcone,
const float &energy,
const uint &bit_trial)
{
num_nodes++;
return new LightTreeNode(bbox, bcone, energy, bit_trial);
return make_unique<LightTreeNode>(bbox, bcone, energy, bit_trial);
brecht marked this conversation as resolved Outdated

We try to avoid new and delete in new code to avoid potential memory allocation bugs. Instead unique_ptr and make_unique can be used.

We try to avoid `new` and `delete` in new code to avoid potential memory allocation bugs. Instead `unique_ptr` and `make_unique` can be used.
}
private:
@ -169,8 +169,8 @@ class LightTree {
/* Do not spawn a thread if less than this amount of primitives are to be processed. */
enum { MIN_PRIMS_PER_THREAD = 4096 };
template<Child child>
void recursive_build(LightTreeNode *parent,
void recursive_build(LightTreeChild child,
LightTreeNode *parent,
int start,
int end,
vector<LightTreePrimitive> *prims,