Multiple direction support

[felix-dumit]

#118

Added the ability to swipe in any direction.
Currently implemented directions are:

public enum SwipeResultDirection:String {
    case None
    case Left
    case Right
    case Up
    case Down
    case TopLeft
    case TopRight
    case BottomLeft
    case BottomRight
}

It could also be extended to another custom direction by specifying a direction point (the coordinate system has its origin at the center to the cardView).

Most drastic changes are:

• to determine the swiped direction it compares the current drag vector to the defined direction vectors and determines which direction the drag has the larges projection
• to determine the swipe percentage, it calculates the normalised distance between the center point of the card, and the border point of the card in the direction in question
  • Also changes had to be made to the animation code so it would be responsive to all direction drags - this is the part I'm least familiar with in the code so I'd advice special care when reviewing these changes

[AEugene]

Hi @felix-dumit . I'll review your code by the end of the week. Sorry for delay.

[felix-dumit]

@AEugene Any updates on this?
I saw you made some pretty big changes so now this branch has conflicts. How do we want to proceed?

[AEugene]

@felix-dumit Sorry for delay again, there were some urgent tasks. Could you update your branch with latest master? And then I'll provide review. Thank you for contribution.

[felix-dumit]

@AEugene Just updated with the changes from master

[AEugene]

Why do you multiple by 2 in normalisation? Also parameter called screenSize, but you're actually using bounds of DraggableCardView.

[felix-dumit]

Ok this function name was actually misleading, I added another function that performed the normalisation correctly.
What this one does is normalize a distance based on the screensize. It is multiplied by two because the normalized coordinate system goes from (-1, 1)

[AEugene]

@felix-dumit I've found a bug. It reproduces with horizontal swiping:
https://www.dropbox.com/s/fnmh686741yaane/HorizontalBug.mov?dl=0

Issue is in dragPercentage calculations. I tried this approach:

private var dragPercentage:CGFloat {
        // normalize dragDistance then convert project closesest direction vector
        let normalizedDragPoint = dragDistance.normalizedPointForSize(bounds.size)
        let swipePoint = normalizedDragPoint.scalarProjectionPointWith(dragDirection.point)

        // if point is outside rect, percentage of swipe in direction is over 100%
        if !SwipeResultDirection.boundsRect.contains(swipePoint) {
            return 1.0
        } else {
            let projectionPoint: CGPoint = directions.reduce(dragDirection.point) {
                if $0.distanceTo(normalizedDragPoint) > $1.point.distanceTo(normalizedDragPoint) {
                    return $1.point
                }

                return $0
            }

            return normalizedDragPoint.scalarProjectionWith(projectionPoint)
        }
    }

And it works great with horizontal and vertical swiping, but there is obvious jumping with directions set [.BottomLeft, .Left, .BottomRight, .Right]

I can't merge it now, because horizontal swiping is key functionality of Koloda.

And setting currentCardIndex requires more complex logic. For example when you try to set
koloda.changeCurrentCardIndex(koloda.currentCardIndex + 1)
All cards disappear event though next cards is on the screen now. Let's leave this functionality now and focus on multiple directions swiping.

Also there are some syntax issues(like space after colon, etc). I started refactoring Koloda, so let's keep it clean. We use these guidelines: https://swift.org/documentation/api-design-guidelines/

Thank you for contribution. I'm waiting for updated pull request :)

[felix-dumit]

I'll take a closer look on Monday, but I couldn't reproduce the issue from the video, I set those directions in the BackgroundAnimationViewController but it worked as expected. Can you verify that the swipePercentageMargin set in the delegate is < 1?
Because previously the delegate method returned the necessary drag distance and now it's the percentage of distance between center point and edge of card at drag direction.

Anyway thanks for review I'll update soon

[AEugene]

@felix-dumit It is reproducible with default threshold value. You can try it on first example(one with red background).

[felix-dumit]

@AEugene I was able to reproduce, you have to start dragging at the very center of the card either up or down then I get the issue.
I'll work on a fix

[felix-dumit]

@AEugene Ok I fixed the issue. It was because if the dragDistance was very small (rounded to CGPoint.zero) it would not intersect with any of the target lines so the dragPercentage would be CGFloat.infinity

I updated so that now it will return 0 if no instersections are found:

return rect.perimeterLines
            .flatMap { CGPoint.intersectionBetweenLines(targetLine, line2: $0) }
            .map { centerDistance / $0.distanceTo(.zero) }
            .minElement() ?? 0

[felix-dumit]

Should be ready for re-review now

[weyert]

This sounds great 👍

[AEugene]

could you name variable as "ratio" here?

[AEugene]

hi @felix-dumit . Code looks great, you did good job. Thank you for contribution.
I left a few comments. Also could you discard changes in following files:
CustomOverlayView.swift
ExampleOverlayView.swift
ViewController.swift
OverlayView.xib
Main.storyboard

Your code will be merged after these changes.
Our team will provide designs for example project (Up/Down swiping overlay, etc), and then I’ll integrate it into Koloda.

Also I’ve noticed finish percentage jumping with configuration [.BottomLeft, .Left, .BottomRight, .Right] on transition from left to bottom-left mode. But that’s minor issue.

[felix-dumit]

@AEugene Ok just pushed those changes, I had to leave the change from OverlayMode to SwipeResultDirection in CustomOverlayView.swift and ExampleOverlayView.swift.

[briansunter]

@felix-dumit I'm testing your branch and I'm getting some weird behaviour when using.

 func koloda(koloda: KolodaView, allowedDirectionsForIndex index: UInt) -> [SwipeResultDirection]{
        return [SwipeResultDirection.None]
    }

The card moves slightly then "falls" off the screen.

Better control of allowed directions would be a great feature though. Thanks for working on this.

[felix-dumit]

@brsunter yeah that's kind of undefined behaviour for that scenario as it doesn't make much sense to use it. If you don't want to allow any directions just return an empty array.

Maybe there could be an assert preventing your example. Or even remove the None option as I'm not sure it's even serving any purpose at the moment, wherever it's used we could just use an optional instead. I think this only really becomes relevant if the allowedDirections = [].
What do you think @AEugene ?

[briansunter]

I think removing .Nonewould be best since it also prevents [.None, .Down] etc. Not sure about Optional vs[] though

[AEugene]

@felix-dumit @brsunter Yup, I think we should remove .None and make allowedDirections optional

    func koloda(koloda: KolodaView, allowedDirectionsForIndex index: UInt) -> [SwipeResultDirection]?

[felix-dumit]

@AEugene I think we don't need to make allowedDirections optional as users can still return an empty array. The dragDirection property in DraggableCardView will have to be made optional.

[AEugene]

@felix-dumit Hm, maybe you're right. It will complicate the logic, because we still need to handle empty array of directions. Also we need to handle nil direction when we update overlay state. Could you implement it?

[felix-dumit]

@AEugene Ok I just pushed that, tell me what you think.

[AEugene]

Shouldn't we handle nil direction to update overlay image?

[felix-dumit]

Yes, you're right, I just fixed this.

[AEugene]

@felix-dumit I merged your request. Thanks for contribution.

[felix-dumit]

@AEugene Great!