Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Checks:
# - misc-*
# - modernize-*
# - objc-*
# - performance-*
- performance-*
# - portability-*
# - readability-*
WarningsAsErrors: '*'
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jsi::Value AnimatedSensorModule::registerSensor(
"[Reanimated] Sensor event handler must be a worklet.");

int sensorId = platformRegisterSensorFunction_(
sensorType,
static_cast<int>(sensorType),
interval.asNumber(),
iosReferenceFrame.asNumber(),
[sensorType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace reanimated {
using namespace facebook;
using namespace worklets;

enum SensorType {
enum class SensorType : std::uint8_t {
ACCELEROMETER = 1,
GYROSCOPE = 2,
GRAVITY = 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace reanimated::css {

enum class TransformOp {
enum class TransformOp : std::uint8_t {
Perspective,
Rotate,
RotateX,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

namespace reanimated::css {

enum class ColorType {
enum class ColorType : std::uint8_t {
Rgba,
Transparent,
CurrentColor, // for SVG
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace reanimated::css {

using namespace facebook;

enum class RelativeTo {
enum class RelativeTo : std::uint8_t {
Parent,
Self,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ template <typename TResult>
std::unordered_map<size_t, TResult> parseHelper(
jsi::Runtime &rt,
const jsi::Object &settingsObj,
std::function<TResult(jsi::Runtime &, const jsi::Value &)> parseFunction) {
const std::function<TResult(jsi::Runtime &, const jsi::Value &)>
&parseFunction) {
std::unordered_map<size_t, TResult> result;

const auto animationIndices = settingsObj.getPropertyNames(rt);
Expand Down Expand Up @@ -173,14 +174,13 @@ CSSAnimationUpdates parseCSSAnimationUpdates(
CSSAnimationUpdates result;

if (configObj.hasProperty(rt, "animationNames")) {
const auto animationNames =
result.animationNames =
parseAnimationNames(rt, configObj.getProperty(rt, "animationNames"));
result.animationNames = std::move(animationNames);

if (configObj.hasProperty(rt, "newAnimationSettings")) {
result.newAnimationSettings = parseNewAnimationSettings(
rt,
animationNames,
result.animationNames.value(),
configObj.getProperty(rt, "newAnimationSettings"));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,14 @@

namespace reanimated::css {

enum class AnimationDirection { Normal, Reverse, Alternate, AlternateReverse };
enum class AnimationFillMode { None, Forwards, Backwards, Both };
enum class AnimationPlayState { Running, Paused };
enum class AnimationDirection : std::uint8_t {
Normal,
Reverse,
Alternate,
AlternateReverse
};
enum class AnimationFillMode : std::uint8_t { None, Forwards, Backwards, Both };
enum class AnimationPlayState : std::uint8_t { Running, Paused };

struct CSSAnimationSettings {
double duration;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#include <reanimated/CSS/interpolation/transforms/TransformOperationInterpolator.h>
#include <reanimated/CSS/interpolation/transforms/operations/matrix.h>

#include <algorithm>
#include <deque>
#include <memory>

Expand Down Expand Up @@ -143,7 +142,7 @@ MatrixOperation::MatrixOperation(TransformOperations operations)
// Simplify operations to reduce the number of matrix
// multiplications during matrix keyframe interpolation
: TransformOperation(TransformOp::Matrix), value([&]() {
const auto &[value, is3D] = simplifyOperations(std::move(operations));
const auto &[value, is3D] = simplifyOperations(operations);
is3D_ = is3D;
return value;
}()) {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

namespace reanimated::css {

enum class AnimationProgressState {
enum class AnimationProgressState : std::uint8_t {
Pending, // When the animation is waiting for the delay to pass
Running,
Paused,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@

namespace reanimated::css {

enum class TransitionProgressState { Pending, Running, Finished };
enum class TransitionProgressState : std::uint8_t {
Pending,
Running,
Finished
};

class TransitionPropertyProgressProvider final
: public KeyframeProgressProvider,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ void CSSAnimationsRegistry::apply(
registry_.emplace(
viewTag,
RegistryEntry{
std::move(animationsVector),
buildAnimationToIndexMap(animationsVector)});
animationsVector, buildAnimationToIndexMap(animationsVector)});
Comment on lines 35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should keep std::move here and just remove the const modifier from the animationsVector declaration:

const auto animationsVector =
      buildAnimationsVector(rt, shadowNode, animationNames, newAnimations);

This std::move call was added purposely but the the const modifier near the animationsVector prevented it from working as expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't get here is why do we move the animationsVector first and then try to use it straight away. Isn't it in indeterminate state after that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. Maybe, to be extra safe, we could move these registry updates below function calls that use the animationsVector directly to get something like this in the end:

auto animationsVector =
    buildAnimationsVector(rt, shadowNode, animationNames, newAnimations);

const auto viewTag = shadowNode->getTag();
if (animationsVector.empty()) {
  remove(viewTag);
  return;
}

runningAnimationIndicesMap_[viewTag].clear();

std::vector<size_t> updatedIndices;
for (const auto &[index, _] : newAnimations) {
  updatedIndices.push_back(index);
}
for (const auto &[index, _] : settingsUpdates) {
  updatedIndices.push_back(index);
}

updateAnimationSettings(animationsVector, settingsUpdates, timestamp);
for (size_t i = 0; i < animationsVector.size(); ++i) {
  scheduleOrActivateAnimation(i, animationsVector[i], timestamp);
}

auto animationToIndexMap = buildAnimationToIndexMap(animationsVector);
registry_.erase(viewTag);
registry_.emplace(
    viewTag,
    RegistryEntry{
        std::move(animationsVector), std::move(animationToIndexMap)});

Basically, I just removed the const modifier from the animationsVector and moved the registry updates after updateAnimationSettings and scheduleOrActivateAnimation function calls, which are passed the animationsVector. I also added std::move to the animationToIndexMap which is now created before the animationsVector is moved to the RegistryEntry.

Let me know what do you think about this approach.

runningAnimationIndicesMap_[viewTag].clear();

std::vector<size_t> updatedIndices;
Expand Down Expand Up @@ -93,7 +92,7 @@ CSSAnimationsVector CSSAnimationsRegistry::buildAnimationsVector(
// the registry
if (!animationNames.has_value()) {
if (registryIt != registry_.end()) {
return std::move(registryIt->second.animationsVector);
return registryIt->second.animationsVector;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ namespace reanimated::css {

template <typename TValue>
DelayedItem<TValue>::DelayedItem(const double timestamp, const TValue value)
: timestamp(timestamp), value(value) {}
: timestamp(timestamp), value(std::move(value)) {}

template <typename TValue>
bool DelayedItemComparator<TValue>::operator()(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#pragma once

typedef enum LayoutAnimationType {
#include <cstdint>

typedef enum class LayoutAnimationType : std::uint8_t {
ENTERING = 1,
EXITING = 2,
LAYOUT = 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ namespace reanimated {
void LayoutAnimationsManager::configureAnimationBatch(
const std::vector<LayoutAnimationConfig> &layoutAnimationsBatch) {
auto lock = std::unique_lock<std::recursive_mutex>(animationsMutex_);
for (auto layoutAnimationConfig : layoutAnimationsBatch) {
for (const auto &layoutAnimationConfig : layoutAnimationsBatch) {
const auto &[tag, type, config] = layoutAnimationConfig;
if (type == ENTERING) {
if (type == LayoutAnimationType::ENTERING) {
enteringAnimationsForNativeID_[tag] = config;
continue;
}
Expand Down Expand Up @@ -110,11 +110,11 @@ void LayoutAnimationsManager::transferConfigFromNativeID(
std::unordered_map<int, std::shared_ptr<Serializable>> &
LayoutAnimationsManager::getConfigsForType(const LayoutAnimationType type) {
switch (type) {
case ENTERING:
case LayoutAnimationType::ENTERING:
return enteringAnimations_;
case EXITING:
case LayoutAnimationType::EXITING:
return exitingAnimations_;
case LAYOUT:
case LayoutAnimationType::LAYOUT:
return layoutAnimations_;
default:
throw std::invalid_argument("[Reanimated] Unknown layout animation type");
Expand Down
Loading
Loading