Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Instanced attribute sets #91

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Instanced attribute sets #91

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
945611f
Initial support for instanced attributes
dedoardo Jul 15, 2021
f33f6f6
Indexing instanced attributes by interpolation type
dedoardo Jul 16, 2021
c2f6fd7
Replaced particle_index with instance index
dedoardo Jul 16, 2021
583d73e
Removed duplicated attributes
dedoardo Jul 16, 2021
7e8291c
Do not duplicate attributes
dedoardo Jul 19, 2021
ead81e2
Update object offset only if necessary
dedoardo Jul 19, 2021
d5f67a9
Removed debugging comments
dedoardo Jul 19, 2021
dadb64b
Merge branch 'master' into feature/instanced_attributes
skwerner Jul 20, 2021
725a4fe
Fixed uninitialized variable
skwerner Jul 21, 2021
ddb9c1f
Fixed double initialization. Better encapsulation of instances. More …
dedoardo Jul 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/kernel/geom/geom_attribute.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,24 @@ ccl_device_inline AttributeDescriptor find_attribute(KernelGlobals *kg,
desc.type = (NodeAttributeType)(attr_map.w & 0xff);
desc.flags = (AttributeFlag)(attr_map.w >> 8);

/* Offset the index by the instance number */
if (desc.flags & ATTR_INSTANCED) {
const int instance_index = kernel_tex_fetch(__objects, sd->object).instance_index;

if (desc.element == ATTR_ELEMENT_VERTEX || desc.element == ATTR_ELEMENT_VERTEX_MOTION) {
desc.offset += instance_index * kernel_tex_fetch(__objects, sd->object).numverts;
} else if (desc.element == ATTR_ELEMENT_CORNER || desc.element == ATTR_ELEMENT_CORNER_MOTION) {
/* todo(Edoardo): Implement once we have internal subdivision surfaces */
desc.offset += instance_index * kernel_tex_fetch(__objects, sd->object).numfaces * 3;
} else if (desc.element == ATTR_ELEMENT_FACE) {
desc.offset += instance_index * kernel_tex_fetch(__objects, sd->object).numfaces;
} else if (desc.element == ATTR_ELEMENT_CURVE_KEY) {
desc.offset += instance_index * kernel_tex_fetch(__objects, sd->object).numkeys;
} else {
desc.offset += instance_index;
}
}

return desc;
}

