Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UIBlocks don't flush at the correct time #1365

Closed
vishnevskiy opened this issue May 21, 2015 · 21 comments
Closed

UIBlocks don't flush at the correct time #1365

vishnevskiy opened this issue May 21, 2015 · 21 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@vishnevskiy
Copy link
Contributor

I upgraded from a much lower version (I had a personal fork) to 0.4.4 and the POPAnimation native module I made is now having issues.

RCT_EXPORT_METHOD(addAnimation:(NSNumber *)tag animation:(NSNumber *)animation callback:(RCTResponseSenderBlock)callback) {
  POPAnimation *anim = self.internalAnimations[animation];
  [self.internalAnimations removeObjectForKey:animation];
  anim.completionBlock = ^(POPAnimation *anim2, BOOL finished) {
    callback(@[@(finished)]);
  };

  [self.bridge.uiManager addUIBlock:^(RCTUIManager *uiManager, RCTSparseArray *viewRegistry) {
      RCTAssertMainThread();

      UIView *view = viewRegistry[tag];
      if (view) {
        [view.layer pop_addAnimation:anim forKey:[animation stringValue]];
      }
  }];
}

This is all I am doing to add animations to a view, but now sometimes the UIBlock won't execute until the next user interaction after this function was called or even more oddly if I just leave the simulator running within a minute or so it executes.

@vjeux told me to tag @tadeuzagallo on this 😄

Any ideas?

@brentvatne
Copy link
Collaborator

Curious to see what the solution is here!

@JohnyDays
Copy link
Contributor

I had a problem where a native action i was executing (animation related) was only happening after a while. The solution I found worked was to add

- (dispatch_queue_t)methodQueue {
    return dispatch_get_main_queue();
}

To my native module implementation

@tadeuzagallo
Copy link
Contributor

EDIT: The UIManager methods actually runs on its _shadowQueue, and -[UIManager flushUIBlocks] dispatches to the main queue.

The issue is that when you call -[UIManager addUIBlock:] the block will later be called by -[UIManager flushUIBlocks]. That happens on -[UIManager batchDidComplete], that is called on -[RCTBridge _handleBuffer:], but that only happens when there was a call made from JS to objc.

-[UIManager flushUIBlocks] will be dispatched on UIManager's _shadowQueue, so if your module is running on a different queue (the default is the bridge's _methodQueue) there's no way to be sure it'll finish before flushUIBlocks is called, and if it hasn't finished, it'll be called (as mentioned above) on the next time JS calls objc, that should be on the next time you interact with the app.

Hope that explains the behavior 😃

@ide
Copy link
Contributor

ide commented May 22, 2015

@tadeuzagallo should we make -[UIManager addUIBlock] schedule a task to run -[UIManager batchDidComplete] on the next iteration of the run loop if JS hasn't sent any commands?

@vishnevskiy
Copy link
Contributor Author

@tadeuzagallo Thanks I will try that out soon :)

@tadeuzagallo
Copy link
Contributor

@ide Hum, not sure, it'd introduce a 1 frame delay, that's probably not desired in most cases.

I think it'd make more sense to just allow accessing the UIManager on the main thread, but might be a little too much. What I believe makes more sense is to dispatch the blocks on the VSync, but that'd be a bigger change.

/cc @nicklockwood

@JohnyDays
Copy link
Contributor

@tadeuzagallo If I correctly understand your explanation, then perhaps my case was actually bug, since no calls to UIManager were actually being made in the native method. In fact I attempted to just call a native Alert (not through the react alert manager) and it still displayed the same behaviour, only appearing after a significant amount of time.

@ide
Copy link
Contributor

ide commented May 22, 2015

@JohnyDays that is expected behavior because you were calling into UIKit off of the main thread. Almost always dispatch_async(dispatch_get_main_queue()) when doing UIKit work.

@nicklockwood
Copy link
Contributor

Here's our internal POP module implementation. This will probably never be released as it will be made obsolete by the official animation API that @vjeux is working on, but if you're using it anyway, you may as well use something that works:

RKPOPAnimationManager.h

// Copyright 2004-present Facebook. All Rights Reserved.

