SG-40852: Improve paint speed degrading as you draw many annotations (#984)

### Linked issues

SG-40852

### Summarize your change.

Pretty significant fix in the management of the annotation fbo cache for
the current frame, which got broken some years ago.

In a nutshell, the rendered fbo cache should always have all of the
paint commands (eg: annotation) rendered in it, except for the last
paint command, in case we're currently editing it (eg: drawing a
stroke).

Somehow, this cache management was really borked. It would fail to find
a cache, then draw all annotations in the cache, keep the cache. At the
next redraw, it would find the cache, render the start of a new stroke,
but as you edited it, it wouldn't find the cache anymore, recreated the
cache, found apply the last stroke, which would somehow invalidate the
cache, and at the next update it would go through this cycle again. In
other words, it would redraw everything, every time.

I've made a fix such that when a new stroke is added, it will
(unfortunately) still draw everything, but as the stroke updates, it
will keep only rendering a single paint command on top of the cache,
until you start a new stroke.

Repainting all strokes when you start a new one, if you have many, like
250+ srokes, might take like 150ms, which is a definitely a lot, but it
fixes the problem where you'd pay the 150ms at every single mouse move
(ugh).

Anyway, things should be be much better now.


### Describe the reason for the change.

customer complaints, and could reproduce this easily with rv_tests
scripts.

### Describe what you have tested and on which operating system.

macOS.

### Add a list of changes, and note any that might need special
attention during the review.

During the review, you should make sure that nothing got broken in the
process. Try all tools: reaser, pen, dodge, burn, etc.

You should also verify that simulateneous paint strokes in live review
collab mode still works.

In particular, @bernie-laberge, please make sure that your fix for
recomputing the render ID still works.

During testing, please do the following:
- Start OLD RV (eg:last week). Open media. Press F10 to show annotate
tools
- Start NEW RV (this VT build). Open media. Press F10 also

Both RV, do:
-> Menu -> RV Tests -> Automatic Annotation Drawing -> start drawing
circles.
(load rv tests package if not done so already)

Both RVs should start to draw circles.

Let it got for a few minutes. After a while, maybe 5-6-10 minutes, the
OLD RV should be extremely slow. The NEW RV will have slowed down a bit,
but not nearly as bad. (note: this will create hundreds and hundreds of
strokes at that frame,

### If possible, provide screenshots.

---------

Signed-off-by: Patrick Bergeron <[email protected]>

Author: pbergeron-adsk

src/lib/ip/IPCore/ImageRenderer.cpp

@@ -16,6 +16,7 @@
 #include <IPCore/IPNode.h>
 #include <IPCore/ShaderProgram.h>
 #include <IPCore/Application.h>
+#include <IPCore/PaintCommand.h>
 #include <TwkExc/TwkExcException.h>
 #include <TwkGLF/GL.h>
 #include <TwkGLF/GLState.h>
@@ -1595,8 +1596,7 @@ namespace IPCore
                     {
                         uploadRoot = root;
                     }
-                    m_uploadThreadPrefetch =
-                        (root == uploadRoot) ? false : true;
+                    m_uploadThreadPrefetch = (root == uploadRoot) ? false : true;
 
                     //
                     // traverse our IPTree, and create gl textures/buffers for
@@ -4854,9 +4854,6 @@ namespace IPCore
             return;
 
         const string prenderID = imageToFBOIdentifier(root);
-        // these two are for pingpong
-        const GLFBO* tempfbo1 = m_imageFBOManager.newImageFBO(fbo, m_fullRenderSerialNumber, prenderID)->fbo();
-        const GLFBO* tempfbo2 = m_imageFBOManager.newImageFBO(fbo, m_fullRenderSerialNumber, prenderID)->fbo();
 
         assert(fbo);
 
@@ -4905,11 +4902,26 @@ namespace IPCore
             if (foundCachedFBO)
             {
                 startCmd = lastCmdNum;
-                cachedFBO->fbo()->copyTo(tempfbo1);
             }
             else
             {
                 cachedFBO = m_imageFBOManager.newImageFBO(fbo, m_fullRenderSerialNumber, newRenderID.str());
+            }
+        }
+
+        // Allocate temp FBOs AFTER cache lookup so the cached FBO is protected from being reused
+        const GLFBO* tempfbo1 = m_imageFBOManager.newImageFBO(fbo, m_fullRenderSerialNumber, prenderID)->fbo();
+        const GLFBO* tempfbo2 = m_imageFBOManager.newImageFBO(fbo, m_fullRenderSerialNumber, prenderID)->fbo();
+
+        // Copy initial render to temp buffer
+        if (root->commands.size() > 1)
+        {
+            if (foundCachedFBO)
+            {
+                cachedFBO->fbo()->copyTo(tempfbo1);
+            }
+            else
+            {
                 fbo->copyTo(tempfbo1);
             }
         }
@@ -5049,7 +5061,9 @@ namespace IPCore
             Paint::renderPaintCommands(paintContext);
         }
 
-        if (!paintContext.cacheUpdated && cachedFBO)
+        // Only clear the cache identifier if we didn't use an existing cache
+        // If we had a cache hit (foundCachedFBO), keep the identifier even if we didn't update it
+        if (!paintContext.cacheUpdated && cachedFBO && !foundCachedFBO)
             cachedFBO->identifier = "";
 
         /////////////////// clean up /////////////////////////////////////

src/lib/ip/IPCore/PaintCommand.cpp

@@ -784,32 +784,6 @@ namespace IPCore
             size_t count = 0;
             for (size_t i = 0; i < context.commands.size(); count++)
             {
-                //
-                // update the cache, only up to the second to the last cmd we
-                // don't cache the render with the very last cmd because doing
-                // so will cause the last cmd to be rendered twice, and in
-                // case of alpha not 0, or 1, it will not be correct
-                //
-                // Note: There is no cache to update when there is only 1
-                // command to render as the context.cachedfbo is already up to
-                // date. In other words when the last command is also the first
-                // command, we don't update the cache.
-                //
-
-                if (context.updateCache && context.cachedfbo && (context.commands.size() > 1)
-                    && (context.commands[i] == context.lastCommand))
-                {
-                    if (i > 0)
-                    {
-                        currentFBO->copyTo(context.cachedfbo);
-                    }
-                    else
-                    {
-                        tempfbo1->copyTo(context.cachedfbo);
-                    }
-                    context.cacheUpdated = true;
-                }
-
                 const Paint::Command* cmd = context.commands[i];
                 if (cmd->getType() == Paint::Command::ExecuteAllBefore)
                 {
@@ -875,6 +849,23 @@ namespace IPCore
                     cmd->execute(commandContext);
                     ++i;
                 }
+                // Update the cache AFTER command execution
+                // We cache up to the second-to-last command in the batch (not the very last, which might still be dragging)
+                // count is the number of commands rendered so far (1-indexed after increment)
+                // Cache after rendering the second-to-last command: count == curCmdNum - 1
+                bool shouldCache = context.updateCache && context.cachedfbo && (curCmdNum > 1) && (count == curCmdNum - 1);
+
+                if (shouldCache)
+                {
+                    // Always save to cache from tempfbo1 to maintain consistent ping-pong state
+                    // If currentFBO is tempfbo2, copy it to tempfbo1 first
+                    if (currentFBO != tempfbo1)
+                    {
+                        currentFBO->copyTo(tempfbo1);
+                    }
+                    tempfbo1->copyTo(context.cachedfbo);
+                    context.cacheUpdated = true;
+                }
             }
 
             context.commandExecuted = count;