lguminski commented 4 years ago

Specify the revision number of the tween.js library where the bug occurred

Official release 17.4.0

Specify your browser version and operating system (i.e. Chrome 23.0.1271.95, Windows 7)

Chrome 83.0.4103.97 (Official Build) (64-bit) Mac OS

Describe the problem in detail. What happened? What did you expect to happen?

07_dynamic_to.html shows a situation when an object (fox) is tweened to a dynamic target (rabbit). It works, but this is a simple case when target is an absolute value. When the target is a dynamic array of 2 rabbits, then only the initial value of the array is taken into account.

Provide a small test case

Please take a look at https://jsfiddle.net/lguminski/9ahnf8u6/ . In the example fox chases 2 rabbits.

var rabbits = {
  x: [rabbit1.x, rabbit2.x],
  y: [rabbit1.y, rabbit2.y]
};

new TWEEN.Tween(fox).to(rabbits, 3000)
  .interpolation(TWEEN.Interpolation.CatmullRom)
  .start()

and each rabbit dynamically updates its position

new TWEEN.Tween(rabbit1).to({
  x: width - 50,
  y: height - 50
}, 3000).onUpdate(function(object) {
  rabbits.x[0] = object.x;
  rabbits.y[0] = object.y;
}).start();

new TWEEN.Tween(rabbit2).to({
  x: width - 50,
  y: height - 50
}, 3000).onUpdate(function(object) {
  rabbits.x[1] = object.x;
  rabbits.y[1] = object.y;
}).start();

Unfortunately fox does not take the changes into account, and visits only initial positions of rabbits.

This issue seems to be similar to #545

trusktr commented 4 years ago

Hello! Thanks for reporting this!

The issue in #545 happened in v18.6.0 and seems unrelated.

Did this work for you before?

trusktr commented 4 years ago

If this never worked before, it might be a new feature to add.

lguminski commented 4 years ago

Hello @trusktr , thank you for your feedback!

I am pretty sure it worked before., and indeed, after checking, 17.3.0 works fine (in my project I was using 17.2.0). Please check this demo:

https://jsfiddle.net/lguminski/zsmtd5jq/

So 17.4.0 must have broken it.

And let me note that the user guide doesn't state that dynamic target and array values cannot be combined. So I am literally following the guide.

trusktr commented 4 years ago

Ah, thanks for that demo. Indeed, anyone reading the docs can (and should) assume various features can be used at the same time in an intuitive way. Let's get this fixed.

!/JoePea

On Fri, Jul 3, 2020, 2:54 AM Lukasz Guminski notifications@github.com wrote:

Hello @trusktr https://github.com/trusktr , thank you for your feedback!

I am pretty sure it worked before., and indeed, after checking, 17.3.0 works fine. Please check this demo:

https://jsfiddle.net/lguminski/zsmtd5jq/

So 17.4.0 must have broken it.

And let me note that the user guide doesn't state that dynamic target and array values cannot be combined. So I am literally following the guide.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tweenjs/tween.js/issues/555#issuecomment-653460707, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACIVTTJJSH27V5VXNW4MZLRZWTHBANCNFSM4OPJ7STA .

lguminski commented 4 years ago

thanks @trusktr !

Tomorrow I will submit a PR with a new example, say 17_dynamic_to_an_array_of_values.html, to protect this feature against accidental removal.

trusktr commented 4 years ago

Sweet! We also need units tests for the old and new dynamic-to features. I've been adding unit tests for every change I've made plus existing features that I noticed weren't covered, but hadn't yet realize dynamic-to had no tests.

lguminski commented 4 years ago

@trusktr In PR https://github.com/tweenjs/tween.js/pull/556 you will find 2 unit tests for old dynamic-to

"Test TWEEN.Tween.to() with a dynamic target provided as object"
"Test TWEEN.Tween.to() with a dynamic target provided as array"

both are failing on master, and I suppose this is due to #554 . Which is good.

I created also a test for the new dynamic-to:

"Test TWEEN.Tween.to() with multiple dynamic targets provided as array"

Obviously it is failing as well.

In addition to that there is a new example 07b_dynamic_to_an_array_of_values.html. It presents a fox chasing rabbits, similar to the example that I already provided, just here every time it reaches a rabbit, the corresponding tween gets stopped. This does not change the total length of the target array, but gradually values in it stop changing (because rabbit tweens get stopped).

After I implemented this, I realized that this example could be implemented in another way. Instead of leaving last values of passed (away) rabbits in the array, the values could be entirely removed. This way the target array would get smaller and smaller with each rabbit passed, until the array is empty.

I know that this is probably harder to implement, but this would be more flexible. If the target array could shrink (and possibly grow as well), this would pave a way for new features in the future. For instance tweens that could be gradually fed with new values. Lets say, first you give to the fox 3 rabbits, then it stops after reaching the last, then you give another 5, it chases them and waits for more. Currently you need to create a new tween for every batch of rabbits, which means the tween does not know the past values. With a gradually-fed tween, it would remember the last position, so the movement could be smoother (with Bezier or Catmull–Rom interpolation). This thinking is beyond this feature but perhaps you find it useful. Just in case I submitted one more example (07c_dynamic_to_an_variable_length_array_of_values.html) to illustrate this idea.

PS. Please excuse me if there are any bugs in the tests and examples, but because master is currently broken, I could not test my code fully. But I hope it will be of help.

