Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a constrainCenter option to ol.View #2777

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@elemoine
Copy link
Member

commented Sep 30, 2014

IThis is unfinished work. Probably needs discussions, tests and a better example. I'm just creating this PR for people to be able track progress on this. See this post and this post on the mailing list for related discussions.

EDIT: and this question on StackOverflow.

@elemoine elemoine changed the title Add a constrainCenter option to ol.View (WIP) Add a constrainCenter option to ol.View Nov 30, 2014

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2014

This is now ready for review.

This PR adds a constrainCenter option to ol.View. This option references a function that takes a non-constrained center and returns a constrained center.

The PR also includes a restricted-extent example that shows how to restrict the extent of a map, similarly to OL2's restrictedExtent map option.

In the future, if possible, the library could provide more convenience. The constrainCenter option provides flexibility.

Thanks for any review.

EDIT: the restricted-extent example is available at http://erilem.net/ol3/constrain-center/examples/restricted-extent.html.

<h4 id="title">Restricted extent example</h4>
<p id="shortdesc">This example shows how to restrict the extent of a map.</p>
<div id="docs">
<p>Here the map is restricted to the France extent. The map view is configured with a <code>maxZoom</code> (<code>minResolution</code> could also be used) and a specific <code>constrainCenter</code> function. Optionally, a <code>change:resolution</code> listener may be used to use a constrained center when the resolution changes. See the <a href="restricted-extent.js" target="_blank">restricted-extent.js source</a> to see how this is done.</p>

This comment has been minimized.

Copy link
@tschaub

tschaub Dec 1, 2014

Member

This should read "the extent of France" rather than "the France extent."

Upon first read, I'm left confused by "a change:resolution listener may be used to use a constrained center when the resolution changes." I'll see if reading the code makes things clearer.

var maxExtent = [
-546677.6272683458, 5244890.488572459,
920913.3158070382, 6516802.639237792];
// This is the "constrain center" function. It constrains the center

This comment has been minimized.

Copy link
@tschaub

tschaub Dec 1, 2014

Member

I think this example would be easier to read if maxExtent were defined outside this IIFE. Then there would be no need for the IIFE.

* The *center constraint* is determined by the `extent` option. By
* default the center is not constrained at all.
* The *center constraint* is determined by the following options: `extent` and
* `constrainCenter`. By default the center is not constrained at all.

This comment has been minimized.

Copy link
@tschaub

tschaub Dec 1, 2014

Member

This doesn't make it clear what the relationship is (if any) between the extent option and the constrainCenter option.

It would be clearer to say that the view's center is constrained if either the constrainCenter or extent option is provided (with the constrainCenter option taking precedence).

I know your change doesn't change the behavior of the extent option, but it would be nice to have its behavior better documented so people could understand when constrainCenter should be used instead.

var mapSize = /** @type {ol.Size} */ (map.getSize());
var viewResolution = map.getView().getResolution();
var mapHalfWidth = (mapSize[0] * viewResolution) / 2.0;
var mapHalfHeight = (mapSize[1] * viewResolution) / 2.0;

This comment has been minimized.

Copy link
@tschaub

tschaub Dec 1, 2014

Member

Not necessarily your responsibility here, but it would be nice to make it easy to have this work with rotated maps as well.

@tschaub

This comment has been minimized.

Copy link
Member

commented Dec 1, 2014

Perhaps it is outside the scope for this change, but it strikes me that we could provide better handling of center constraints during resolution changes. At a minimum, I think the example added here should say "by default, the center constraint is not considered during resolution changes" or something to that effect (as motivation for adding the change:resolution listener).

If we did respect center constraints during resolution changes, we could do so during an animated resolution change. In the end, I think this would provide nicer behavior than the current example behavior (where the center jumps to the constrained value before the animation begins).

Curious to hear your opinion on this @elemoine.

A few documentation related comments above, but this looks like a good code change to me.

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2014

Thanks for the good review comments @tschaub. I made changes for your "easy" comments.

