Page MenuHome

Materials: Add custom object properties as usable attributes
ClosedPublic

Authored by Alexander Gavrilov (angavrilov) on Jun 17 2016, 3:35 AM.
Tags
None
Tokens
"Like" token, awarded by bonalex01."Love" token, awarded by lopoIsaac."Like" token, awarded by hitrpr."Like" token, awarded by spiv."Love" token, awarded by blueprintrandom."Love" token, awarded by Shimoon."Like" token, awarded by IPv6."Like" token, awarded by EAW."Love" token, awarded by Rusculleda."Love" token, awarded by yodus."Love" token, awarded by angelo-lexset."Love" token, awarded by FrandSX."Love" token, awarded by rotoglup."Love" token, awarded by ramification."Love" token, awarded by ogierm."Love" token, awarded by cerebral_malfunction."Love" token, awarded by ugosantana."Burninate" token, awarded by lordodin."Love" token, awarded by monio."Love" token, awarded by bliblubli.

Details

Summary

Up until now, materials only had access to constants stored in the material itself, or attributes associated with the mesh vertices. However, in certain cases it is convenient to provide parameters associated with individual objects that use that material. This patch addresses that use case by adding two connected features to the Attribute node.

Access to the Alpha Channel

A new 'Alpha' output socket is added to the Attribute node, which returns the fourth channel of the attribute if available.

Currently there already is a four channel attribute (vertex color), but the alpha channel is only accessible through the dedicated Vertex Color node. This extends the generic attribute node to handle that case. If the attribute has fewer than 4 channels, the 'alpha' value is not well specified, but generally seems to be 1.

Object and Instance Attributes

As the main feature, a new dropdown allows switching the node from accessing Geometry attributes to the Object or Instance mode. In those modes the attribute name is interpreted as the name of a custom property, or a generic RNA path like the one produced by Copy Data Path.

The Object mode searches for the property in the object and its data ID in that order. The Instance mode additionally looks first in the particle system settings and the instance parent object if the current object was instanced.

This feature supports properties that are integer, float, or a float array of up to 4 components. The values are appropriately padded to have 4 channels, and the alpha channel is guaranteed to default to 1 if the property exists, or 0 if it was not found.

Techical

The primary reason for introducing the Geometry/Object/Instance dropdown is that without a way to distingush varying and uniform attributes when the material is compiled it would be very difficult to implement this for Eevee. Eevee is also limited to only 8 such attributes per material because of hardware limitations on UBO size.

Cycles seems to be designed with a common namespace for all attributes in mind, so the cycles-blender interface translates attribute types by internally adding a name prefix.

Links

The code is available as individual commits at: https://developer.blender.org/diffusion/BS/history/temp-angavrilov-material-uniforms/

Test file (updated):

Diff Detail

Repository
rB Blender
Branch
temp-angavrilov-material-uniforms (branched from master)
Build Status
Buildable 9915
Build 9915: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Now I understand the DRW part better. I don't have any issue with the implementation details. It's mostly code style issues / lack of documentation.

After theses are fixed, it's a greenlight for me.

source/blender/draw/intern/draw_manager_data.c
71

Why not use a BLI_bitmap?

192

I think this code is better suited to be put inside draw_instance_data.c, which already does something similar.

195

It's a DRW structure. Use DRW prefix. Also Same thing as the GPU struct, could abreviate to DRWUniformAttrBuf (naming style of the GPU module).

196

This really lack some comment on the data structure. Please add some comment about the whole struct and each members.

199

Why is the next pointer at the end of the struct? Is this a linklist?

Ok I just saw this is used only when deleting the buffers. Then put it in a comment or/and rename the pointer next_orphan.

202

I don't like having this type here. Maybe just use BLI_math_vector.h functions with a float[4] for readability and code consistency.

source/blender/gpu/GPU_material.h
256

I'm starting to think the name is a bit too verbose but that's a bit personal as I tend to like shorter names.

The fact we have GPUMaterialTexture is to avoid mixing with the GPUTexture.

I would prefer GPUUniformAttr or at least GPUMaterialUniformAttr (attr is ubiquitous in the codebase).

Same for GPUMaterialUniformAttributeSet which could be rename to GPUUniformAttrList.

source/blender/gpu/GPU_viewport.h
61

use plural and specify it contains ubos. The container type can be viewed in other ways.

source/blender/gpu/intern/gpu_node_graph.c
385

It's not obvious that this function can return NULL. Add a comment on top of it's declaration.

502

Add a comment that says that this is the case when there is not enough GPU slots.

503

Nitpick: Isn't Attribs supposed to be at max a vec4?

This revision now requires changes to proceed.Sep 17 2020, 10:32 PM
Alexander Gavrilov (angavrilov) marked 15 inline comments as done.Sep 19 2020, 6:24 PM
Alexander Gavrilov (angavrilov) added inline comments.
source/blender/gpu/intern/gpu_node_graph.c
503

Overallocating is safer than underallocating ;)

I won't be able to review it properly in the next two weeks. So I'm tagging @Jeroen Bakker (jbakker) as reviewer instead of me to avoid stalling the review.

In EEVEE, when using a float property the data is send to the GPU as (prop_value, prop_value, prop_value, 1) when read in the node_attribute it is averaged so it will be (prop_value * 3.0 + 1.0) / 4.0 this isn't what the user expects. Might be related to the change of how Alpha values are handled.

Fixed attribute float output.

The main risk for Cycles here is GPU rendering performance. For example an additional call to kernel_tex_image_interp_3d can a have a big impact on OpenCL rendering AMD which may inline the entire function, and slow down other code.

