OpenLayers OpenLayers

Ticket #1341 (closed feature: fixed)

Opened 2 years ago

Last modified 2 years ago

panTo should use tween if new center is in the current bounds + a ratio

Reported by: pgiraud Assigned to: pagameba
Priority: critical Milestone: 2.7 Release
Component: Map Version: 2.5
Keywords: Cc:
State: Complete

Description

I think that the comparison with the current bbox to choose if the panning is animated or not is too strict. We should add a ratio to the bbox before we check if it contains the new center.

Attachments

Map.js.diff (1.0 kB) - added by sbenthall on 07/29/08 16:12:13.
Bounds.js.diff (1.4 kB) - added by sbenthall on 07/29/08 16:12:25.
Bounds.html.diff (1.1 kB) - added by sbenthall on 07/29/08 16:12:33.
scaledTweenPanning.patch (3.4 kB) - added by euzuro on 07/30/08 10:57:22.
this is all three of seb's patches bundled into one.

Change History

02/12/08 10:53:21 changed by euzuro

+1

07/04/08 02:34:45 changed by euzuro

  • milestone set to 2.7 Release.

07/04/08 08:00:47 changed by pagameba

what sort of ratio do you think would be aesthetically pleasing? Or should it be definable through map options and default to some reasonable value (1.25?)

07/28/08 11:44:55 changed by crschmidt

  • owner set to sbenthall.
  • priority changed from minor to critical.

07/28/08 11:45:26 changed by sbenthall

  • status changed from new to assigned.

07/29/08 16:12:13 changed by sbenthall

  • attachment Map.js.diff added.

07/29/08 16:12:25 changed by sbenthall

  • attachment Bounds.js.diff added.

07/29/08 16:12:33 changed by sbenthall

  • attachment Bounds.html.diff added.

07/29/08 16:14:43 changed by sbenthall

  • owner changed from sbenthall to euzuro.
  • status changed from assigned to new.
  • state set to Review.

Patches added. As part of the solution to this, added a "scale" method to the Bounds basetype that scales the bounds with a ratio and an origin (the origin defaults to center). Added tests for this method.

For the actual panning feature, there is a new property, panRatio, on the map (that can be set in an option).

Waiting for review.

07/29/08 17:41:05 changed by elemoine

  • state changed from Review to Commit.

sbenthall, just a comment related to the scale method in the OpenLayers.Bounds class. How about having: scale(ratio, x, y), scalePixel(ratio, pixel) and scaleLonLat(ratio, lonlat) with the last two being based on the first, to be in line with what's done with contains. What you think? It's up to you actually, your patches look to me anyway.

07/29/08 17:47:20 changed by pagameba

When a bounds object is in pixel space, using scale() may result in non-integral values of left, top, right and bottom - is this okay (using scalePixel and scale LonLat could avoid this)

07/30/08 10:56:10 changed by euzuro

  • state changed from Commit to Needs Discussion.

07/30/08 10:57:22 changed by euzuro

  • attachment scaledTweenPanning.patch added.

this is all three of seb's patches bundled into one.

08/01/08 12:56:41 changed by euzuro

  • owner changed from euzuro to pagameba.

pagameba -- i see you have some concerns with this patch. i'm assigning it to you... can you clarify what you think the issue is (and any proposed solutions you might have?)

08/01/08 13:22:25 changed by pagameba

my concern was that the scale function magically determines if the passed in object is a LonLat or Pixel but doesn't attempt to ensure that Pixel values are integral values after scaling. Browsers ignore fractional pixel values as far as I know, but may deal with them in different ways (rounding, floor, etc) so I think it would be better if we are scaling pixel values to be intentional about making sure they are integral values. But maybe I am being paranoid or stupid about this in some way? Right now this is only going to be used for scaling lonLat bounds as part of the test in the panTo function. I'm not convinced this would ever be used to scale Pixel values that would consequently be used to *position* something in the browser in which case my concerns are not very valid.

Does anyone else have an opinion on whether we should care about ensuring Pixel values are integers or not? If yes, I'll make the necessary changes to the patch. If not, it can go in as is as I doubt it will cause any immediate problems.

08/01/08 20:53:29 changed by crschmidt

  • state changed from Needs Discussion to Commit.

Paul,

This code creates a bounds by scaling a ratio around an origin, and the origin can be a bounds. I don't see any reason to be concerned about this. (Certainly, we make no promises about members of bounds being integers.) With that in mind, although I understand your concern, I don't think it's relevant. We should treat values where we set some browser display pixels based on a bounds object with care, but I don't think this changes that at all.

With that in mind, I'd say that although I understand your complaint, I don't think it's really relevant to this.

I think this patch is fine as is.

08/01/08 20:56:36 changed by crschmidt

  • status changed from new to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

(In [7678]) "panTo should use tween if new center is in the current bounds + a ratio". Add a bounds.scale method (takes a ratio and an optional center) and call it from the panTo to give a ratio we can pan inside of. Patch by sbenthall, r=me,elemoine (Closes #1341)