I left the change:resolution listener in the example but I agree that the non-animated center changes when the resolution changes are not very nice. I tried to use ol.animation.pan in the change:resolution listener to animate center changes, but I then ran into other problems.

I'll try to modify the zoom interactions and controls to constrain the center when the resolution changes…

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2014

There's now a lot more changes than in the initial patch. The code and commit history still need cleaning up. The new behavior can be tested on http://erilem.net/ol3/constrain-center/examples/restricted-extent.html.

@tschaub

This comment has been minimized.

Copy link
Member

commented Dec 3, 2014

Looks really nice @elemoine. Thanks for tackling the extra work - I think it is a significant usability improvement.

As one additional developer nicety, I'd be inclined to track down where we are calling the center constraint function with undefined values and avoid that (if possible). But that could be handled separately.

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2014

Was thinking about the same!

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2014

@tschaub I added a FIXME in the example to mention that the constrainCenter function does not correctly handle the case of rotated maps. I also added e0abf0a to fix an issue on touch devices. This is a bit of a hack but I haven't found a better way. I wouldn't say this is the perfect patch, but that's what I've been able to come up with at this point. I'd say that we can merge this, but I'll wait for your final feedback. Thanks.

@tschaub

This comment has been minimized.

Copy link
Member

commented Dec 4, 2014

Looks good to me.

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2014

Really? :-) The things I don't like:

  • The constrain center function has two args (the non-constrained center and the target resolution) while the constrain resolution and rotation functions have only one (the non-constrained value). This is not such a big deal, as we do not currently provide a way for users to provide constrain resolution and rotation functions, but still...
  • The hack in the last commit. With two fingers on the screen you can simultaneously "pinch zoom", "pinch rotate" and "drag pan" the map. While pinch zooming/rotating no resolution/rotation constraint is applied. So if you apply the center constrain while drag panning, and if the constrain center function uses the (non-constrained) resolution for the caculation of the constrained center you may end up with ugly map jumps while pinch zooming. The hack is to not call the constrain center function while drag panning when more than pointers are detected on the screen. Not such a terrible hack, but still...
  • The FIXME in the example. I should come up with a function that works when the view is rotated as well. We could provide a factory that returns a constrain center function that restricts the extent. But that factory would need to take the map as an argument, which would bind the view to the map again...

@tschaub tschaub modified the milestone: v3.2.0 Dec 22, 2014

@elemoine elemoine modified the milestones: v3.3.0, v3.2.0 Feb 2, 2015

@tremby

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2015

For anyone waiting on this, I posted a temporary solution of sorts to the Stackoverflow question mentioned in the OP.

@ahocevar ahocevar modified the milestones: v3.4.0, v3.3.0 Mar 3, 2015

@bartvde bartvde modified the milestones: v3.5.0, v3.4.0 Mar 29, 2015

@tschaub tschaub removed this from the v3.5.0 milestone May 5, 2015

@Honigbaum

This comment has been minimized.

Copy link

commented Jul 20, 2015

Is there any chance that the pull request gets merged in one of the next versions?

@bartvde bartvde added this to the v3.8.0 milestone Jul 30, 2015

@tschaub

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

@elemoine can you resurrect this (rebase, resolve conflicts, and merge if you like it)?

@tschaub tschaub modified the milestone: v3.8.0 Aug 4, 2015

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2015

I'll try to go back to this for the next release.

@elemoine elemoine self-assigned this Aug 7, 2015

@ahocevar

This comment has been minimized.

Copy link
Member

commented Oct 8, 2015

@elemoine, just a gentle reminder that you wanted to go back to this. If you don't have much time, it'd also be nice if you could just update what's here to remove conflicts. Your remaining concerns could be addressed in a separate pull request.

@elemoine

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2015

I've just rebased this. But this cannot be merged as-is. The example doesn't work correctly. You'll observe jumps when the view is at minZoom and you try to zoom out again (with the mousewheel or the zoom control's minus button).

markathomas added a commit to markathomas/ol3 that referenced this pull request Nov 23, 2015

@tbarsballe