Expand Down
12 changes: 6 additions & 6 deletions src/kernel/geom/geom_motion_point.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ ccl_device_inline int find_attribute_point_motion(KernelGlobals *kg,

ccl_device_inline float4 motion_point_attribute_for_step(KernelGlobals *kg,
int offset,
int numkeys,
int numverts,
int numsteps,
int step,
int prim,
Expand All @@ -62,7 +62,7 @@ ccl_device_inline float4 motion_point_attribute_for_step(KernelGlobals *kg,
if (step > numsteps)
step--;

offset += step * numkeys;
offset += step * numverts;

return kernel_tex_fetch(__attributes_float3, offset + prim);
}
Expand All @@ -78,8 +78,8 @@ ccl_device_inline float4 motion_point_attribute(KernelGlobals *kg,
int attr)
{
/* get motion info */
int numsteps, numkeys;
object_motion_info(kg, object, &numsteps, NULL, &numkeys);
int numsteps, numverts;
object_motion_info(kg, object, &numsteps, &numverts, NULL);

/* figure out which steps we need to fetch and their interpolation factor */
int maxstep = numsteps * 2;
Expand All @@ -93,9 +93,9 @@ ccl_device_inline float4 motion_point_attribute(KernelGlobals *kg,

/* fetch key coordinates */
float4 point_attr = motion_point_attribute_for_step(
kg, offset, numkeys, numsteps, step, prim, n_points_attrs, attr);
kg, offset, numverts, numsteps, step, prim, n_points_attrs, attr);
float4 next_point_attr = motion_point_attribute_for_step(
kg, offset, numkeys, numsteps, step + 1, prim, n_points_attrs, attr);
kg, offset, numverts, numsteps, step + 1, prim, n_points_attrs, attr);

/* interpolate between steps */
return (1.0f - t) * point_attr + t * next_point_attr;
Expand Down
6 changes: 4 additions & 2 deletions src/kernel/kernel_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,7 @@ typedef enum AttributeStandard {
typedef enum AttributeFlag {
ATTR_FINAL_SIZE = (1 << 0),
ATTR_SUBDIVIDED = (1 << 1),
ATTR_INSTANCED = (1 << 2)
} AttributeFlag;

typedef struct AttributeDescriptor {
Expand Down Expand Up @@ -1534,10 +1535,12 @@ typedef struct KernelObject {
float dupli_uv[2];

int numkeys;
int numverts;
int numverts; /* Number of vertices in a mesh or points in a cloud */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I have problems wrapping my head around this PR to understand what is going on. How numkeys is different than numverts and why numfaceshas been added. I suspect that keys are actually vertices for curves? And why numfaces is important for instancing. Is because we want to override face attributes?

Copy link
Contributor Author

@dedoardo dedoardo Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the goal is to override attributes with different interpolation (or element in cycles speak). If an attribute has ATTR_ELEMENT_VERTEX interpolation, we want to bump the offset by the number of vertices.
The current code was using numkeys for points in a cloud, while attributes authored on those point have ATTR_ELEMENT_VERTEX as interpolation. If we wanted to branch on the type of geometry to calculate the indexed offset, the code would have to be scattered and duplicated in the various primitive_*_attribute. I decided to make that change to streamline the logic.

So far numkeys/numverts was being used for motion blur positions, so faces wasn't needed until now. I know that on the delegate side we currently only support constants, but the USD point instancer specification doesn't impose this limit so I went for a more generic approach. And for completeness.

Copy link
Contributor Author

@dedoardo dedoardo Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are talking about the fact that numkeys and numverts are never set together and we could avoid using one of them, I totally agree. It hasn't been done in this PR to limit the number of changes as it would require patching more parts of the code.

int numfaces;
uint16_t num_tfm_steps;
uint16_t num_dfm_steps;

uint instance_index;
uint patch_map_offset;
uint attribute_map_offset;
uint motion_offset;
Expand All @@ -1549,7 +1552,6 @@ typedef struct KernelObject {
float cryptomatte_asset;

float shadow_terminator_offset;
float pad1, pad2;
} KernelObject;
static_assert_align(KernelObject, 16);

Expand Down
16 changes: 10 additions & 6 deletions src/render/attribute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ CCL_NAMESPACE_BEGIN
/* Attribute */

Attribute::Attribute(
ustring name, TypeDesc type, AttributeElement element, Geometry *geom, AttributePrimitive prim)
: name(name), std(ATTR_STD_NONE), type(type), element(element), flags(0)
ustring name, TypeDesc type, AttributeElement element, Geometry *geom, AttributePrimitive prim, uint instances)
: name(name), std(ATTR_STD_NONE), type(type), element(element), flags(0), instances(instances)
{
/* string and matrix not supported! */
assert(type == TypeDesc::TypeFloat || type == TypeDesc::TypeColor ||
Expand All @@ -44,6 +44,10 @@ Attribute::Attribute(
else {
resize(geom, prim, false);
}

if (instances > 1) {
flags |= ATTR_INSTANCED;
}
}

Attribute::~Attribute()
Expand Down Expand Up @@ -164,7 +168,7 @@ size_t Attribute::element_size(Geometry *geom, AttributePrimitive prim) const

size_t Attribute::buffer_size(Geometry *geom, AttributePrimitive prim) const
{
return element_size(geom, prim) * data_sizeof();
return element_size(geom, prim) * data_sizeof() * instances;
}

bool Attribute::same_storage(TypeDesc a, TypeDesc b)
Expand Down Expand Up @@ -328,8 +332,8 @@ void Attribute::get_uv_tiles(Geometry *geom,

/* Attribute Set */

AttributeSet::AttributeSet(Geometry *geometry, AttributePrimitive prim)
: geometry(geometry), prim(prim)
AttributeSet::AttributeSet(Geometry *geometry, AttributePrimitive prim, uint instances)
: geometry(geometry), prim(prim), instances(instances)
{
}

Expand All @@ -350,7 +354,7 @@ Attribute *AttributeSet::add(ustring name, TypeDesc type, AttributeElement eleme
remove(name);
}

Attribute new_attr(name, type, element, geometry, prim);
Attribute new_attr(name, type, element, geometry, prim, instances);
attributes.emplace_back(std::move(new_attr));
return &attributes.back();
}
Expand Down
9 changes: 7 additions & 2 deletions src/render/attribute.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,15 @@ class Attribute {
AttributeElement element;
uint flags; /* enum AttributeFlag */

/* Multiplier on the size of the buffer */
uint instances;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we follow some here some encapsulation design principles or has this field intentionally been made public for free manipulation? What happens if we instantiate AttributeSet aset{geo, prim, 100} and then we update instances? For instance aset.instances = 10000; Will that resize all attributes automatically? Maybe we should proactively hide it under some method that automatically does it for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, the field can be made private. Attribute::resize already uses the number of instances to determine the buffer size. I don't think we need to expose the functionality you are talking about.


Attribute(ustring name,
TypeDesc type,
AttributeElement element,
Geometry *geom,
AttributePrimitive prim);
AttributePrimitive prim,
uint instances = 1);
Attribute(Attribute &&other) = default;
Attribute(const Attribute &other) = delete;
Attribute &operator=(const Attribute &other) = delete;
Expand Down Expand Up @@ -175,8 +179,9 @@ class AttributeSet {
Geometry *geometry;
AttributePrimitive prim;
list<Attribute> attributes;
uint instances;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can Attribute::instances be different than AttributeSet::instances? What is the difference between those two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can't be different and the one in Attribute should be private. It's the attribute set that adds the attributes, but I forgot to patch the other add calls, will do that.


AttributeSet(Geometry *geometry, AttributePrimitive prim);
AttributeSet(Geometry *geometry, AttributePrimitive prim, uint instances = 1);
~AttributeSet();

Attribute *add(ustring name, TypeDesc type, AttributeElement element);
Expand Down
Loading