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

Better bbox reporting #2618

Closed
springmeyer opened this issue Dec 23, 2014 · 2 comments
Closed

Better bbox reporting #2618

springmeyer opened this issue Dec 23, 2014 · 2 comments
Assignees
Milestone

Comments

@springmeyer
Copy link
Member

We should implement @brandonreavis's suggestions at:
mapbox/mapnik-omnivore#43 (comment)

mapnik_projection.cpp

double minx = a->Get(0)->NumberValue();
double miny = a->Get(1)->NumberValue();
double maxx = a->Get(2)->NumberValue();
double maxy = a->Get(3)->NumberValue();
p->projection_->forward(minx,miny);
p->projection_->forward(maxx,maxy);
Local<Array> arr = NanNew<Array>(4);
arr->Set(0, NanNew(minx));
arr->Set(1, NanNew(miny));
arr->Set(2, NanNew(maxx));
arr->Set(3, NanNew(maxy));
NanReturnValue(arr);

should become something like this:

double ulx, ulx, urx, ury, lrx, lry, llx, lly;
ulx = llx = a->Get(0)->NumberValue();
lry = lly = a->Get(1)->NumberValue();
lrx = urx = a->Get(2)->NumberValue();
uly = ury = a->Get(3)->NumberValue();
p->projection_->forward(ulx,uly);
p->projection_->forward(urx,ury);
p->projection_->forward(lrx,lry);
p->projection_->forward(llx,lly);
Local<Array> arr = NanNew<Array>(4);
arr->Set(0, NanNew(ulx<llx ? ulx : llx));
arr->Set(1, NanNew(lry<lly ? lry : lly));
arr->Set(2, NanNew(urx>lrx ? urx : lrx));
arr->Set(3, NanNew(ury>uly ? ury : uly));
NanReturnValue(arr);
@flippmoke
Copy link
Member

@springmeyer just to be clear on the problem here, I wanted to draw what the current code does, and then consider what we should do going forward.

bbox_issue

If you reprojected epsg:4326 to epsg:3857 as a box in the northern hemisphere you might get a resulting box like the dashed blue area shown. The red points are the points we use from a currently transformed bounding box and the green is the new bounding box we form.

Our current implementation is bad, so we should switch to what he is suggesting, however, this MIGHT also be confusing to some people because you have the possibility to include data that you didn't expect to include in the first place with bounding boxes like this.

The fool proof solution would be track all 4 corners of a bounding box always, but this would complicate so much.

/cc @artemp

@flippmoke
Copy link
Member

Implemented fix in 90de0d6 and 3e24c5a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants