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)
Details
Details
This patch implements the Vector Transform node for GLSL mode and the internal renderer.
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- arcpatch-D909_3
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
- Updated for current master
- glsl names fixed
- BGE support (right view matrix in BL_BlenderShader.cpp: GPU_material_bind_uniforms params)
Ready for review.
Comment Actions
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 | |
Comment Actions
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.
Comment Actions
Adding some more eyes here since Antony is temporary unavailable for the reviews. Will have a closer look a bit later.
Comment Actions
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. | |
Comment Actions
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. | |
Comment Actions
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. | |
Comment Actions
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, | |
Comment Actions
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 | |
| 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? | |
Comment Actions
I've marked this as accepted, though since this isn't code I maintain, best have @Sergey Sharybin (sergey) / @Antonis Ryakiotakis (psy-fi) or @Brecht Van Lommel (brecht) do final sign-off before going into master.
Comment Actions
@Sergey Sharybin (sergey) checked this patch. Since the last review by @Antonis Ryakiotakis (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. | |
Comment Actions
'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).