Page MenuHome

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

Authored by Campbell Barton (campbellbarton) on Feb 17 2015, 9:48 AM.

Details

Summary

The Normal Map node is a useful node which is present in the Cycles render. It makes it possible to use normal mapping without additional Material node in a node tree.
This patch implements Normal Map node for GLSL mode and the internal render.

Example:

Alexander (Blend4Web Team)

Diff Detail

Repository
rB Blender
Branch
arcpatch-D1120

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
Alexander Romanov (a.romanov) updated this revision to Diff 5946.

Tangent calculation for editmesh (single thread).

Alexander Romanov (a.romanov) updated this revision to Diff 5967.

Multi-threaded tangent calculation

source/blender/blenkernel/intern/DerivedMesh.c
3215–3227

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).

source/blender/blenkernel/intern/DerivedMesh.c
3247

can be static function.

3302–3307

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];
};
source/blender/blenkernel/intern/DerivedMesh.c
3302–3307

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.

Alexander Romanov (a.romanov) removed rB Blender as the repository for this revision.
Alexander Romanov (a.romanov) updated this revision to Diff 6008.

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

Alexander Romanov (a.romanov) marked 11 inline comments as done.Feb 8 2016, 10:50 AM
Alexander Romanov (a.romanov) updated this revision to Diff 6009.
  • 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.

source/blender/blenkernel/intern/DerivedMesh.c
3311–3323

These can be shared for all threads. as suggested in D1120#39919

source/blender/blenkernel/BKE_DerivedMesh.h
787–800

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;
source/blender/blenkernel/intern/DerivedMesh.c
3311–3323

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

Sergey Sharybin (sergey) requested changes to this revision.

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 ↗(On Diff #6009)

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
Alexander Romanov (a.romanov) removed rB Blender as the repository for this revision.
Alexander Romanov (a.romanov) updated this revision to Diff 6132.

No more uv_index. All mapping implemented using names.

Alexander Romanov (a.romanov) marked 7 inline comments as done.
Alexander Romanov (a.romanov) updated this revision to Diff 6133.

minor changes

Alexander Romanov (a.romanov) marked 2 inline comments as done.Feb 18 2016, 5:16 PM

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.

Brecht Van Lommel (brecht) requested changes to this revision.

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 ↗(On Diff #6259)

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) 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 ↗(On Diff #6259)

Sorry, it's just a accidental mistake

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

concrete == specific, is opposition to "active" or "render"

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.
  • Cleanup: minor edits, code style
  • Remove CustomData_has_layer_index_n, just loop over the total number of layers