trusktr commented 4 years ago

This is great, thanks for the work on this, and sorry for the late reply! Let's get this in.

trusktr commented 4 years ago

Working on this now. Fixed #545 already.

The diff that causes the break is: https://github.com/tweenjs/tween.js/compare/cc2d4ea1799723f5fd30c58eab0b75883297b3cb..35d6cac39d3e175594680f9d02760f6496a48a3e

@mikebolt Ironically it was the introduction of this._valuesEnd = Object.create(properties) that broke dynamic_to with array values, which I restored in order to fix #545, but now we need to figure how to make it work with array values too.

trusktr commented 4 years ago

@lguminski I came up with a fix locally. Here's the plan:

release a patch bump for v17 to npm, which essentially reverses this breaking change in v17.
We'll leave v18 alone, and v18 will be considered the version that introduced the breaking change (it is already a major version bump).
Finally we'll release v19 which will change the signature of to() to have a new dynamic option: when true values of the to object can be modified during animation (and array values may be modified by to()), and when false then to() creates a copy internally and will never modify the original object that was passed in but because it has a copy the user code an not dynamically modify the passed-in object during animation.

I know that this is probably harder to implement, but this would be more flexible.

The above does not address the concept you mentioned of being able to remove values from the array, it merely restores the old behavior of v17 before the break, and then exposes both the old and new behavior behind a new dynamic option in v19, but otherwise both behaviors are the same as v17 and v18.

Regarding your dynamically-resized array idea, right now the arrays are handled during tween.start(), but to make things more dynamic like with your idea, it may

require handling arrays on each update (basically this is "polling") so that if the arrays changed, the change is caught during update, or
require handling arrays only when they've changed (this would need a notification system to be built into the implementation), which would prevent unnecessary overhead during update but would introduce more complexity into the code.

I believe option 1 was implemented at some point already (though never tested and no example ever made for it), but the code was refactored to make it handled only once in tween.start() so as not to incur a cost during animations.

Besides the technical details of how to implement it, I'm not completely clear on what the expected behavior should actually be. I mean, what would the end user of that feature want, exactly? How would it compare to, for example, just setting a new to() value every time the group of rabbits changes (f.e. each time an eaten rabbit is removed)? (Note I haven't tested calling to() to change the to values while a tween isPlaying, not sure if that works as expected)

trusktr commented 4 years ago

v17 has a fix released in v17.6.0.

A future breaking change (most likely v19) will make dynamic a boolean option. PR: https://github.com/tweenjs/tween.js/pull/585

lguminski commented 3 years ago

@trusktr I haven't been here for quite long. I am sorry about that.

I would suggest to step back and think from user perspective. So let's for a moment forget about a fox chasing a few rabbits. Instead let's just think again about one fox chasing one rabbit, as it is in the original examples/07_dynamic_to.html:

var rabbit = {x: width - 50, y: 50}
new TWEEN.Tween(rabbit).to({x: width - 50, y: height - 50}, 3000).start()

var fox = {x: 50, y: 50}
new TWEEN.Tween(fox).to(rabbit, 3000).start

According to documentation, arrays is just another way of expressing the same behavior. So there is nothing that prohibits making an array with just one rabbit.

var rabbit = {x: width - 50, y: 50}

var rabbitAsArray = {
  x: [rabbit.x],
  y: [rabbit.y]
};

new TWEEN.Tween(rabbit).to({
  x: width - 50,
  y: height - 50
}, 3000).onUpdate(function(object) {
   rabbitAsArray.x[0] = object.x;
   rabbitAsArray.y[0] = object.y;
}).start();

new TWEEN.Tween(fox).to(rabbitAsArray, 3000).start()

If you do it like this, you realize that array are just a different semantics to achieve the same outcome.

That's why I think that introducing a new dynamic flag for arrays might appear convenient from developer's perspective, but it would be hard to justify why user needs to use dynamic flag in array form, and not in the object form.

Therefore regarding

Regarding your dynamically-resized array idea, right now the arrays are handled during tween.start(), but to make things more dynamic like with your idea, it may

require handling arrays on each update (basically this is "polling") so that if the arrays changed, the change is caught during update, or

require handling arrays only when they've changed (this would need a notification system to be built into the implementation), which would prevent unnecessary overhead during update but would introduce more complexity into the code.

I would say go for 1. because this is what the original object version is doing. And if I was you I would not introduce any extra flags. Because objects work like this without any additional flags.

trusktr commented 3 years ago

Hello @lguminski. I think that makes sense. I will circle back soon to get back into the thought mode.

trusktr commented 1 year ago

Currently you need to create a new tween for every batch of rabbits, which means the tween does not know the past values. With a gradually-fed tween, it would remember the last position

Btw, tween.start() has a new arg that makes the tween start from the last known values: tween.start(performance.now(), true), so that idea you have will work now! (tween.startFromCurrentValues() is an alias for tween.start(undefined, true)) #522 is ideating an option to make starting with last-known values the default behavior without having to pass true to start calls.

trusktr commented 1 year ago

The PR has been merged into main and released in v20.0.0.

I decided it would be too much work to get the purely non-mutating implementation in place, and that it would be valuable to merge this now.

tweenjs / tween.js

Dynamic_to does not work with array values #555

!/JoePea