Cycles: Speedup Tangent calculation in Mikktspace
AbandonedPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Jun 4 2017, 11:58 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)
Campbell Barton (campbellbarton)

Summary

The current code is fairly inefficient for multiple reasons.
This commit addresses two of them:

The getter functions were directly accessing the RNA arrays, which causes a lot of overhead since they are called many times per vertex/face. By iterating over the RNA arrays once and caching the data in temporary storage, this overhead is avoided.
The step that merges duplicate vertices only performed binning in one spatial dimension and then looped over all vertices in each bin. By hashing all nine components, binning these hashes and then checking inside each bin the code is simplified a lot while also saving a huge amount of time.

There are many more optimization opportunities, but these two already halve
render times for my test file (suzanne with default unwrapping and subdivided
to level 6).

Diff Detail

Repository

rB Blender

Branch

tangent_speedup

Build Status

Buildable 688
Build 688: arc lint + arc unit

Event Timeline

Lukas Stockner (lukasstockner97) created this revision.Jun 4 2017, 11:58 PM

Hi Lukas, great to see you back on other task than Denoiser :)
When trying to build on MSVC 2015, it prints some errors:

error log MSVC 2015.txt11 KBDownload

Ah, I see, you'll have to add 'typedef unsigned int uint;' above float_as_uint.

Tested this with 9-10 scene's and had no issues, Gives nice speedup, 5-20% generally for me.

Built OSX, added the 'typedef unsigned int uint;' above float_as_uint.
Then got these two errors in mikktspace.c (had to use screen capture as I didn't find a way to export error log yet).

Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).Jun 7 2017, 5:14 AM

If the C++ RNA is a bottleneck, then we should either look into ways to optimize it or to reformulate synchronization code in Cycles in a way that we don't need to duplicate data again and again: you can easily re-use attributes which already exists in Cycles. Additionally, it if it's RNA to blame, you still have overhead on synchronizing mesh itself.

One more thing: since rB5abae51 we can have access to all tangent maps generated on Blender side. If we are going to change how tangents are calculated in Cycles i would strongly suggest to make it so tangents are handled by Blender side. Would avoid the RNA bottleneck you've mentioned, avoid duplicated code, and additionally will make code more robust against further changes.

intern/cycles/blender/blender_mesh.cpp
152–155	This adds yet another copy of data. How does this affect on memory usage when rendering production files?

Steffen Dünner (SteffenD) added a subscriber: Steffen Dünner (SteffenD).Jun 7 2017, 11:18 AM

Brecht Van Lommel (brecht) mentioned this in T50974: Normal map tangent computation is slow.Jul 21 2017, 3:37 AM

Changes to mikkspace itself can go into master perhaps, but Cycles part i;d rather look into solution which will not involve data and code duplicaiton. Have some ideas which i'm experimenting with atm.

Sergey Sharybin (sergey) mentioned this in D2810: Cycles: Speedup up tangent space calculation.Aug 25 2017, 10:50 PM

Now that we have D2810, here's a new version containing only the Mikktspace changes.