small bugs fixes in C++ Animated that were not observed in production (facebook#54795)

Summary:
changelog: [internal]

Pull Request resolved: facebook#54795

Fix five bugs in C++ Animated that could cause incorrect animation behavior, memory issues, and graph traversal failures:

- Fixed duplicate condition check in BFS graph traversal that prevented proper node visitation
- Fixed RoundAnimatedNode reading wrong config key for the rounding factor
- Fixed setOffset returning true even when value unchanged, causing unnecessary updates
- Fixed FrameAnimationDriver accumulating frames on reconfiguration instead of clearing
- Removed unreachable dead code in ColorAnimatedNode

Reviewed By: zeyap

Differential Revision: D88362078

fbshipit-source-id: 50b5cbf7ecb83f1a098498a752e2cb2297657dd8

Author: sammy-SC

packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp

@@ -723,7 +723,8 @@ void NativeAnimatedNodesManager::updateNodes(
     for (auto childTag : nextNode.node->getChildren()) {
       auto child = getAnimatedNode<AnimatedNode>(childTag);
       child->activeIncomingNodes--;
-      if (child->activeIncomingNodes == 0 && child->activeIncomingNodes == 0) {
+      if (child->activeIncomingNodes == 0 &&
+          child->bfsColor != animatedGraphBFSColor_) {
         child->bfsColor = animatedGraphBFSColor_;
 #ifdef REACT_NATIVE_DEBUG
         updatedNodesCount++;

packages/react-native/ReactCommon/react/renderer/animated/drivers/FrameAnimationDriver.cpp

@@ -43,6 +43,7 @@ void FrameAnimationDriver::updateConfig(folly::dynamic config) {
 void FrameAnimationDriver::onConfigChanged() {
   auto frames = config_["frames"];
   react_native_assert(frames.type() == folly::dynamic::ARRAY);
+  frames_.clear();
   for (const auto& frame : frames) {
     auto frameValue = frame.asDouble();
     frames_.push_back(frameValue);

packages/react-native/ReactCommon/react/renderer/animated/nodes/ColorAnimatedNode.cpp

@@ -62,7 +62,6 @@ Color ColorAnimatedNode::getColor() {
     manager_->updatedNodeTags_.erase(tag_);
   }
   return color_;
-  return 0;
 }
 
 } // namespace facebook::react

packages/react-native/ReactCommon/react/renderer/animated/nodes/RoundAnimatedNode.cpp

@@ -22,7 +22,7 @@ RoundAnimatedNode::RoundAnimatedNode(
     NativeAnimatedNodesManager& manager)
     : ValueAnimatedNode(tag, config, manager),
       inputNodeTag_(static_cast<Tag>(getConfig()["input"].asInt())),
-      nearest_(getConfig()["input"].asDouble()) {
+      nearest_(getConfig()["nearest"].asDouble()) {
   react_native_assert(
       nearest_ != 0 &&
       "'nearest' cannot be 0 (can't round to the nearest multiple of 0)");

packages/react-native/ReactCommon/react/renderer/animated/nodes/ValueAnimatedNode.cpp

@@ -55,7 +55,7 @@ bool ValueAnimatedNode::setOffset(double offset) noexcept {
     offset_ = offset;
     return true;
   }
-  return true;
+  return false;
 }
 
 double ValueAnimatedNode::getValue() const noexcept {

packages/react-native/ReactCommon/react/renderer/animated/tests/AnimatedNodeTests.cpp

@@ -215,6 +215,73 @@ TEST_F(AnimatedNodeTests, DiffClampAnimatedNode) {
   EXPECT_EQ(nodesManager_->getValue(diffClampTag), 1);
 }
 
+TEST_F(AnimatedNodeTests, RoundAnimatedNodeUsesNearestConfigKey) {
+  // This test verifies that RoundAnimatedNode reads the "nearest" config key
+  // for the rounding factor, not the "input" key.
+  initNodesManager();
+
+  auto rootTag = getNextRootViewTag();
+
+  auto valueTag = ++rootTag;
+  auto roundTag = ++rootTag;
+
+  nodesManager_->createAnimatedNode(
+      valueTag,
+      folly::dynamic::object("type", "value")("value", 7.3)("offset", 0));
+
+  // The round node should read "nearest" for the rounding factor (5.0),
+  // not "input" (which is the valueTag integer).
+  nodesManager_->createAnimatedNode(
+      roundTag,
+      folly::dynamic::object("type", "round")("input", valueTag)(
+          "nearest", 5.0));
+  nodesManager_->connectAnimatedNodes(valueTag, roundTag);
+
+  runAnimationFrame(0);
+
+  // 7.3 rounded to nearest 5.0 should be 5.0 (since round(7.3/5) * 5 = 1 * 5)
+  // If the bug existed (reading "input" instead of "nearest"), it would use
+  // the valueTag as the rounding factor, giving incorrect results.
+  EXPECT_DOUBLE_EQ(nodesManager_->getValue(roundTag).value(), 5.0);
+
+  // Test another value to ensure rounding works correctly
+  nodesManager_->setAnimatedNodeValue(valueTag, 12.6);
+  runAnimationFrame(0);
+
+  // 12.6 rounded to nearest 5.0 should be 15.0 (since round(12.6/5) * 5 = 3 *
+  // 5)
+  EXPECT_DOUBLE_EQ(nodesManager_->getValue(roundTag).value(), 15.0);
+}
+
+TEST_F(AnimatedNodeTests, SetOffsetReturnsFalseWhenUnchanged) {
+  // This test verifies that setAnimatedNodeOffset doesn't trigger unnecessary
+  // updates when the offset value hasn't changed.
+  initNodesManager();
+
+  auto rootTag = getNextRootViewTag();
+  auto valueTag = ++rootTag;
+
+  nodesManager_->createAnimatedNode(
+      valueTag,
+      folly::dynamic::object("type", "value")("value", 10)("offset", 0));
+
+  runAnimationFrame(0);
+  EXPECT_EQ(nodeNeedsUpdate(valueTag), false);
+
+  // First setOffset should mark the node as needing update
+  nodesManager_->setAnimatedNodeOffset(valueTag, 5);
+  EXPECT_EQ(nodeNeedsUpdate(valueTag), true);
+  EXPECT_EQ(nodesManager_->getValue(valueTag), 15); // 10 + 5
+
+  runAnimationFrame(0);
+  EXPECT_EQ(nodeNeedsUpdate(valueTag), false);
+
+  // Setting the same offset again should NOT mark the node as needing update
+  nodesManager_->setAnimatedNodeOffset(valueTag, 5);
+  EXPECT_EQ(nodeNeedsUpdate(valueTag), false); // No change, no update needed
+  EXPECT_EQ(nodesManager_->getValue(valueTag), 15); // Still 10 + 5
+}
+
 TEST_F(AnimatedNodeTests, ObjectAnimatedNode) {
   initNodesManager();
 

packages/react-native/ReactCommon/react/renderer/animated/tests/AnimationDriverTests.cpp

@@ -58,4 +58,63 @@ TEST_F(AnimationDriverTests, framesAnimation) {
   EXPECT_EQ(round(nodesManager_->getValue(valueNodeTag).value()), toValue);
 }
 
+TEST_F(AnimationDriverTests, framesAnimationReconfigurationClearsFrames) {
+  // This test verifies that when an animation is reconfigured via updateConfig,
+  // the frames array is cleared before adding new frames. Without clearing,
+  // frames would accumulate and cause incorrect animation behavior.
+  initNodesManager();
+
+  auto rootTag = getNextRootViewTag();
+
+  auto valueNodeTag = ++rootTag;
+  nodesManager_->createAnimatedNode(
+      valueNodeTag,
+      folly::dynamic::object("type", "value")("value", 0)("offset", 0));
+
+  const auto animationId = 1;
+  // First animation: 5 frames from 0 to 100
+  const auto frames1 = folly::dynamic::array(0.0f, 0.25f, 0.5f, 0.75f, 1.0f);
+  const auto toValue1 = 100;
+  nodesManager_->startAnimatingNode(
+      animationId,
+      valueNodeTag,
+      folly::dynamic::object("type", "frames")("frames", frames1)(
+          "toValue", toValue1),
+      std::nullopt);
+
+  const double startTimeInTick = 12345;
+
+  // Run first frame
+  runAnimationFrame(startTimeInTick);
+  EXPECT_EQ(round(nodesManager_->getValue(valueNodeTag).value()), 0);
+
+  // Reconfigure the same animation (same animationId) with new frames
+  // This triggers updateConfig on the existing FrameAnimationDriver
+  const auto frames2 = folly::dynamic::array(0.0f, 0.5f, 1.0f);
+  const auto toValue2 = 200;
+  nodesManager_->startAnimatingNode(
+      animationId,
+      valueNodeTag,
+      folly::dynamic::object("type", "frames")("frames", frames2)(
+          "toValue", toValue2),
+      std::nullopt);
+
+  // Reset animation timing
+  const double newStartTimeInTick = 20000;
+
+  // Run animation at halfway point (1 frame into 3-frame animation)
+  runAnimationFrame(newStartTimeInTick);
+  runAnimationFrame(newStartTimeInTick + SingleFrameIntervalMs * 1);
+
+  // At frame 1 of 3 frames (50% progress), value should be approximately:
+  // startValue (0) + 0.5 * (toValue2 - startValue) = 0 + 0.5 * 200 = 100
+  // If frames accumulated (5 + 3 = 8 frames), we'd be at wrong position
+  // Use ceil rounding so 100.00x becomes 100.01
+  EXPECT_EQ(round(nodesManager_->getValue(valueNodeTag).value()), 100.01);
+
+  // Complete the animation
+  runAnimationFrame(newStartTimeInTick + SingleFrameIntervalMs * 2);
+  EXPECT_EQ(round(nodesManager_->getValue(valueNodeTag).value()), toValue2);
+}
+
 } // namespace facebook::react