We'll need to run benchmarks here. Have you tried a basic performance comparison with CUDA or OpenCL in a scene like bmw?

If there is a big performance impact, refactoring the code so that there are no separate primitive_attribute functions per data type may help.

intern/cycles/blender/blender_object.cpp
361

I think this should default to all zero.

For 3-channel colors it can set alpha to 1, but I would not do this for arbitrary 4D float arrays.

402–406

This can be wrapped in a AttributeRequestSet requests = object->needed_attributes() function, next to the need_attribute function.

We may need to add additional logic here besides just looping over shaders, and I want to keep that abstracted away like it is for need_attribute.

420–421

Rather than a prefix for the name, there should be an additional field in AttributeRequest.

I don't really see a reason to use special prefixes here.

intern/cycles/render/geometry.cpp
354–366

Overriding should be the other way around, that's how it works in other renderers and 3D software that I'm aware of and so will be better for interop.

For example a per curve point radius on hair should override a radius per curve, which should override a radius for the entire object.

401–405

Can you add a comment explaining the chain link thing?

intern/cycles/render/geometry.h
95

Index into what? This needs a better name/description.

intern/cycles/render/object.h
57

This should be a protected member, not mixed with the public members.

source/blender/blenlib/BLI_bitmap.h
109 ↗(On Diff #29123)

This seems like something that can be committed already if it's required.

source/blender/makesrna/intern/rna_nodetree.c
4402–4404

I'd rename this to "Instancer", for consistency with the "From Instancer" option in the texture coordinate node.

"Instance" implies to me some different value per instance, not a shared value for all objects instanced by an instancer.

4406

caused the instancing -> instanced it

Brecht Van Lommel (brecht) requested changes to this revision.Sep 23 2020, 6:48 PM
This revision now requires changes to proceed.Sep 23 2020, 6:48 PM
intern/cycles/blender/blender_object.cpp
361

I completely disagree - defaulting the alpha to 1 for existing attributes, and 0 for missing ones allows detecting if the property was found. Also, for all varying attributes alpha already defaults to 1, and has to be that way due to how it works in eevee, which apparently has basis in OpenGL itself.

420–421

The reason for prefixes is that the uniform attributes form a completely separate namespace in Blender with a dropdown in the node (required by Eevee), but Cycles already has all attributes in a single namespace looked up only by name. This information must be part of the attribute key, i.e. attribute "foo" with types Geometry, Object and Instance are all separate attributes, which can be used simultaneously by the same material shader.

intern/cycles/render/geometry.cpp
354–366

This would be impossible to achieve in the SVM case without either completely duplicating the mesh attribute maps, scanning through the whole list until the end, or introducing a separate attribute map index for objects instead of linking. The way I implemented is that object maps only contain object attributes, and then link to the mesh maps.

401–405

It's simple: to avoid duplicating the mesh maps for each objects with object attributes, maps now can link to other maps like a linked list, so object maps only have object attributes, and then link to the mesh map. Since this is your code originally, if you have a better implementation idea I'd gladly hear it.

intern/cycles/render/geometry.h
95

Index into mesh vector? It's used to access the temporary attribute data arrays by index when all you have is a mesh pointer.

source/blender/blenlib/BLI_bitmap.h
109 ↗(On Diff #29123)

This makes sense since all other allocation functions for bitmap use calloc, and the DRWSparseUniformBuffer added in this patch depends on this.

Updated to address feedback.

Fixed a few missed places that should now use NODE_ATTR_OUTPUT_FLOAT3 (broke all tests)

The main risk for Cycles here is GPU rendering performance. For example an additional call to kernel_tex_image_interp_3d can a have a big impact on OpenCL rendering AMD which may inline the entire function, and slow down other code.
We'll need to run benchmarks here. Have you tried a basic performance comparison with CUDA or OpenCL in a scene like bmw?

Just checked with CUDA on BMW and there seems to be no discernible difference. Can't test OpenCL because blender says it's not supported with my gpu (maybe actually I'm missing some library package, but I have no idea).

I made a branch build, so maybe somebody else can test too: https://builder.blender.org/admin/#/builders/23/builds/45

I tested the test file of this diff on OpenCL and seems to be working fine.

I tested the test file of this diff on OpenCL and seems to be working fine.

What about performance? I tested that there is no observable decrease in Cycles rendering speed on BMW with CUDA. Brecht raised a concern that adding more kernel code could affect things even when it's not called.

Committed the Cycles internal changes to master, rebasing patch on them.

From my testing there should be no performance regressions in GPU rendering.

Approving the Cycles and shader UI part of this. Have not looked at the Eevee implementation.

When this gets committed, be sure to add a test to the regression test files.

Approving the Cycles and shader UI part of this. Have not looked at the Eevee implementation.

When this gets committed, be sure to add a test to the regression test files.

Question, when this gets committed, can we revert the fix for: https://developer.blender.org/D9322 ?

Question, when this gets committed, can we revert the fix for: https://developer.blender.org/D9322?

D9322 is not related to this patch. There is another workaround in the multithreaded export related to dupli particles. For that I plan to implement a better solution using object attributes to store per-instance particle info. But it's not there yet.

Fixed a race condition caused by the recent addition of threaded geometry update.

@Jeroen Bakker (jbakker) @Clément Foucault (fclem) So what is the conclusion on the Eevee part of the patch?

@Brecht Van Lommel (brecht) Created a quite thorough test file, will commit when the patch is in. Does the used_shaders race condition fix look OK?

All the outstanding issues have been addressed for the EEVEE/viewport part and have approved the patch.

This revision is now accepted and ready to land.Mon, Nov 2, 1:17 PM