Triangulated cached conversions for IECoreGL::MeshPrimitive

[ldmoser]

So this solution tries to solve two issues:

A) Our profiles indicate that there's lots of computation going on to the triangulation of meshes when rendering them with IECoreGL. We would like to take advantage of repetitive geometry and cache intermediate results (vertex triangulation, triangulated primVars, generation of normals) as opposed to only get the cache benefits for the entire Mesh (animated geo does not match and have to be reconverted entirely).

B) We envision developing ways to update the GL scene, using a scene edits mechanism. So ideally we would want to convert the IECore primitive to IECoreGL once, and then, replace the whole primitive OR update only a subset of the primitive variables (the ones that were affected), by providing the original non-triangulated prim vars.

I decided to focus the solution on Meshes, since that's the most common geometry in a SceneCache and leave other types of primitives unmodified for now, but the same ideas could be applied for PointsPrimitive and CurvesPrimitive.

Solution adopted: (1) expand the CachedConverter to accept custom converter objects (callable objects with a hash function). Then, (2) adding an additional constructor for IECoreGL::MeshPrimitive where it receives the original non-triangulated topology. And lastly, (3) create converters for each one of the basic operations that happens when converting a Mesh to IECoreGL (including triangulation).

I have some open questions for this approach:

• The concept of the CachedConverter seems pretty useful for other situations. Should we instead create a IECore::CachedResults where it always expect a callable object to be passed (with a hash function) that does the operation? So it could be used on general IECore Ops, such as the Normals, Triangulate, etc. I think the only reason why not, would be that the IECoreGL::CachedConverter cache must be cleared at safe threads.
• The triangulation algorithms are now in IECoreGL::MeshPrimitive. But I left some TODOs to consider unifying the code with the triangulate op in IECore.
• The computation of normals is currently done in the conversion. But the issue (B) would require it to be in the addPrimitiveVariable() every time that "P" is redefined.
• The computation of "st" is currently done in the ToGLMeshConverter (as before). I guess that we would want to support in Cortex V2f UV primVars and deprecate that code eventually.

[andrewkaufman]

John, did you have a chance to look over this one?

[johnhaddon]

I had to tweak this to get it to build on mac :

h.append( (uint64_t)m_faceVaryingLength );

[johnhaddon]

This looks grand to me, aside from the few very minor comments I've made inline.

I quite like the idea of having a generalised cached results mechanism in IECore, but I think for now it'd be preferable to go with the implementation as-is and worry about generalising later. The tricky part of the implementation (the LRUCache) is already shared among the different caching mechanisms we already have, so I'm not sure how much benefit we'd get from generalising further. Each candidate for the generalised form (ops, readers, etc) also seem like they might require a slightly different interface (searchpaths for the CachedReader, delayed destruction and contents for the GL converter) so I think generalising further might be more trouble than it's worth.

Do you have any performance figures you could share for this Lucio? From what I recall in our discussions before it was quite significant, and I've certainly seen profiles where triangulation and facevarying conversion was the primary bottleneck, so it might be nice to give people an idea of what benefit they can expect to see here.

I'm also curious to know if the bound computation in addPrimitiveVariable() is now showing up in profiles at all - might that also be a candidate for caching?

[ldmoser]

Thanks for the review John! I believe I've addressed all of your notes.

I will try to get some data about performance, currently I don't have anything conclusive for this particular implementation, mainly because I was already testing the next round of changes, which would be about only loading the animated primVars as opposed to the whole geometry...

[ldmoser]

I'm having second thoughts about this pull request.

After profiling a bit and testing, I've realized some things:

1. Now each Mesh that is converted is reporting twice the memory it actually consumes in the cache, because each primVar is stored there, plus the whole Mesh is also stored (due to the original cachedConverter mechanism used in the Renderer). As a result, I had to set the IECOREGL_CACHEDCONVERTER_MEMORY higher to prevent cache cleanups while changing the frames in an animation and take advantage of the intermediate cached results.

2. The Normals converter is making the reported memory usage even worse because it passes the whole Mesh as the input object to convert and the CachedConverter takes it's memoryUsage value instead of the memory usage from the object that is stored in the cache. I have a local fix which is inlined with the other converters (it's a low level one that operated directly with "P" data), but the cost is still a bit wrong since it related to the input object, not the object that is stored in the cache.

3. When turning off the original cache mechanism ( by setting automaticInstancing = false ), and using a huge cache size, then switching between two frames a couple times (to add everything to cache), I haven't managed to get same timings. There's a certain cost on reusing intermediate results and I'm not sure we have seen a scenario where it justifies the overhead.

What I have been thinking is on turning off the intermediate cache for now (by calling the custom converter functions directly, without the use of CachedConverter), until we are sure they are necessary. But, keep the ability to call the addPrimitiveVariable passing the non-triangulated primvars.

Thoughts?

[johnhaddon]

I'd still like to see some hard performance numbers for this change - it's all about optimisation so without numbers it's very hard to have a meaningful discussion. How about the time for a single run through of a sequence of a character-sized deforming quad mesh with changing P and with IECOREGL_CACHEDCONVERTER_MEMORY set high enough that cache removals are a non-issue? By making sure the cache is cleared before each run we should negate any benefits of automaticInstancing and get some idea of the benefits of this change alone. I've definitely seen profiles where the triangulation and conversion of the entire mesh when only a primvar was changing was the major overhead, so I'd be hopeful that the numbers would be good.

Unless we do have compelling numbers (either for this case or for a use case using addPrimitiveVariable()) my preference would be to back out of the changes. They introduce a fair bit of additional complexity and duplicated code over the original which is absolutely fine if they bring benefits, but not really worth it if they don't.

Assuming we want to keep the changes, perhaps we can address the memory measurement problem properly, as it's really not a problem with your changes but with the existing mechanism. The CachedConverter was introduced mid-major-version so the current memory usage estimate is the closest we could get at the time without introducing incompatible changes. Perhaps we could introduce a base class shared by all the GL types (rather than just using RunTimeTyped as we do now), and that could provide a memoryUsage() method? I wonder also if there might be some mileage in converting direct from the non-triangulated IECore::Data to an IECoreGL::Buffer, thus genuinely reducing the data storage?