#import "RCTBridgeModule.h"
#import "RCTInvalidating.h"

#import <POP/POPAnimation.h>

@interface RKPOPAnimationManager : NSObject <RCTInvalidating, RCTBridgeModule>

@end

RKPOPAnimationManager.m

// Copyright 2004-present Facebook. All Rights Reserved.

#import "RKPOPAnimationManager.h"

#import "RCTAssert.h"
#import "RCTBridge.h"
#import "RCTConvert.h"
#import "RCTLog.h"
#import "RCTSparseArray.h"
#import "RCTUIManager.h"

#import <POP/POP.h>

typedef NS_ENUM(NSInteger, RKPOPAnimationType) {
  RKPOPAnimationTypeSpring = 0,
  RKPOPAnimationTypeDecay,
  RKPOPAnimationTypeLinear,
  RKPOPAnimationTypeEaseIn,
  RKPOPAnimationTypeEaseOut,
  RKPOPAnimationTypeEaseInEaseOut,
};

/**
 * Properties for all animations that can be specified in JavaScript via
 * RKPOPAnimationManager.createAnimation().  These generally map directly
 * to POPAnimation properties.
 */
static const struct {
  __unsafe_unretained NSString *property;
  __unsafe_unretained NSString *velocity;
  __unsafe_unretained NSString *fromValue;
  __unsafe_unretained NSString *toValue;
  __unsafe_unretained NSString *springBounciness;
  __unsafe_unretained NSString *dynamicsTension;
  __unsafe_unretained NSString *dynamicsFriction;
  __unsafe_unretained NSString *dynamicsMass;
  __unsafe_unretained NSString *deceleration;
  __unsafe_unretained NSString *duration;
} RKPOPAnimationManagerProperties = {
  // Not all of these properties apply to all animation types
  .property = @"property",
  .velocity = @"velocity",
  .fromValue = @"fromValue",
  .toValue = @"toValue",
  .springBounciness = @"springBounciness",
  .dynamicsTension = @"dynamicsTension",
  .dynamicsFriction = @"dynamicsFriction",
  .dynamicsMass = @"dynamicsMass",
  .deceleration = @"deceleration",
  .duration = @"duration",
};

@implementation RKPOPAnimationManager
{
  RCTSparseArray *_animationRegistry;
}

@synthesize bridge = _bridge;

RCT_EXPORT_MODULE()

- (instancetype)init
{
  if ((self = [super init])) {
    _animationRegistry = [[RCTSparseArray alloc] init];
  }
  return self;
}

- (dispatch_queue_t)methodQueue
{
  return _bridge.uiManager.methodQueue;
}

- (NSDictionary *)constantsToExport
{
  return @{
    @"Types": @{
      @"spring": @(RKPOPAnimationTypeSpring),
      @"decay": @(RKPOPAnimationTypeDecay),
      @"linear": @(RKPOPAnimationTypeLinear),
      @"easeIn": @(RKPOPAnimationTypeEaseIn),
      @"easeOut": @(RKPOPAnimationTypeEaseOut),
      @"easeInEaseOut": @(RKPOPAnimationTypeEaseInEaseOut)
    },
    @"Properties": @{
      @"bounds": kPOPLayerBounds,
      @"position": kPOPLayerPosition,
      @"positionX": kPOPLayerPositionX,
      @"positionY": kPOPLayerPositionY,
      @"opacity": kPOPLayerOpacity,
      @"scaleX": kPOPLayerScaleX,
      @"scaleY": kPOPLayerScaleY,
      @"scaleXY": kPOPLayerScaleXY,
      @"subscaleXY": kPOPLayerSubscaleXY,
      @"translationX": kPOPLayerTranslationX,
      @"translationY": kPOPLayerTranslationY,
      @"translationZ": kPOPLayerTranslationZ,
      @"translationXY": kPOPLayerTranslationXY,
      @"subtranslationX": kPOPLayerSubtranslationX,
      @"subtranslationY": kPOPLayerSubtranslationY,
      @"subtranslationZ": kPOPLayerSubtranslationZ,
      @"subtranslationXY": kPOPLayerSubtranslationXY,
      @"zPosition": kPOPLayerZPosition,
      @"size": kPOPLayerSize,
      @"rotation": kPOPLayerRotation,
      @"rotationX": kPOPLayerRotationX,
      @"rotationY": kPOPLayerRotationY,
      @"shadowColor": kPOPLayerShadowColor,
      @"shadowOffset": kPOPLayerShadowOffset,
      @"shadowOpacity": kPOPLayerShadowOpacity,
      @"shadowRadius": kPOPLayerShadowRadius
    }
  };
}

