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

wants to merge 10 commits into from

Conversation

dedoardo
Copy link
Contributor

@dedoardo dedoardo commented Jul 19, 2021 •
edited
Loading

Description
This patch allows objects to read from and index shared attribute sets.
The logic in the geometry manager has been changed to allocate an extra device attribute map for each InstanceGroup which is essentially an alias for an AttributeSet. The attribute requests are fulfilled by the instanced attribute set, or the geometry attribute set if the former are not found.

Design considerations are very welcome. One thing that can be addressed with the current method is that geometry attributes which are overwritten by instanced attributes are committed to the device buffers. Skipping this commit requires knowing which attributes in the geometry attribute set are always overwritten by instanced attribute sets, requiring extra management logic.

Changes

  • Added a vector of attribute sets at the scene level which can be referenced by objects
  • Changed Geometry::device_update to combine geometry and instanced attribute sets
  • find_attribute uses the per object instance_index to index the attribute set
  • The kernel object for point clouds has switched to using numverts, rather than numkeys to match the interpolation
    ATTR_ELEMENT_VERTEX

Test
Here is a test file
perpoint_attr.zip

image

@dedoardo dedoardo added the enhancement New feature or request label Jul 19, 2021
@dedoardo dedoardo self-assigned this Jul 19, 2021
@dedoardo dedoardo changed the title Instanced attributes sets Instanced attribute sets Jul 19, 2021
@dedoardo dedoardo mentioned this pull request Jul 20, 2021
Open
@dedoardo dedoardo linked an issue Jul 20, 2021 that may be closed by this pull request
Constant attributes on instanced objects for shader access #37
Open
bareya
bareya reviewed Jul 21, 2021
View reviewed changes
src/kernel/kernel_types.h
@@ -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.

src/render/attribute.h Outdated
@@ -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.

src/render/attribute.h Outdated
@@ -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.

src/render/geometry.h Outdated Show resolved Hide resolved
src/render/object.cpp Outdated Show resolved Hide resolved
src/render/instance_group.h Outdated Show resolved Hide resolved
@dedoardo
Copy link
Contributor Author

Hi, thanks a lot for the review @bareya and @skwerner for the bugfix :) I made the following changes

  • Removed double initialization of InstanceGroup::attr_map_offset
  • Moved Attribute::instances to a private block and changed the type to size_t to be consistent with other sizes
  • Made Geometry::num_points virtual. There should probably be a simpler way to pass numverts/numpoints specific to each geometry and avoid duplication.
  • More descriptive comments for internal variables

Maybe Attribute::element_size should include the number of instances. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bareya bareya bareya left review comments

@sergeneren sergeneren Awaiting requested review from sergeneren

@skwerner skwerner Awaiting requested review from skwerner

Assignees

@dedoardo dedoardo

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Constant attributes on instanced objects for shader access
3 participants
@dedoardo @bareya @skwerner