Details

Reviewers

Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)
Clément Foucault (fclem)
Mike Erwin (merwin)

Commits

rBS2a489273d7e2: Implement Uniformbuffer objects for nodetree parameters
rB2a489273d7e2: Implement Uniformbuffer objects for nodetree parameters

Summary

For users that means you can tweak shaders in the nodetree and there is
no more significant lag. This is a huge improvement, particularly in
systems that have no shader cache.

From the code perspective it means we are no longer re-compiling the
shader every time a value is tweaked in the UI. We are using uniforms
for those values.

It would be slow to add that many uniforms for all the shaders. So
instead we are using UBO (Uniform Buffer Objects).

The following patch is updating only a few nodes. Once the design is
approved the remaining nodes will be tackled.

This fixes the main issue of T51467. However GWN_shaderinterface_create() still
needs to be improvedi. When opening a .blend all shaders are compiled once, so
optimizing it will bring a measurable impact.

This should be enough for testing and evaluating the API:

Mix RGB
RGB
Vale
Emission
Eevee Metallic

Diff Detail

Repository

rB Blender

Branch

temp-shaders-ubo-merge

Build Status

Buildable 713
Build 713: arc lint + arc unit

Event Timeline

Dalai Felinto (dfelinto) created this revision.Jul 11 2017, 4:58 PM

Cleanup the patch

Convert all the BSDF nodes

Harbormaster completed remote builds in B715: Diff 9002.Jul 11 2017, 6:23 PM

Small todos:

This broke Cycles updates. I need to investigate what was being called before that was triggering updates in Cycles.
When updating world nodes, the world probe needs to be re-generated. At the moment it's not, so object material shades are not updated in relation to the world.
Convert missing nodes.

I'm still unsure about putting all this logic in GPUUniformBuffer and using two distinct buffer type. At least merging *data and flag into the GPUUniformBuffer and put all this sorting logic in gpu_material.c. But this means doing the update for all material that changes even thoses not displayed.

But doing the update at binding time (as it is in this patch) may be preferable since only needed UBOs would be updated. Still it would introduce a stall because directly needed by the next drawcall.

I don't have a better solution than the one in the patch so I'll live with this.

The Principled BSDF is buggy here. Some sliders are not working or not doing what they should.

source/blender/draw/intern/draw_manager.c
247	Not needed anymore.
source/blender/gpu/intern/gpu_codegen.c
608	You don't need the uniform identifier here.
1934	Can't we use bNodeSocket->type for this instead of passing the gputype and potentially have errors?
source/blender/gpu/intern/gpu_uniformbuffer.c
63	I'm kindof not sure this belongs here. Since it will be mainly (only) used by the material system I would prefer to put this in the GPUMaterial instead. Same for all associated function bellow.
201	Why do you free knowing when the UBO size cannot change?
212	This could go to the static ubo as well.

This revision now requires changes to proceed.Jul 12 2017, 5:01 PM

Brecht Van Lommel (brecht) requested changes to this revision.Jul 12 2017, 5:17 PM

Brecht Van Lommel (brecht) added inline comments.