- (void)dealloc
{
  RCTAssert(!self.valid, @"must call -invalidate before -dealloc");
}

- (BOOL)isValid
{
  return _animationRegistry != nil;
}

- (void)invalidate
{
  _animationRegistry = nil;
  _bridge = nil;
}

RCT_EXPORT_METHOD(createAnimationInternal:(NSNumber *)animationTag
                  type:(NSInteger)type
                  props:(NSDictionary *)props)
{
  RCTAssert(props != nil, @"props should exist");
  [_bridge.uiManager addUIBlock:^(RCTUIManager *uiManager, RCTSparseArray *viewRegistry) {

    NSString *propertyName = [props objectForKey:RKPOPAnimationManagerProperties.property];
    if (!propertyName) {
      RCTLogError(@"Animation has no property to animate: %@", props);
    }
    POPAnimatableProperty *property = [POPAnimatableProperty propertyWithName:propertyName];

    static NSDictionary *animationsByType;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
      animationsByType = @{
        @(RKPOPAnimationTypeSpring): ^{return [POPSpringAnimation animation];},
        @(RKPOPAnimationTypeDecay): ^{return [POPDecayAnimation animation];},
        @(RKPOPAnimationTypeLinear): ^{return [POPBasicAnimation linearAnimation];},
        @(RKPOPAnimationTypeEaseIn): ^{return [POPBasicAnimation easeInAnimation];},
        @(RKPOPAnimationTypeEaseOut): ^{return [POPBasicAnimation easeOutAnimation];},
        @(RKPOPAnimationTypeEaseInEaseOut): ^{return [POPBasicAnimation easeInEaseOutAnimation];},
      };
    });

    POPPropertyAnimation *(^animationBlock)() = animationsByType[@(type)];
    if (!animationBlock) {
      RCTLogError(@"Unknown animation type: %zd", type);
      animationBlock = ^{return [POPBasicAnimation easeInEaseOutAnimation];};
    }

    POPPropertyAnimation *animation = animationBlock();
    animation.property = property;

    [props enumerateKeysAndObjectsUsingBlock:^(NSString *key, id jsonValue, BOOL *stop) {
      // Specially handled these keys above.
      if ([key isEqualToString:RKPOPAnimationManagerProperties.property] || [key isEqualToString:@"type"]) {
        return;
      }

      if ([RKPOPAnimationManager _propertyKeyIsPOPAnimationKey:key]) {
        id animationValue;
        if ([jsonValue isKindOfClass:[NSNumber class]]) {
          //
          // JSON number -> NSNumber
          //
          animationValue = jsonValue;

          // Make sure it's actually an NSNumber(float) or NSNumber(double); POPAnimation fails on NSNumber(int).
          animationValue = @([animationValue doubleValue]);
        } else if ([jsonValue isKindOfClass:[NSArray class]]) {
          NSArray *jsonArray = (NSArray *)jsonValue;
          if (jsonArray.count == 2) {
            //
            // JSON array -> NSValue(CGPoint)
            //
            CGPoint point = [RCTConvert CGPoint:jsonArray];
            animationValue = [NSValue valueWithCGPoint:point];
          } else if (jsonArray.count == 4) {
            //
            // JSON array -> NSValue(CGRect)
            //
            CGRect rect = [RCTConvert CGRect:jsonArray];
            animationValue = [NSValue valueWithCGRect:rect];
          } else {
            RCTLogError(@"Only two-dimensional (point) values supported for %@ of property: %@", key, propertyName);
            return;
          }

        }

        // Various POPAnimation methods, such as -setToValue:, throw if the value is of a type not supported for the animated property (for example, if the animated property is 'scaleXY', the toValue should be a CGPoint, never an NSNumber or some other type).  Catch and log errors instead of crashing.
        @try {
          [animation setValue:animationValue forKey:key];
        } @catch (NSException *exception) {
          RCTLogError(@"Error setting animation property \"%@\" to %@: %@", key, animationValue, exception);
        }
      } else {
        RCTLogError(@"Unknown animation property: %@", key);
      }
    }];

    _animationRegistry[animationTag] = animation;
  }];
}

