Page MenuHome

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

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



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.


Alexander (Blend4Web Team)

Diff Detail

rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.


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.


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) edited edge metadata.

Tangent calculation for editmesh (single thread).

Alexander Romanov (a.romanov) edited edge metadata.

Multi-threaded tangent calculation


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


can be static function.


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];


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) edited edge metadata.
Alexander Romanov (a.romanov) removed rB Blender as the repository for this revision.

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

Alexander Romanov (a.romanov) marked 11 inline comments as done.
  • 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.

Now I use existing structure for thread state.


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


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.


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


if (r == NULL)
    goto fail;

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.Feb 10 2016, 10:26 AM
Sergey Sharybin (sergey) edited edge metadata.

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.

1023 ↗(On Diff #6009)

That is real weak to synchronize on draw. And it's also a performance costly.


Guess that's actually MAX_MTFACE ?

This revision now requires changes to proceed.Feb 10 2016, 10:26 AM
Alexander Romanov (a.romanov) edited edge metadata.
Alexander Romanov (a.romanov) removed rB Blender as the repository for this revision.

No more uv_index. All mapping implemented using names.

Alexander Romanov (a.romanov) edited edge metadata.
Alexander Romanov (a.romanov) marked 7 inline comments as done.

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.Apr 14 2016, 12:00 AM
Brecht Van Lommel (brecht) edited edge metadata.

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


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.


This should be strength = clamp(strength, 0.0, 1.0), now it's a no-op.


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.

4216 ↗(On Diff #6259)

Why is this being removed?


Should be float vecIn[3].


Should be float strength;


Should be nodestack_get_vec(&strength, SOCK_FLOAT, in[0]);


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.


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.


This makes no sense to me, why would shi->do_preview affect the tangent?


Would be more clear to use interp_v3_v3v3() here.


This can be simplified to interp_v3_v3(out[0]->vec, N, vecIn);


This can be simplified to interp_v3_v3(out[0]->vec, N, vecIn);


Same comments as the Blender Internal node implementation.


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.


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.

To match Cycles, no clamping is needed

8 ↗(On Diff #6259)

Sorry, it's just a accidental mistake


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.
Campbell Barton (campbellbarton) marked 3 inline comments as done.
  • Cleanup: minor edits, code style
  • Remove CustomData_has_layer_index_n, just loop over the total number of layers