renderer: fix hide/show by removing the unsound draw-list fingerprint replay gate

src/rendering/SoVulkanRenderManager.cpp

@@ -430,17 +430,6 @@ class SoVulkanRenderManagerP {
   uint32_t sceneFpMainCount = 0;
   SbBool sceneFpValid = FALSE;
 
-  // Cheap draw-list fingerprint gate for the expensive graph-fingerprint walk.
-  // computeSceneFingerprint() hashes only the retained main commands (world
-  // matrices + geometry buffer pointers + counts), so it changes on a real
-  // geometry/transform edit but is invariant under camera motion.  When it
-  // matches the previous frame's value, computeGraphFingerprint() (a full scene
-  // tree walk) is skipped and the cached graph fingerprint is reused.
-  uint64_t drawFpCached = 0;
-  SoNode * drawFpScene = nullptr;
-  uint32_t drawFpMainCount = 0;
-  SbBool drawFpValid = FALSE;
-
   SoIRRenderAction irAction;
   //! Second IR action used to re-record the overlay/decoration scenes every
   //! frame (their node-ids churn with the camera, so they cannot be retained);
@@ -1667,29 +1656,19 @@ SoVulkanRenderManagerP::prepareRenderParams(SbBool clearwindow,
   // below.  They are re-recorded separately afterwards (cheap) and merged onto
   // this main list.
    SbBool irReplayed = FALSE;
-   // Cheap fast-path: the graph fingerprint walk is O(N) over the whole scene,
-   // and on a retained (replayed) frame with no scene change it is pure waste.
-   // The walk folds scene node-ids but deliberately SKIPS the camera, so the
-   // fingerprint is invariant under camera motion; the only thing that changes
-   // it is a change to the main scene graph.  An SoNodeSensor attached to the
-   // scene root fires whenever any descendant is notified (a field write or a
-   // child-list edit), which is precisely a main-scene change -- so when the
-   // sensor has NOT fired since the last walk, the cached fingerprint is still
-   // exact and the O(N) walk is skipped.  This catches every change the walk
-   // would (Coin propagates notify() up to the root), independent of any
-   // external revision wiring, and camera-only frames still produce the same
-   // (camera-invariant) fingerprint.
-   //
-   // The sensor firing is itself the replay gate, not the fingerprint: the
-   // fingerprint folds node identity only (pointer + node-id + child count),
-   // so a FIELD-ONLY write (material, transform, preselection state) leaves it
-   // unchanged.  Replaying after such a write is unsafe anyway: the write
-   // notifies the shape (SoShape::notify() drops its retained tessellation),
-   // so the retained list's raw geometry pointers may reference freed storage.
-   // Re-traverse whenever the sensor has fired since the last walk; camera
-   // motion alone never fires the scene-root sensor (the camera lives outside
-   // the scene graph), so camera-only frames still replay.
-   const SbBool graphChanged = this->sceneGraphDirty;
+   // The graph fingerprint walk is O(N) over the scene nodes, so on a retained
+   // (replayed) frame with no scene change at all it is pure waste.  The walk
+   // folds scene node-ids but deliberately skips camera, light, environment and
+   // tag/infra nodes, so the fingerprint is invariant under camera motion; the
+   // only thing that changes it is a change to a render-affecting node.  An
+   // SoNodeSensor attached to the scene root fires whenever any descendant is
+   // notified (a field write or a child-list edit, including the camera pose --
+   // FreeCAD keeps the camera inside the scene graph).  When the sensor has NOT
+   // fired since the last walk the cached fingerprint is still exact and the
+   // walk/re-traversal can be skipped via the branch below.  When it HAS fired
+   // we must recompute the fingerprint (see the else) to distinguish camera-only
+   // churn (identical fingerprint -> replay) from a real content change
+   // (different fingerprint -> re-traverse).
    uint64_t graphFp;
    const SbVec2s fpVpSize = this->viewportRegion.getViewportSizePixels();
     if (this->graphFingerprintValid && this->lastFpValid &&
@@ -1698,34 +1677,26 @@ SoVulkanRenderManagerP::prepareRenderParams(SbBool clearwindow,
       graphFp = this->graphFingerprint;
     }
     else {
-      // Recomputing computeGraphFingerprint() walks the ENTIRE scene-graph tree
-      // (graphFingerprintWalk, O(nodes) + geometry identity) and is measurable
-      // on a many-object scene (1000 boxes -> ~28 ms).  It only needs to run
-      // when the retained main draw list actually changed.  graphSceneDirty is
-      // set by a scene-root sensor that fires on ANY descendant notification,
-      // including the camera pose (FreeCAD keeps the camera inside the scene
-      // graph), so camera-orbit frames set it too -- and recomputing the full
-      // graph fingerprint there is pure waste: camera motion never changes the
-      // retained main-list content.  Gate the expensive walk on the cheap
-      // draw-list fingerprint (computeSceneFingerprint hashes only the retained
-      // commands: world matrix + geometry pointers + counts).  Camera-only
-      // frames produce the same draw-list fingerprint, so we keep the previous
-      // graph fingerprint (which is likewise camera-invariant) and the retained
-      // list replays instead of re-traversing.
-      const uint64_t drawFp = computeSceneFingerprint(
-        this->irAction, static_cast<int>(this->mainCommandCount));
-      if (this->drawFpValid && this->drawFpCached == drawFp &&
-          this->scene == this->drawFpScene &&
-          this->mainCommandCount == this->drawFpMainCount) {
-        graphFp = this->graphFingerprint;  // draw list unchanged -> reuse
-      }
-      else {
-        graphFp = this->computeGraphFingerprint();
-        this->drawFpCached = drawFp;
-        this->drawFpScene = this->scene;
-        this->drawFpMainCount = this->mainCommandCount;
-        this->drawFpValid = TRUE;
-      }
+      // Recompute the graph fingerprint whenever the scene sensor has fired.
+      // graphFingerprintWalk folds the node-id of every non-camera-coupled,
+      // render-affecting node, so a real content change that alters what the
+      // walk sees -- an add/remove, a geometry rebuild, a transform/model-matrix
+      // write, a material/selection field write, or a SoSwitch::whichChild
+      // visibility toggle -- bumps at least one folded id and produces a
+      // DIFFERENT fingerprint, correctly forcing a re-traverse below.  The
+      // root sensor also fires on camera pose/headlight motion, but those nodes
+      // are EXCLUDED from the walk, so camera-only frames yield an identical
+      // (camera-invariant) fingerprint and the retained main list replays.
+      //
+      // Do NOT short-circuit this walk with a cheap hash of the retained draw
+      // list: that is unsound.  The retained list is the PREVIOUS frame's graph
+      // output, so a visibility toggle (SoSwitch::whichChild) changes the scene
+      // without yet changing the retained commands -- their fingerprint is
+      // therefore unchanged, and skipping the walk would replay the stale list
+      // forever, leaving an object's show/hide state never reflected in the
+      // viewport.  The walk is the authoritative signal and is O(nodes) (a few
+      // mixHash per node), far cheaper than an actual re-traversal.
+      graphFp = this->computeGraphFingerprint();
       this->sceneGraphDirty = FALSE;
     }
    this->lastFpScene = this->scene;

testsuite/CMakeLists.txt

@@ -593,6 +593,20 @@ if(HAVE_EGL)
 		add_test(NAME VulkanRenderManagerTest COMMAND VulkanRenderManagerTest)
 		set_tests_properties(VulkanRenderManagerTest PROPERTIES SKIP_RETURN_CODE 77)
 
+		add_executable(VulkanRenderManagerVisibilityTest vulkan/vulkan-rendermanager-visibility-test.cpp)
+		target_link_libraries(VulkanRenderManagerVisibilityTest Coin ${COIN_TARGET_LINK_LIBRARIES})
+		target_compile_definitions(VulkanRenderManagerVisibilityTest PRIVATE COIN_INTERNAL)
+		target_include_directories(VulkanRenderManagerVisibilityTest PRIVATE
+			${PROJECT_SOURCE_DIR}/src
+			${PROJECT_SOURCE_DIR}/include
+			${PROJECT_BINARY_DIR}/include
+			${PROJECT_SOURCE_DIR}/testsuite
+			${COIN_TARGET_INCLUDE_DIRECTORIES}
+		)
+		add_test(NAME VulkanRenderManagerVisibilityTest COMMAND VulkanRenderManagerVisibilityTest)
+		set_tests_properties(VulkanRenderManagerVisibilityTest PROPERTIES SKIP_RETURN_CODE 77)
+
+
 		add_executable(VulkanLightStabilityTest vulkan/vulkan-light-stability-test.cpp)
 		target_link_libraries(VulkanLightStabilityTest Coin ${COIN_TARGET_LINK_LIBRARIES})
 		target_compile_definitions(VulkanLightStabilityTest PRIVATE COIN_INTERNAL)

testsuite/vulkan/vulkan-rendermanager-visibility-test.cpp

@@ -0,0 +1,151 @@
+// testsuite/vulkan/vulkan-rendermanager-visibility-test.cpp
+//
+// End-to-end test of the retained-IR replay gate in SoVulkanRenderManager:
+// FreeCAD hides/shows an object by setting SoSwitch::whichChild on the view's
+// root switch.  A garbage frame (camera-only) first settles the retained-list
+// fingerprint cache; a subsequent visibility toggle changes the scene WITHOUT
+// changing the (yet-to-be-rebuilt) retained draw list.  The replay gate must
+// NOT treat that as "unchanged" and replay the stale list -- the cube must
+// actually disappear on the next render.
+//
+// Before the fix the graph-fingerprint walk was short-circuited by a cheap hash
+// of the retained draw list; the toggle left that hash unchanged, the walk was
+// skipped, and the stale list replayed forever -- the cube stayed visible.
+
+#include "VulkanTestHarness.h"
+
+#include <Inventor/SoDB.h>
+#include <Inventor/SbColor.h>
+#include <Inventor/nodes/SoCube.h>
+#include <Inventor/nodes/SoLightModel.h>
+#include <Inventor/nodes/SoMaterial.h>
+#include <Inventor/nodes/SoPerspectiveCamera.h>
+#include <Inventor/nodes/SoSeparator.h>
+#include <Inventor/nodes/SoSwitch.h>
+#include <Inventor/rendering/SoVulkanRenderManager.h>
+
+using namespace vulkan_test;
+
+int
+main()
+{
+  Harness harness;
+  const int initResult = harness.init();
+  if (initResult != 0) return initResult;
+
+  int failures = 0;
+
+  SoSeparator * root = new SoSeparator;
+  root->ref();
+
+  SoPerspectiveCamera * camera = new SoPerspectiveCamera;
+  camera->position.setValue(0.0f, 0.0f, 5.0f);
+  camera->nearDistance = 0.1f;
+  camera->farDistance = 100.0f;
+  camera->heightAngle = 0.785398f; // 45 degrees
+  camera->orientation.setValue(SbVec3f(0.0f, 0.0f, 1.0f), 0.0f);
+  root->addChild(camera);
+
+  SoLightModel * lightModel = new SoLightModel;
+  lightModel->model = SoLightModel::BASE_COLOR;
+  root->addChild(lightModel);
+
+  SoMaterial * material = new SoMaterial;
+  material->diffuseColor.setValue(1.0f, 0.0f, 0.0f);
+  root->addChild(material);
+
+  SoSwitch * sw = new SoSwitch;
+  sw->whichChild = 0;
+  SoCube * cube = new SoCube;
+  cube->width = 2.0f;
+  cube->height = 2.0f;
+  cube->depth = 2.0f;
+  sw->addChild(cube);
+  root->addChild(sw);
+
+  {
+    SoVulkanRenderManager manager;
+    manager.setSceneGraph(root);
+    manager.setCamera(camera);
+
+    SbViewportRegion viewport(kWidth, kHeight);
+    viewport.setViewportPixels(SbVec2s(0, 0), SbVec2s(kWidth, kHeight));
+    manager.setViewportRegion(viewport);
+    manager.setBackgroundColor(SbColor4f(0.0f, 0.0f, 0.0f, 1.0f));
+    manager.setRenderTarget(&harness.target);
+
+    if (!manager.initialize(&harness.deviceContext)) {
+      std::cerr << "FAIL: manager could not initialize the Vulkan backend"
+                << std::endl;
+      ++failures;
+      root->unref();
+      harness.shutdown();
+      SoDB::finish();
+      return failures == 0 ? 0 : 1;
+    }
+
+    auto countRed = [&](const char * tag) {
+      const std::vector<uint8_t> pixels = harness.readback();
+      const int red = countNear(pixels, 255, 0, 0);
+      const uint8_t * center = pixelAt(pixels, 16, 16);
+      std::cerr << "[TEST] " << tag << " redPixels=" << red
+                << " center=(" << (int)center[2] << "," << (int)center[1]
+                << "," << (int)center[0] << ")" << std::endl;
+      return red;
+    };
+
+    // Frame 1: initial build -> cube visible.
+    if (!manager.render(TRUE, TRUE)) {
+      std::cerr << "FAIL: frame-1 render failed" << std::endl;
+      ++failures;
+    } else if (countRed("frame1 build") <= 0) {
+      std::cerr << "FAIL: cube not rendered red on frame 1" << std::endl;
+      ++failures;
+    }
+
+    // Frame 2: camera-only "settling" frame.  Moving the camera inside the
+    // scene fires the scene-dirty sensor but leaves the retained content
+    // unchanged, so the retained-list fingerprint cache settles (drawFpCached
+    // now equals the cube list) and the gate is armed for the steady state.
+    camera->position.setValue(0.0f, 0.0f, 6.0f);
+    SoDB::getSensorManager()->processDelayQueue(TRUE);
+    if (!manager.render(TRUE, TRUE)) {
+      std::cerr << "FAIL: frame-2 framing render failed" << std::endl;
+      ++failures;
+    } else if (countRed("frame2 settle") <= 0) {
+      std::cerr << "FAIL: cube not rendered red on settle frame" << std::endl;
+      ++failures;
+    }
+
+    // Frame 3: hide the cube via SoSwitch::whichChild.  The retained list is
+    // NOT rebuilt yet, so its fingerprint is unchanged; only the graph walk can
+    // see the toggle.  Old code replays the stale list; the fix re-traverses.
+    sw->whichChild = -1;
+    SoDB::getSensorManager()->processDelayQueue(TRUE);
+    if (!manager.render(TRUE, TRUE)) {
+      std::cerr << "FAIL: frame-3 (hide) render failed" << std::endl;
+      ++failures;
+    } else if (countRed("frame3 hide") != 0) {
+      std::cerr << "FAIL: cube STILL rendered after hide (stale replay)"
+                << std::endl;
+      ++failures;
+    }
+
+    // Frame 4: show the cube again.
+    sw->whichChild = 0;
+    SoDB::getSensorManager()->processDelayQueue(TRUE);
+    if (!manager.render(TRUE, TRUE)) {
+      std::cerr << "FAIL: frame-4 (show) render failed" << std::endl;
+      ++failures;
+    } else if (countRed("frame4 show") <= 0) {
+      std::cerr << "FAIL: cube not rendered on frame-4 after show"
+                << std::endl;
+      ++failures;
+    }
+  } // manager destroyed before harness.shutdown()
+
+  root->unref();
+  harness.shutdown();
+  SoDB::finish();
+  return failures == 0 ? 0 : 1;
+}