RCT_EXPORT_METHOD(addAnimation:(NSNumber *)viewTag
                  withTag:(NSNumber *)animationTag
                  completion:(RCTResponseSenderBlock)callback)
{
  if (!animationTag || !viewTag) {
    RCTLogError(@"addAnimation requires both animationTag and viewTag, received (#%@, #%@)", animationTag, viewTag);
    return;
  }

  [_bridge.uiManager addUIBlock:^(RCTUIManager *uiManager, RCTSparseArray *viewRegistry) {
    UIView *view = viewRegistry[viewTag];
    POPAnimation *animation = _animationRegistry[animationTag];
    if (!view) {
      RCTLogError(@"addAnimation cannot find view with tag #%@, attempting to add animation:\n%@", viewTag, animation);
      return;
    }

    if (!animation) {
      RCTLogError(@"addAnimation cannot find animation with tag #%@, attempting to animate view %@ with tag #%@", animationTag, view, viewTag);
    }

    if (callback) {
      animation.completionBlock = ^(POPAnimation *anim, BOOL finished) {
        callback(@[@(finished)]);
      };
    }

    CALayer *layer = view.layer;
    [layer pop_addAnimation:animation forKey:[animationTag stringValue]];
  }];
}

RCT_EXPORT_METHOD(removeAnimation:(NSNumber *)viewTag
                  withTag:(NSNumber *)animationTag)
{
  if (!animationTag || !viewTag) {
    RCTLogError(@"removeAnimation requires both animationTag and viewTag, received (%zd, %zd)", animationTag, viewTag);
    return;
  }

  [_bridge.uiManager addUIBlock:^(RCTUIManager *uiManager, RCTSparseArray *viewRegistry) {
    UIView *view = viewRegistry[viewTag];
    // Simply getting the animation to verify that it exists.
    POPAnimation *animation = _animationRegistry[animationTag];
    if (!view) {
      RCTLogError(@"removeAnimation cannot find view with tag #%@, attempting to remove animation:\n%@", viewTag, animation);
      return;
    }
    if (!animation) {
      RCTLogError(@"removeAnimation cannot find animation with tag #%@, attempting to remove from view %@ with tag #%@", animationTag, view, viewTag);
    }

    CALayer *layer = view.layer;
    [layer pop_removeAnimationForKey:[animationTag stringValue]];
  }];
}

+ (BOOL)_propertyKeyIsPOPAnimationKey:(NSString *)propertyKey
{
  // These are RKAnimationProperties which correspond exactly to CGFloat or NSValue(CGPoint) properties of POPAnimation.  Specifying one of these in the animation properties means that it'll be set directly on the underlying POPAnimation.  This is for convenience so that -createAndRegisterAnimationWithTag:type:props: can parse various similar properties in the same way.
  static NSSet *POPAnimationPropertyKeys = nil;
  static dispatch_once_t onceToken;
  dispatch_once(&onceToken, ^{
    __unsafe_unretained NSString **objects = (__unsafe_unretained NSString **)(void *)&RKPOPAnimationManagerProperties;
    POPAnimationPropertyKeys = [NSSet setWithObjects:objects count:sizeof(RKPOPAnimationManagerProperties) / sizeof(*objects)];
  });
  return [POPAnimationPropertyKeys containsObject:propertyKey];
}

@end

@vishnevskiy
Copy link
Contributor Author

Awesome thx :) I just reimplemented the API and it worked for me but this is more complete than mine.

@JohnyDays
Copy link
Contributor