This comment has been minimized.

Copy link
Contributor

commented May 6, 2016

Is this still going anywhere?
I recently build a custom restricted extent implementation for an OL3 project I was working on, and was wondering if I would be better off polishing it up and contribuiting it seperately?
It seems like restricted extent is one portion of this PR, but not the main goal.

@ahocevar

This comment has been minimized.

Copy link
Member

commented May 7, 2016

@tbarsballe Thanks for asking. Currently, the extent option on ol.View creates a center constraint. This means that you will be able to pan the map to any place where the center is within the specified extent. If we had an additional boolean restrictExtent (or whatever a good name for it would be) option, the configured extent could be used to restrict panning and zooming to places where the whole viewport is within that extent. Is this along the lines of what you had in mind with your implementation?

I think the challenge with implementing this properly is restricting the zoom properly, and creating a nice user experience when zooming out an area that will not be fully within the constrained extent any more after zooming out.

That said, we'd gladly accept a contribution that is able to address this properly.

@tremby

This comment has been minimized.

Copy link
Contributor

commented May 8, 2016

It seems to me that things would get very tricky as soon as the map is rotated! But if it can be made to work, it'd certainly be a very useful feature.

@ahocevar

This comment has been minimized.

Copy link
Member

commented May 8, 2016

Yes, rotation will be equally challenging as the zoom user experience I outlined above.

@tbarsballe

This comment has been minimized.

Copy link
Contributor

commented May 9, 2016

@ahocevar Yes, that was aproximately what I had in mind, although I was thinking of restrictedExtent as its own ol.Extent value, not neccessarily the same as the extent of the view. My solution for zoom was to have a custom zoom constraint that does not allow zooming outside of the bounds of the extent.

One requirement for both these constraints is that the view needs to be aware of the element it is getting drawn in (in the same way that Map has a target parameter) in order to determine the size and aspect ratio of the actual view.

I had not considered how rotation would act with this, and don't really have a good solution. I feel like rotation would have to be mutually exclusive with restricted extent.

@ahocevar

This comment has been minimized.

Copy link
Member

commented May 10, 2016

@tbarsballe:

I was thinking of restrictedExtent as its own ol.Extent value, not neccessarily the same as the extent of the view.

Where would it make sense to keep those separate? Or in other words: what would be a use case for having a larger restrictedExtent than extent? Because in the opposite case, when restrictedExtent is smaller than extent, you would never reach the bounds of extent.

My solution for zoom was to have a custom zoom constraint that does not allow zooming outside of the bounds of the extent.

That sounds good. And even better if the restrictExtent option can influence the default zoom constraint so it does this.

One requirement for both these constraints is that the view needs to be aware of the element it is getting drawn in (in the same way that Map has a target parameter) in order to determine the size and aspect ratio of the actual view.

To constrain center, resolution and rotation, ol.View#constrain* functions are called by ol.interaction.Interaction instances, which know about the map and can use ol.Map#getSize to get the size of the view. So these constrain* functions could be called with the size as additional argument.

I had not considered how rotation would act with this, and don't really have a good solution. I feel like rotation would have to be mutually exclusive with restricted extent.

It could work similar to the way you handle zoom - with a rotation constraint that only allows rotations where the viewport wouldn't exceed the restricted extent.

@samimussbach

This comment has been minimized.

Copy link

commented Jul 3, 2016

Any news on this issue? Unfortunately I am not able to help with code.

@0xBADDCAFE

This comment has been minimized.

Copy link

commented Jul 6, 2016

This PR works like this example (on ol2)?

http://dev.openlayers.org/examples/restricted-extent.html

And I check this issue #1243 but it not correct because above example (restrictedExtent in ol2) restrict drag area but in this issue (extent in ol3) couldn't restrict drag area correctly. I want to restrict drag area.

@klickagent

This comment has been minimized.

Copy link

commented Apr 16, 2017

Any news on this issue yet? Looking forward to this functionality. Thanks a lot!

@stale

This comment has been minimized.

Copy link

commented May 22, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.