Minimize sequence unwrapping and instantiating

[cypherdare]

I started to do a PR for this but it causes an existing test to fail, so I think it needs discussion.

IMO, SequenceAction.then should not unwrap its children if it can be avoided. Consider:

val sequence = moveTo(1f, 2f) + moveTo(3f, 4f) + moveTo(5f, 6f) + moveTo (7f, 8f)

The first operation creates a SequenceAction. The second operation immediately unwraps that sequence and creates a new sequence to put the same two original actions in. Then the third operation unwraps that sequence to transfer three actions.

So a simple sequence of 4 actions results in 3 SequenceActions instantiated and has transferred actions 5 times.

If we do this:

infix fun Action.then(action: Action): SequenceAction {
  if (this is SequenceAction) {
      this.addUnwrapped(action)
      return this
    }
  val result = Actions.sequence()
  result.addUnwrapped(this)
  result.addUnwrapped(action)
  return result
}

Only one SequenceAction is instantiated and no actions have to be unwrapped, no matter how long the chain of actions is. However, this fails the test should not mutate multiple actions with then, which raises the question of, Why shouldn't it mutate if it can avoid all the extra memory churn? Why is it better to throw away a SequenceAction than to mutate it?

Incidentally, I think addUnwrapped should also free the SequenceActions that it throws away, since they are no longer useful and their reference is likely to be lost. Like this:

private fun SequenceAction.addUnwrapped(action: Action) {
  when (action) {
    is SequenceAction -> {
      action.actions.forEach(::addAction)
      action.pool?.free(action)
    }
    else -> addAction(action)
  }
}

[czyzby]

I believe that's how it was implemented at first, but we eventually decided against it, since + operator in Kotlin should not mutate either of the operands. See #172 #174

[cypherdare]

Ah, OK. Then I would suggest we change then() back to the way it was and replace + with +=. Otherwise, it would be poor practice to ever use either of these if you don't like your game stuttering.

…

On Wed, Aug 21, 2019 at 3:07 AM MJ ***@***.***> wrote: I believe that's how it was implemented at first, but we eventually decided against it, since + operator in Kotlin should not mutate either of the operands. See #172 <#172> #174 <#174> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#206?email_source=notifications&email_token=AAOB7O3JDJXDGBHD5MOG5HLQFTSUNA5CNFSM4IN7KKVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4YVOJY#issuecomment-523327271>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOB7OYYTAMGU6UKKIPPXJDQFTSUNANCNFSM4IN7KKVA> .

[czyzby]

Otherwise, it would be poor practice to ever use either of these if you don't like your game stuttering.

Depends on the amount of actions that you use and the target device. Unless you're spawning thousands of sequence actions, creating a few extra short-lived objects here and there should not be an issue. Modern JVMs are rather powerful. On desktop this is unlikely to be a problem. On mobile - well, you can deal with it if it becomes an issue.

Also, having a non-mutating variant is useful when you don't want to modify the sequence actions. Even the garbage collection aside, there could be use cases for that alone.

That being said, I can see why providing a mutable variant would be beneficial. Would you like to submit a PR with then changes? We could also provide similar variant for parallelTo, but I honestly can't think of a name. I guess we could go with / for the non-mutating variant (similarly to + operator) and a mutating parallelTo, but I'm not sure this API would be obvious. Even with + and then you'd have to read the documentation to know the difference.

[cypherdare]

I see what you mean about performance. A few years ago, generating a few dozen objects worth of garbage would get you a noticeable GC stutter on a mid-range phone. I've used Actions as easy tween managers before, and in those projects I'd be creating many sequences. Of course, phone performance has advanced very quickly so maybe it's no longer an issue unless the amount of actions created is extreme.

I agree it would be nice to have options, and it makes sense for + to be non-mutating and then to be mutating.

Maybe for ParallelAction a nice infix function name would be along.

I'll play around with it a bit the next few days. We have to be careful with functions on ParallelAction since SequenceAction is weirdly extended from it. SequenceAction.plusAssign() and SequenceAction.addUnwrapped() are both currently redundant. And if I add ParallelAction.along() as a mutating function, it may need to be careful about not acting like then when the caller is a SequenceAction. Probably the solution is to also define a SequenceAction.along() that wraps the caller and parameter in a ParallelAction.

[czyzby]

You can see Git history to check how it was implemented before the non-mutating variants were introduced. addUnwrapped is just a local utility, AFAIR. You can remove it during the refactoring.

I think the parallel and sequence actions API should be consistent. We could stick with operators being non-mutating, and their more verbose variants being mutating. Otherwise there is no obvious distinction or reason why parallelTo or along would mutate the objects or not. / might not be the most obvious operator, but I think it would benefit the API consistency in general.

By the way, I really like the name along. I'm considering deprecating parallelTo and pointing to along instead.

[cypherdare]

I found a serious bug while experimenting with this. I'll include the fix in my PR (should be today or tomorrow). Since SequenceAction is a ParallelAction, ParallelAction.addUnwrapped() unwraps SequenceActions as if they are fellow ParallelActions. The following test fails.

  @Test
  fun `should not unwrap sequences with parallelto`() {
    val firstSequence = SequenceAction(MockAction(), MockAction())
    val secondSequence = SequenceAction(MockAction(), MockAction())

    val parallel = firstSequence parallelTo secondSequence

    assertTrue(firstSequence in parallel.actions)
    assertTrue(secondSequence in parallel.actions)
    assertEquals(2, parallel.actions.size)
  }

[czyzby]

I'd expect these to have a common superclass, but not extend one another. Thanks for the fix, I'll review the PR this week.

[czyzby]

I'm also not sure if we should unwrap actions in the first place. It seems like it would be a really rare occurrence in practice with hardly any benefits, and might lead to some weird bugs (e.g. someone keeps a reference to a sub-sequence to track it for whatever reason, and it ends up being cleared and freed immediately). What do you think, @cypherdare?

[cypherdare]

Yeah, I had thought of that. In my opinion, non-mutating should just wrap them both every time so it's not causing the original objects to be useless. Also, they'll get picked up by their pools when they are run. Let me know what you think and I can update it.

Do we care that there's a ParallelAction.div(), but instead of ParallelAction.divAssign() it's ParallelAction.plusAssign()? My opinion is to leave it. If there is a ParallelAction.divAssign(), then what should happen when its called on a SequenceAction?

I'm thinking I'm going to PR libGDX to break up the bug-prone hierarchy.

[czyzby]

In my opinion, non-mutating should just wrap them both every time so it's not causing the original objects to be useless.

Agreed, I'd appreciate if you update the PR.

ParallelAction.divAssign()

When you override /, you get a default implementation of /= calling = for free, although you have to do it on a var, as it is a simple reassignment. The difference is that += would be mutating. += is cleaner as far as adding a new action to the group in my opinion, we can leave it as is.

I'm thinking I'm going to PR libGDX to break up the bug-prone hierarchy.

Good idea.

[czyzby]

Thank you for your contribution, @cypherdare. Snapshot release with your changes will be available shortly. I might also release 1.9.10-b2 pretty soon, since there were 2 separate bug fixes and notable dependencies updates.

[cypherdare]

Sure, and thank you for the library!