⚙ D1120 Normal Map node support for GLSL mode and the internal render (multiple tangents support)

Campbell Barton (campbellbarton) added inline comments.Jan 28 2016, 2:03 PM

source/blender/blenkernel/intern/DerivedMesh.c
3244–3245	Note: calculating tangents can be quite a heavy operation, not essential but this is a candidate for threading, first allocate all needed layers - then use BLI_task for threading.
3281–3299	Theres no need to re-calculate this for every tangent layer. But to keep code simple, I think its best if `face_as_quad_map` is declared above the tangent-layer loop. Then we can simple check if its non-null - so as only to calculate once. Then move the free outside the tangent loop too.
source/blender/blenkernel/intern/customdata.c
1666	For consistency: `CustomData_check_layer_index_n` -> `CustomData_has_layer_index_n`

This revision now requires changes to proceed.Jan 28 2016, 2:03 PM

Tangent calculation for editmesh (single thread).

Multi-threaded tangent calculation

Campbell Barton (campbellbarton) commandeered this revision.Feb 5 2016, 3:28 PM

Campbell Barton (campbellbarton) edited reviewers, added: Alexander Romanov (a.romanov); removed: Campbell Barton (campbellbarton).

Minor improvements

Alexander Romanov (a.romanov) commandeered this revision.Feb 5 2016, 4:14 PM

Alexander Romanov (a.romanov) edited reviewers, added: Campbell Barton (campbellbarton); removed: Alexander Romanov (a.romanov).

Campbell Barton (campbellbarton) added inline comments.Feb 5 2016, 4:15 PM

source/blender/blenkernel/intern/DerivedMesh.c
3253–3265	None of these `get` functions can run in the threading function, since they aren't threadsafe (can allocate and fill in arrays in the `dm`).

Campbell Barton (campbellbarton) added inline comments.Feb 5 2016, 4:26 PM

source/blender/blenkernel/intern/DerivedMesh.c
3243	can be static function.
3366–3371	While this works, think its a bit fragile. you can forget to call one, and then obscure threading bugs get in. Think its best to just assign arrays to the state, struct ThreadedTangentCalcState { /* shared, const variables / const mvert; const mpoly; ... loop looptri etc. struct { int uv_layer_index; int num_face_as_quad_map; int face_as_quad_map; } layer_data[MAX_MTFACE]; };

Campbell Barton (campbellbarton) added inline comments.Feb 5 2016, 4:28 PM

source/blender/blenkernel/intern/DerivedMesh.c

3366–3371

Correction

struct ThreadedTangentCalcState {   
    /* shared, const variables */
    const *mvert;
    const *mpoly; ... loop looptri etc.
    int num_face_as_quad_map;
    int *face_as_quad_map;

    int uv_layer_index[MAX_MTFACE];  /* use threadid for lookups */
};

Probably worth avoiding using threading entirely when theres only one UV layer or there are very few faces.

Used existing *MeshToTangent* structure for thread state. + Some minor changes.

cast to non-const for mul_mat3_m4_v3

Alexander Romanov (a.romanov) marked 6 inline comments as done.Feb 8 2016, 10:54 AM

Alexander Romanov (a.romanov) added inline comments.

source/blender/blenkernel/intern/DerivedMesh.c
3366–3371	Now I use existing structure for thread state.

Campbell Barton (campbellbarton) added inline comments.Feb 8 2016, 1:26 PM

source/blender/blenkernel/intern/DerivedMesh.c
3323–3335	These can be shared for all threads. as suggested in D1120#39919

Campbell Barton (campbellbarton) added inline comments.Feb 8 2016, 1:36 PM

source/blender/blenkernel/BKE_DerivedMesh.h
784	Looks like this is runtime only (never saved to disk)... Good to say in comments. Also not sure we really need all the `DM_ADD_TANGENT_LAYER_0..7`, Could just comment first 8 bits are for tangent layer bitfield.
source/blender/nodes/shader/nodes/node_shader_normal_map.c
30	not really a fan of putting any flow control inside macros (unless it can't be avoided). Would rather just do this without the macro... or... if (r == NULL) goto fail;

Alexander Romanov (a.romanov) added inline comments.Feb 8 2016, 1:55 PM

source/blender/blenkernel/intern/DerivedMesh.c
3323–3335	I transfer this structure into genTangSpaceDefault without any modifications. In other way we have to copy data in thread function. Now we just store mesh2tangent pointer into SMikkTSpaceContext

Nice progress on the patch!

However, i'm still not a fan of NodeShaderNormalMap::uv_index. It's totally weak and fragile approach. You can have linked mesh with a local material, and in this case you can't reliably keep index in sync. Additionally, same shader could be re-used by multiple meshes and then it's not clear at all what exact uv_index will be cached in the node.

source/blender/editors/space_node/drawnode.c
1023	That is real weak to synchronize on draw. And it's also a performance costly.
source/blender/render/intern/include/renderdatabase.h
79	Guess that's actually `MAX_MTFACE` ?

This revision now requires changes to proceed.Feb 10 2016, 10:26 AM

No more uv_index. All mapping implemented using names.

minor changes

Updated for the current master branch

Hugo Barreira (hbar) added a subscriber: Hugo Barreira (hbar).Mar 16 2016, 1:00 AM

Hey guys!
On the eve of the 2.8 branch with total removal of the code to be refactored, I would like to finish this patch and push it into the master. I just want to increase the probability that this functionality will be in both branches.
The last update was about identifying the tangent layer. In particular I would like to see @Sergey Sharybin (sergey) comments because of he marked the patch. Thanks in advance.

I only checked the Blender Internal and GLSL shader code in detail.

source/blender/blenkernel/intern/editderivedmesh.c
482	This can use `tangent_names[MAX_TFACE][MAX_NAME]`, only reason to used number is for DNA headers where our parser is not smart enough.
source/blender/gpu/shaders/gpu_shader_material.glsl
381	This should be `strength = clamp(strength, 0.0, 1.0)`, now it's a no-op.
382	I'm not sure if this is still something that matters, but I remember issues with using integer constants instead of float constants like `1.0` in GLSL.
source/blender/makesrna/intern/rna_nodetree.c
4216	Why is this being removed?
source/blender/nodes/shader/nodes/node_shader_normal_map.c
51	Should be `float vecIn[3]`.
52	Should be `float strength;`
64	Should be `nodestack_get_vec(&strength, SOCK_FLOAT, in[0]);`
67–71	This is assuming that the image texture node will convert from sRGB to linear RGB and that as a result the conversion must be undone. This will not work in a Cycles shader node setup, where the image texture node is supposed to be configured to non-color data. Then undoing an operation which was not done will lead to wrong results. I think Blender images should be expected to be configured as non-color data too, and this code should be removed.
95–98	Slightly simpler logic is to initialize `int uv_index = 0` above and leave out these 4 lines. `shi->totuv` will never be >= 8 so no need to check that.
102–103	This makes no sense to me, why would `shi->do_preview` affect the tangent?
109	Would be more clear to use `interp_v3_v3v3()` here.
115–116	This can be simplified to `interp_v3_v3(out[0]->vec, N, vecIn);`
122–123	This can be simplified to `interp_v3_v3(out[0]->vec, N, vecIn);`
146–147	Same comments as the Blender Internal node implementation.
148	I don't know why we `goto` here instead of doing `return r;` directly. Why check the return value of `GPU_link` at all? None of the other GLSL generation code does, it should never fail if the code is working correctly.
source/blender/render/intern/source/convertblender.c
3121	`concreet` => `concrete` Though perhaps more clear naming would be `need_named_nmap_tangent`? I'm not sure what "concrete" means in this context.

This revision now requires changes to proceed.Apr 14 2016, 12:00 AM

Alexander Romanov (a.romanov) updated this revision to Diff 6430.Apr 15 2016, 11:09 AM

Alexander Romanov (a.romanov) edited edge metadata.

Alexander Romanov (a.romanov) marked 16 inline comments as done.Apr 15 2016, 11:24 AM

Alexander Romanov (a.romanov) added inline comments.

source/blender/gpu/shaders/gpu_shader_material.glsl
381	To match Cycles, no clamping is needed
source/blender/makesrna/intern/rna_nodetree.c
8	Sorry, it's just a accidental mistake
source/blender/render/intern/source/convertblender.c
127	concrete == specific, is opposition to "active" or "render"

Brecht Van Lommel (brecht) mentioned this in T47947: Normal maps use UV normals of selected UV map, not specified UV map, in 3DView 'material' drawing mode.Apr 23 2016, 3:31 AM

Campbell Barton (campbellbarton) retitled this revision from Normal Map node support for GLSL mode and the internal render to Normal Map node support for GLSL mode and the internal render (multiple tangents support).Apr 26 2016, 10:36 AM

Campbell Barton (campbellbarton) edited edge metadata.

Campbell Barton (campbellbarton) commandeered this revision.Apr 26 2016, 10:39 AM

Campbell Barton (campbellbarton) edited reviewers, added: Alexander Romanov (a.romanov); removed: Campbell Barton (campbellbarton).

Sergey Sharybin (sergey) mentioned this in T48249: Multiple UVs tangents bug..Apr 26 2016, 11:16 AM

Cleanup: minor edits, code style

Remove CustomData_has_layer_index_n, just loop over the total number of layers

Committed rB5abae51a6ef5b0f1b817ef5ce4bff34fef5001cd

Alexander Romanov (a.romanov) mentioned this in T48453: Fix: wrong material preview colors for the cycles normalmap node.May 17 2016, 1:27 PM

Alexander Romanov (a.romanov) mentioned this in rB5494ea1d084a: Fix own mistake in D1120: wrong indexing of UV maps during tangent calculation.May 24 2016, 4:55 PM

Jeroen Bakker (jbakker) mentioned this in rB17f716be679a: Fix own mistake in D1120: wrong indexing of UV maps during tangent calculation.Jun 8 2016, 9:49 PM

Alexander Romanov (a.romanov) mentioned this in rB4b0aeaf327d3: Fix T48757: Broken in D1120 normal baking.Jun 30 2016, 11:40 AM

Normal Map node support for GLSL mode and the internal render (multiple tangents support)
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 6132

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_DerivedMesh.h

source/blender/blenkernel/BKE_customdata.h

source/blender/blenkernel/BKE_mesh.h

source/blender/blenkernel/intern/DerivedMesh.c

source/blender/blenkernel/intern/cdderivedmesh.c

source/blender/blenkernel/intern/customdata.c

source/blender/blenkernel/intern/editderivedmesh.c

source/blender/blenkernel/intern/material.c

source/blender/blenkernel/intern/mesh_evaluate.c

source/blender/blenkernel/intern/subsurf_ccg.c

source/blender/editors/space_node/drawnode.c

source/blender/gpu/shaders/gpu_shader_material.glsl

source/blender/makesdna/DNA_material_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/shader/nodes/node_shader_normal_map.c

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

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

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

source/blender/render/intern/source/bake_api.c

source/blender/render/intern/source/convertblender.c

source/blender/render/intern/source/multires_bake.c

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

source/blender/render/intern/source/shadeinput.c

source/gameengine/Converter/BL_BlenderDataConversion.cpp

Normal Map node support for GLSL mode and the internal render (multiple tangents support)AbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 6132

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_DerivedMesh.h

source/blender/blenkernel/BKE_customdata.h

source/blender/blenkernel/BKE_mesh.h

source/blender/blenkernel/intern/DerivedMesh.c

source/blender/blenkernel/intern/cdderivedmesh.c

source/blender/blenkernel/intern/customdata.c

source/blender/blenkernel/intern/editderivedmesh.c

source/blender/blenkernel/intern/material.c

source/blender/blenkernel/intern/mesh_evaluate.c

source/blender/blenkernel/intern/subsurf_ccg.c

source/blender/editors/space_node/drawnode.c

source/blender/gpu/shaders/gpu_shader_material.glsl

source/blender/makesdna/DNA_material_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/shader/nodes/node_shader_normal_map.c

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

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

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

source/blender/render/intern/source/bake_api.c

source/blender/render/intern/source/convertblender.c

source/blender/render/intern/source/multires_bake.c

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

source/blender/render/intern/source/shadeinput.c

source/gameengine/Converter/BL_BlenderDataConversion.cpp

Normal Map node support for GLSL mode and the internal render (multiple tangents support)
AbandonedPublic
Actions

Revision Contents
Changeset List