source/blender/makesrna/intern/rna_nodetree.c
2283–2284	This doesn't take into account node groups. There is a utility function somewhere to iterate over all shading node trees. This should also be done only if ntree is a shading node tree.
2320	This notifier is being run also for e.g. compositing nodes which it shouldn't. I think should be more fine grained in any case, the 3D view only needs to be redrawn if there is an actual gpumaterial being changed, and I would expect we already had some other notifier for that?
source/blender/nodes/shader/node_shader_tree.c
505–510	This function name doesn't describe what it does differently than `ntreeGPUMaterialNodes`. It skips the localization, that is used to modify the nodetree to support reroute, mute and bump. So those are broken now or did I miss something?
source/blender/nodes/shader/nodes/node_shader_ambient_occlusion.c
44 ↗	(On Diff #9002)	Can't we handle all sockets automatically in `GPU_stack_link`? That's the purpose of that function, to avoid having to link each socket manually.

Steffen Dünner (SteffenD) added a subscriber: Steffen Dünner (SteffenD).Jul 12 2017, 5:36 PM

From review: No need to store UBO in DRWShadingGroup
From review: No need of "uniform" identifier inside UBO block
From review: Use socket->type instead of gputype
From review: No need to clear the data when updating UBO
From review: Use common functions for dynamic and static UBOs

Patch updated addressing most of @Clément Foucault (fclem) requests. I will tackle Brecht's next.

@Brecht Van Lommel (brecht):

It skips the localization, that is used to modify the nodetree to support reroute, mute and bump. So those are broken now or did I miss something?

You are right. I was talking with @Sergey Sharybin (sergey) yesterday about that. I will look at fixing this elsewhere, without requiring operating in a local copy of the tree.

source/blender/gpu/intern/gpu_uniformbuffer.c
201	Originally I was populating the items individually, but it makes no sense now indeed.

From review: fix world probe and better notifier and FOREACH_NODETREE
From review: Make it more clear what the ntreeGPUMaterialNodes functions do
From review: use GPU_stack_link

Finished the patch as far as Eevee is concerned. The remaining issues are Cycles related:

Tag node tree to update cycles rendering [showstopper]
Support bumpmap/displacement trick

source/blender/makesrna/intern/rna_nodetree.c
2283–2284	Done, and done.

Looks good so far. Reroute and mute is still broken for Eevee as well, but should be solved if there is some solution to the localization for Cycles displacement.

I think node->original could be set and read to get the pointer to the original non-localized socket value.

From review: Use node->original to get the sockets pointers

Clément Foucault (fclem) accepted this revision.Jul 14 2017, 12:39 PM

I talked to @Sergey Sharybin (sergey) he is ok with this patch being merged as it is, and then he takes care of the depsgraph/cycles missing update.

Yes, let's not delay things for too long. Easier to fix missing bits once the code is in.

Closed by commit rB2a489273d7e2: Implement Uniformbuffer objects for nodetree parameters (authored by Dalai Felinto (dfelinto)). · Explain WhyJul 14 2017, 5:47 PM

This revision was automatically updated to reflect the committed changes.

Implement Uniformbuffer objects for nodetree parameters
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 9000

source/blender/blenkernel/BKE_node.h

source/blender/draw/intern/draw_manager.c

source/blender/gpu/GPU_material.h

source/blender/gpu/GPU_uniformbuffer.h

source/blender/gpu/intern/gpu_codegen.h

source/blender/gpu/intern/gpu_codegen.c

source/blender/gpu/intern/gpu_material.c

source/blender/gpu/intern/gpu_shader.c

source/blender/gpu/intern/gpu_uniformbuffer.c

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/shader/node_shader_tree.c

source/blender/nodes/shader/node_shader_util.h

source/blender/nodes/shader/nodes/node_shader_eevee_metallic.c

source/blender/nodes/shader/nodes/node_shader_emission.c

source/blender/nodes/shader/nodes/node_shader_mixRgb.c

source/blender/nodes/shader/nodes/node_shader_rgb.c

source/blender/nodes/shader/nodes/node_shader_value.c

Implement Uniformbuffer objects for nodetree parametersClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 9000

source/blender/blenkernel/BKE_node.h

source/blender/draw/intern/draw_manager.c

source/blender/gpu/GPU_material.h

source/blender/gpu/GPU_uniformbuffer.h

source/blender/gpu/intern/gpu_codegen.h

source/blender/gpu/intern/gpu_codegen.c

source/blender/gpu/intern/gpu_material.c

source/blender/gpu/intern/gpu_shader.c

source/blender/gpu/intern/gpu_uniformbuffer.c

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/shader/node_shader_tree.c

source/blender/nodes/shader/node_shader_util.h

source/blender/nodes/shader/nodes/node_shader_eevee_metallic.c

source/blender/nodes/shader/nodes/node_shader_emission.c

source/blender/nodes/shader/nodes/node_shader_mixRgb.c

source/blender/nodes/shader/nodes/node_shader_rgb.c

source/blender/nodes/shader/nodes/node_shader_value.c

Implement Uniformbuffer objects for nodetree parameters
ClosedPublic
Actions

Revision Contents
Changeset List