Ticket #1308 (closed feature: fixed)

Opened 5 years ago

Last modified 4 years ago

tween (animation) in OpenLayers

Reported by: pgiraud Owned by:
Priority: minor Milestone: 2.6 Release
Component: general Version: 2.5
Keywords: Cc:
State: Complete

Description

Some of the new features require a handler for animation tween. The following patch, including tests and and example, adds the new classes for tween and easing.

note : there may be some work left to be done with the licence.

Attachments

patch-1308-r5916-A0.diff Download (15.2 KB) - added by pgiraud 5 years ago.
patch-1308-r5926-B0.diff Download (12.7 KB) - added by pgiraud 5 years ago.
same patch with minor fixes and without the animated panning code
patch-1308-r5982-C0.diff Download (12.8 KB) - added by pgiraud 5 years ago.
tween.patch Download (14.9 KB) - added by crschmidt 5 years ago.
With licensing bits

Change History

Changed 5 years ago by pgiraud

  Changed 5 years ago by pgiraud

Paul (or other folks), I would be very happy if you can have a look at this. Chris, in your opinion, how do I have to deal with licence ?

  Changed 5 years ago by crschmidt

  • milestone set to 2.6 Release

follow-up: ↓ 4   Changed 5 years ago by pspencer

I think this is great stuff. The Tween class should be pretty easy to integrate into the animated zooming, reduce the code a bit, and provide some interesting options.

For this specific patch, I have a couple of comments:

  • there seems to be an extra button left over in the example.
  • I am concerned about the panTo method not being 're-entrant' ... perhaps panTo and setCenter should cancel any existing tween before doing their jobs?

in reply to: ↑ 3   Changed 5 years ago by pgiraud

Replying to pspencer:

I think this is great stuff. The Tween class should be pretty easy to integrate into the animated zooming, reduce the code a bit, and provide some interesting options. For this specific patch, I have a couple of comments: * there seems to be an extra button left over in the example.

Good catch, I'll correct this and probably add an new input to let user choose the duration of the animation.

* I am concerned about the panTo method not being 're-entrant' ... perhaps panTo and setCenter should cancel any existing tween before doing their jobs?

I totaly agree with that, but my mistake, the panTo shouldn't have been in this patch. I would like this to settle in #110 instead.

New patch to follow.

Changed 5 years ago by pgiraud

same patch with minor fixes and without the animated panning code

follow-up: ↓ 6   Changed 5 years ago by pgiraud

  • state set to Review

tests pass under FF almost all tests pass under IE6 : test_OSM doesn't but it doesn't with trunk for me either

in reply to: ↑ 5 ; follow-up: ↓ 7   Changed 5 years ago by elemoine

Replying to pgiraud:

tests pass under FF almost all tests pass under IE6 : test_OSM doesn't but it doesn't with trunk for me either

Pierre,

The code looks good to me!

A few comments:

  • The initialize method doesn't have the usual ND comment indicating that it is the constructor.
  • In the tween.html example you may want to call stop() on the previous tween object when changing the tween.
  • By the way, still in the tween.html example, why do you need to create a new tween object to change the easing? Reading your code, I think your could just do tween.easing = OpenLayers.Easing[transition][easing], couldn't you?
  • Even though I think the tween.html example is useful, I'm not sure it should be in the examples/ directory. To me the things in examples/ show the end-user how to use OpenLayers. The tween code doesn't belong the OpenLayers API.
  • The functions of the Easing objects are marked APIFunction. Do we really want these functions to be part of the OpenLayers API? (to me: no)

in reply to: ↑ 6   Changed 5 years ago by pgiraud

Thanks very much for this great review. I think I should have checked the code before I set this ticket's state to review.

* The initialize method doesn't have the usual ND comment indicating that it is the constructor.

Ok, and I completed/modified some other ND docs. Some properties are now marked as API.

* In the tween.html example you may want to call stop() on the previous tween object when changing the tween.

It looks like it a good choice. I don't think this is really needed.

* By the way, still in the tween.html example, why do you need to create a new tween object to change the easing? Reading your code, I think your could just do tween.easing = OpenLayers.Easing[transition][easing], couldn't you?

You seem to be right. Changed.

* Even though I think the tween.html example is useful, I'm not sure it should be in the examples/ directory. To me the things in examples/ show the end-user how to use OpenLayers. The tween code doesn't belong the OpenLayers API.

Agreed. I moved the tween.html example to tests/manual. Do you think this is a better idea ?

* The functions of the Easing objects are marked APIFunction. Do we really want these functions to be part of the OpenLayers API? (to me: no)

I mixed API/ non API with private/public, my bad. Fixed.

Everything is fixed in the upcoming patch. I think, this is now ready for a last review.

Changed 5 years ago by pgiraud

Changed 5 years ago by crschmidt

With licensing bits

  Changed 5 years ago by crschmidt

  • state changed from Review to Commit

Pierre:

I've reviewed this, and I'm ready for it to go to trunk. I've uploaded a patch which is exactly the same as yours, only with licensing bits added; please use this one, and go ahead to commit when ready.

  Changed 5 years ago by crschmidt

(In [6032]) Add tweening to sandbox (See #1308)

  Changed 5 years ago by pgiraud

  • status changed from new to closed
  • resolution set to fixed

fixed in r6097

  Changed 4 years ago by fredj

  • state changed from Commit to Complete
Note: See TracTickets for help on using tickets.