Vector Transform node support for GLSL mode and the internal renderer
ClosedPublic

Authored by Alexander Romanov (a.romanov) on Nov 21 2014, 5:08 PM.

Details

Summary

The Vector Transform node is a useful node which is present in the Cycles renderer.


This patch implements the Vector Transform node for GLSL mode and the internal renderer.

Example:

Alexander (Blend4Web Team)

Diff Detail

Repository
rB Blender
There are a very large number of changes, so older changes are hidden. Show Older Changes
  1. Updated for current master
  2. glsl names fixed
  3. BGE support (right view matrix in BL_BlenderShader.cpp: GPU_material_bind_uniforms params)

Ready for review.

Updated for the actual master.

Updated for the actual master.

updated for actual master

Generally doesn't look bad to me apart from the formatting issues.

I would love to avoid adding view matrices to ShadeInput. It can be done if you pass Render to ntreeShaderExecTree and copy to ShaderCallData but might cause concurrency issues? @Sergey Sharybin (sergey)? What would you say?

source/blender/editors/render/render_internal.c
1304

style

source/blender/gpu/shaders/gpu_shader_material.glsl
156

vin/vout would be good here as well.

source/blender/nodes/shader/nodes/node_shader_vectTransform.c
57

indentation

123

indentation

source/blender/render/intern/include/render_types.h
356

Yes, because there may be many transform nodes in the graph

source/blender/render/intern/source/renderdatabase.c
1371

style

Alright, discussed with @Sergey Sharybin (sergey) and we agree that it's better to pass Render to ntreeShaderExecTree and copy to ShaderCallData. That way you can avoid duplicating the matrix in ShadeInput.

Store Render pointer in ShaderCallData; style correction

Update for actual master and RE_updateRenderInstances call fix

Update for the actual master.

Adding some more eyes here since Antony is temporary unavailable for the reviews. Will have a closer look a bit later.

Alexander Romanov (a.romanov) marked 4 inline comments as done.

Updated for the current master. Now it is ready for review.

Alexander Romanov (a.romanov) marked 26 inline comments as done.Jan 11 2016, 10:18 AM

Set the done flag for all comments.

Sergey Sharybin (sergey) requested changes to this revision.Jan 13 2016, 8:33 AM

There're quite some code style issues in the code which were reported but seems not being addressed. Please read the patch carefully and make code style matching our existing one.

Also, an idea. Seems global R is always passed to ntreeShaderExecTree. In this case, can we simply make it RE_render_current_current_get_matrix(int which_matrix) which will get matrix from R? Seems it'll simplify some cases here.

Also, is it really needed to get a pointer to matrix instead of having a copy semantic? If the code is only run on shader compile would prefer to have a copy semantic instead of piercing internal pointers to an external world, especially without any const qualifier.

source/blender/editors/render/render_internal.c
1304

Something is wrong here, either it's not done or the patch is not updated. Should be space between keyword and bracket.

source/blender/nodes/shader/node_shader_tree.c
301 ↗(On Diff #5797)

Picky: Render is kinda more generic, and should go before shi/shr.

source/blender/nodes/shader/nodes/node_shader_vectTransform.c
59

Code style.

79

Same as above.

124

Indentation and whitespace.

source/blender/render/extern/include/RE_pipeline.h
369

Picky: Use braces around value. Also emum is better to be defined before the function declaration.

source/blender/render/extern/include/RE_shader_ext.h
230

Either use copy semantic (copy_m4_m4 to a float[4] result) or use const qualifier here. We don't want those matricies to be modified externally.

237

Same as above.

source/blender/render/intern/include/renderdatabase.h
122

What is flag here?

source/blender/render/intern/source/shadeoutput.c
2135

Indentation.

This revision now requires changes to proceed.Jan 13 2016, 8:33 AM
  • style fix and some refactoring
Alexander Romanov (a.romanov) marked 5 inline comments as done.
  • reduce warnings and minor style fix
Alexander Romanov (a.romanov) marked 15 inline comments as done.
  • style fix

Ready for review

source/blender/editors/render/render_internal.c
1304

Here we are updating some stuff in render instances that depends on the flag. if the view was updated, we must update the relevant matrices that depend on it.

source/blender/render/extern/include/RE_shader_ext.h
230

Here I am using pointers because it will be called for each pixel

source/blender/render/intern/include/renderdatabase.h
122

It is a flag from RE_updateRenderInstances. Specifies what things have changed.

Generally looks fine, with some minor feedback. Maybe @Campbell Barton (campbellbarton) would like to have some final glance over this.

source/blender/nodes/shader/nodes/node_shader_vectTransform.c
55

Please use lowercase for functions in this module.

source/blender/render/intern/source/renderdatabase.c
1368

Style.

1373

Style.

1380

Style.

Alexander Romanov (a.romanov) marked 4 inline comments as done.
  • style fix

Looks like feedback so far has been addressed, I just went over and noted some real minor issues. Otherwise seems this is ready for master.

source/blender/gpu/intern/gpu_material.c
412

convention in our code is (float *)

source/blender/nodes/shader/nodes/node_shader_vectTransform.c
115

can use copy_v4_v4

source/blender/render/intern/source/shadeoutput.c
2133

Name is a bit odd 'current_get_matrix', could just have functions for each of these.

We have for eg, RE_GetViewPlane - possible could add RE_GetViewMatrix, RE_GetViewMatrixInv ? realize naming here is messy.

source/gameengine/Ketsji/BL_BlenderShader.cpp
170

You can avoid cast by doing &viewmat[0][0], same above.

source/blender/nodes/shader/nodes/node_shader_vectTransform.c
109

Am getting error here passing argument 1 of 'mul_m4_v4' discards 'const' qualifier,
rather annoying since its not modified, but we didnt set the args to const here. safe to cast.

Campbell Barton (campbellbarton) requested changes to this revision.Jan 20 2016, 10:34 AM
This revision now requires changes to proceed.Jan 20 2016, 10:34 AM
Alexander Romanov (a.romanov) marked 4 inline comments as done.
  • fix style + avoid type casting

Updated according to last comments

source/blender/render/intern/source/shadeoutput.c
2133

If I remember correctly RE_object_instance_get_matrix and RE_render_current_get_matrix came from separate functions that was suggested by reviewer. Maybe just remove "current", which indicates that the matrix is taken for the current BI renderer? This style of naming was copied from RE_lamp_get_data

Campbell Barton (campbellbarton) added inline comments.
source/blender/render/intern/source/shadeoutput.c
2133

In this context the term 'current' just seems not very descriptive,

If its the current-view then it could be called RE_render_view_get_matrix?

I've marked this as accepted, though since this isn't code I maintain, best have @Sergey Sharybin (sergey) / @Antony Riakiotakis (psy-fi) or @Brecht Van Lommel (brecht) do final sign-off before going into master.

@Sergey Sharybin (sergey) checked this patch. Since the last review by @Antony Riakiotakis (psy-fi) the code got just minor modifications. So I think that it makes no sense to bother the developers anymore.

source/blender/render/intern/source/shadeoutput.c
2133

It use global R that is always one, so it could be used by view or during render with F12.

'arc land --hold' tells me that revision is not accepted. I think there should be no rejection marks. So I will wait for @Sergey Sharybin (sergey).

This revision is now accepted and ready to land.Jan 23 2016, 12:29 PM
This revision was automatically updated to reflect the committed changes.