Thanks @ide, I did not know method calls to UIKit were limited to the main thread, very new to Obj-C and the Apple APIs(didn't actually know what UIKit was, had to check).

@vishnevskiy
Copy link
Contributor Author

@tadeuzagallo So I added this to my module.

- (dispatch_queue_t)methodQueue {
  return dispatch_get_main_queue();
}

The end result is the same. The weirder part is some of my animations do multiple invokes to addAnimation some of them play instantly and others wait till next interaction or just after sometime.

An example is fading in a backdrop and transforming a modal upwards.

@vishnevskiy
Copy link
Contributor Author

Actually using this from the FB implementation.

- (dispatch_queue_t)methodQueue {
  return _bridge.uiManager.methodQueue;
}

Solved the problem

@tadeuzagallo
Copy link
Contributor

Yes, that makes sense, sorry about that. I read the answer and forgot to check the actual queue, but the chain I explained remains the same. I'll update the answer accordingly for future references. :)

@ide
Copy link
Contributor

ide commented May 23, 2015

@tadeuzagallo - what I was thinking is that it is confusing that the UI blocks do not run until some other unrelated piece of code or a UI interaction causes the UIManager to flush the blocks.

To make this less surprising, here are two ideas: one idea is to schedule the the UI blocks to run ASAP (probably after the next vsync iteration as you suggested). The other idea is to print a warning message if -[UIManager addBlock:] is not called on the shadow queue. Either way, my hope is to help guide module authors in the right direction.

@nicklockwood
Copy link
Contributor

Asserting that [UIManager addBlock:] is called on the shadowQueue seems like a good solution.

tadeuzagallo added a commit to tadeuzagallo/react-native that referenced this issue May 28, 2015
…:] to _shadowQueue

Summary:
@public

Add `RCTAssertThread` to `RCTAssert.h` for convenience when checking the current/queue,
it accepts either a `NSString *`, `NSThread *` or `dispatch_queue_t` as the object to be checked

Also add a check to `-[RCTUIManager addUIBlock:]` - There was a discussion on github (facebook#1365)
due to the weird behavior caused by calling it from a different thread/queue (it might be added after `batchDidComplete` has been called
and will just be dispatched on the next call from JS to objc)

Test Plan:
Change `-[RCTAnimationExperimentalManager methodQueue]` to return `dispatch_get_main_queue()` and run the 2048 example,
it should dispatch with a helpful message (screenshot on the comments)
tadeuzagallo added a commit to tadeuzagallo/react-native that referenced this issue May 28, 2015
…:] to _shadowQueue

Summary:
@public

Add `RCTAssertThread` to `RCTAssert.h` for convenience when checking the current/queue,
it accepts either a `NSString *`, `NSThread *` or `dispatch_queue_t` as the object to be checked

Also add a check to `-[RCTUIManager addUIBlock:]` - There was a discussion on github (facebook#1365)
due to the weird behavior caused by calling it from a different thread/queue (it might be added after `batchDidComplete` has been called
and will just be dispatched on the next call from JS to objc)

Test Plan:
Change `-[RCTAnimationExperimentalManager methodQueue]` to return `dispatch_get_main_queue()` and run the 2048 example,
it should dispatch with a helpful message (screenshot on the comments)
@klvnptr
Copy link

klvnptr commented May 31, 2015

@nicklockwood when do you guys think @vjeux 's animation library will land?

@vjeux
Copy link
Contributor

vjeux commented Jun 1, 2015

@mcz: hopefully a month from now

@grabbou
Copy link
Contributor

grabbou commented Jun 3, 2015

@vjeux React Europe deadline? 🎯

@grabbou
Copy link
Contributor

grabbou commented Jun 3, 2015

On a side note - having something with a POP similar API would be very beneficial to those who are already familiar with your animation library when writing native apps in the old way!

@rainer-liao
Copy link

@tadeuzagallo Still, sometimes the UIBlock won't execute until the next user interaction after this function was called. I met this situation in iPhone6s, but iPhone6, iPhone6sp are fine.

@facebook facebook locked as resolved and limited conversation to collaborators May 29, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests