From: Anton Khorev Date: Sun, 5 Jan 2025 11:10:20 +0000 (+0300) Subject: Support unwrapped bbox values in changeset history queries X-Git-Tag: live~51^2 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/2b4eb3fde9c39176bff83e86b3a816619e4469ab?ds=sidebyside Support unwrapped bbox values in changeset history queries --- diff --git a/app/assets/javascripts/index/history.js b/app/assets/javascripts/index/history.js index 1a23ac7b1..c35aaf288 100644 --- a/app/assets/javascripts/index/history.js +++ b/app/assets/javascripts/index/history.js @@ -220,7 +220,7 @@ OSM.History = function (map) { const neClamped = crs.unproject(crs.project(ne)); if (sw.lat >= swClamped.lat || ne.lat <= neClamped.lat || ne.lng - sw.lng < 360) { - data.set("bbox", map.getBounds().wrap().toBBoxString()); + data.set("bbox", map.getBounds().toBBoxString()); } } diff --git a/app/controllers/changesets_controller.rb b/app/controllers/changesets_controller.rb index 5e79f4da6..07e7a365f 100644 --- a/app/controllers/changesets_controller.rb +++ b/app/controllers/changesets_controller.rb @@ -54,7 +54,10 @@ class ChangesetsController < ApplicationController changesets.where("false") end elsif @params[:bbox] - changesets = conditions_bbox(changesets, BoundingBox.from_bbox_params(params)) + bbox_array = @params[:bbox].split(",").map(&:to_f) + raise OSM::APIBadUserInput, "The parameter bbox must be of the form min_lon,min_lat,max_lon,max_lat" unless bbox_array.count == 4 + + changesets = conditions_bbox(changesets, *bbox_array) elsif @params[:friends] && current_user changesets = changesets.where(:user => current_user.followings.identifiable) elsif @params[:nearby] && current_user @@ -113,21 +116,37 @@ class ChangesetsController < ApplicationController #------------------------------------------------------------ ## - # if a bounding box was specified do some sanity checks. # restrict changesets to those enclosed by a bounding box - def conditions_bbox(changesets, bbox) - if bbox - bbox.check_boundaries - bbox = bbox.to_scaled - - changesets.where("min_lon < ? and max_lon > ? and min_lat < ? and max_lat > ?", - bbox.max_lon.to_i, bbox.min_lon.to_i, - bbox.max_lat.to_i, bbox.min_lat.to_i) - else + def conditions_bbox(changesets, min_lon, min_lat, max_lon, max_lat) + db_min_lat = (min_lat * GeoRecord::SCALE).to_i + db_max_lat = (max_lat * GeoRecord::SCALE).to_i + db_min_lon = (wrap_lon(min_lon) * GeoRecord::SCALE).to_i + db_max_lon = (wrap_lon(max_lon) * GeoRecord::SCALE).to_i + + changesets = changesets.where("min_lat < ? and max_lat > ?", db_max_lat, db_min_lat) + + if max_lon - min_lon >= 360 + # the query bbox spans the entire world, therefore no lon checks are necessary changesets + elsif db_min_lon <= db_max_lon + # the normal case when the query bbox doesn't include the antimeridian + changesets.where("min_lon < ? and max_lon > ?", db_max_lon, db_min_lon) + else + # the query bbox includes the antimeridian + # this case works as if there are two query bboxes: + # [-180*SCALE .. db_max_lon], [db_min_lon .. 180*SCALE] + # it would be necessary to check if changeset bboxes intersect with either of the query bboxes: + # (changesets.min_lon < db_max_lon and changesets.max_lon > -180*SCALE) or (changesets.min_lon < 180*SCALE and changesets.max_lon > db_min_lon) + # but the comparisons with -180*SCALE and 180*SCALE are unnecessary: + # (changesets.min_lon < db_max_lon) or (changesets.max_lon > db_min_lon) + changesets.where("min_lon < ? or max_lon > ?", db_max_lon, db_min_lon) end end + def wrap_lon(lon) + ((lon + 180) % 360) - 180 + end + ## # eliminate empty changesets (where the bbox has not been set) # this should be applied to all changeset list displays diff --git a/test/controllers/changesets_controller_test.rb b/test/controllers/changesets_controller_test.rb index 914f7f651..007a5d679 100644 --- a/test/controllers/changesets_controller_test.rb +++ b/test/controllers/changesets_controller_test.rb @@ -117,6 +117,127 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest check_index_result(changesets) end + def test_index_bbox_across_antimeridian_with_changesets_close_to_antimeridian + west_of_antimeridian_changeset = create(:changeset, :num_changes => 1, :bbox => [176, 0, 178, 1]) + east_of_antimeridian_changeset = create(:changeset, :num_changes => 1, :bbox => [-178, 0, -176, 1]) + + get history_path(:format => "html", :list => "1") + assert_response :success + check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset]) + + # negative longitudes + get history_path(:format => "html", :list => "1", :bbox => "-190,-10,-170,10") + assert_response :success + check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "-183,-10,-177,10") + assert_response :success + check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "-181,-10,-177,10") + assert_response :success + check_index_result([east_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "-183,-10,-179,10") + assert_response :success + check_index_result([west_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "-181,-10,-179,10") + assert_response :success + check_index_result([]) + + # positive longitudes + get history_path(:format => "html", :list => "1", :bbox => "170,-10,190,10") + assert_response :success + check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "177,-10,183,10") + assert_response :success + check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "177,-10,181,10") + assert_response :success + check_index_result([west_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "179,-10,183,10") + assert_response :success + check_index_result([east_of_antimeridian_changeset]) + + get history_path(:format => "html", :list => "1", :bbox => "179,-10,181,10") + assert_response :success + check_index_result([]) + end + + def test_index_bbox_across_antimeridian_with_changesets_around_globe + changeset1 = create(:changeset, :num_changes => 1, :bbox => [-150, 40, -140, 50]) + changeset2 = create(:changeset, :num_changes => 1, :bbox => [-30, -30, -20, -20]) + changeset3 = create(:changeset, :num_changes => 1, :bbox => [10, 60, 20, 70]) + changeset4 = create(:changeset, :num_changes => 1, :bbox => [150, -60, 160, -50]) + + # no bbox, get all changesets + get history_path(:format => "html", :list => "1") + assert_response :success + check_index_result([changeset4, changeset3, changeset2, changeset1]) + + # large enough bbox within normal range + get history_path(:format => "html", :list => "1", :bbox => "-170,-80,170,80") + assert_response :success + check_index_result([changeset4, changeset3, changeset2, changeset1]) + + # bbox for [1,2] within normal range + get history_path(:format => "html", :list => "1", :bbox => "-160,-80,0,80") + assert_response :success + check_index_result([changeset2, changeset1]) + + # bbox for [1,4] containing antimeridian with negative lon + get history_path(:format => "html", :list => "1", :bbox => "-220,-80,-100,80") + assert_response :success + check_index_result([changeset4, changeset1]) + + # bbox for [1,4] containing antimeridian with positive lon + get history_path(:format => "html", :list => "1", :bbox => "100,-80,220,80") + assert_response :success + check_index_result([changeset4, changeset1]) + + # large enough bbox outside normal range + get history_path(:format => "html", :list => "1", :bbox => "-220,-80,220,80") + assert_response :success + check_index_result([changeset4, changeset3, changeset2, changeset1]) + end + + ## + # Test that -180..180 longitudes don't result in empty bbox + def test_index_bbox_entire_world + changeset = create(:changeset, :num_changes => 1, :bbox => [30, 60, 31, 61]) + + get history_path(:format => "html", :list => "1", :bbox => "-180,-80,-180,80") + assert_response :success + check_index_result([]) + + get history_path(:format => "html", :list => "1", :bbox => "180,-80,180,80") + assert_response :success + check_index_result([]) + + get history_path(:format => "html", :list => "1", :bbox => "-180,-80,180,80") + assert_response :success + check_index_result([changeset]) + end + + ## + # Test that -270..270 longitudes don't result in 90..-90 bbox + def test_index_bbox_larger_than_entire_world + changeset1 = create(:changeset, :num_changes => 1, :bbox => [30, 60, 31, 61]) + changeset2 = create(:changeset, :num_changes => 1, :bbox => [130, 60, 131, 61]) + + get history_path(:format => "html", :list => "1", :bbox => "-90,-80,90,80") + assert_response :success + check_index_result([changeset1]) + + get history_path(:format => "html", :list => "1", :bbox => "-270,-80,270,80") + assert_response :success + check_index_result([changeset2, changeset1]) + end + ## # Checks the display of the user changesets listing def test_index_user diff --git a/test/system/history_test.rb b/test/system/history_test.rb index 50215abc5..cb3647354 100644 --- a/test/system/history_test.rb +++ b/test/system/history_test.rb @@ -152,6 +152,30 @@ class HistoryTest < ApplicationSystemTestCase end end + test "changesets at both sides of antimeridian are listed" do + user = create(:user) + PAGE_SIZE.times do + create(:changeset, :user => user, :num_changes => 1, :bbox => [176, 0, 178, 1]) do |changeset| + create(:changeset_tag, :changeset => changeset, :k => "comment", :v => "West-of-antimeridian-changeset") + end + create(:changeset, :user => user, :num_changes => 1, :bbox => [-178, 0, -176, 1]) do |changeset| + create(:changeset_tag, :changeset => changeset, :k => "comment", :v => "East-of-antimeridian-changeset") + end + end + + visit history_path(:anchor => "map=6/0/179") + + within_sidebar do + assert_link "West-of-antimeridian-changeset", :count => PAGE_SIZE / 2 + assert_link "East-of-antimeridian-changeset", :count => PAGE_SIZE / 2 + + click_on "Older Changesets" + + assert_link "West-of-antimeridian-changeset", :count => PAGE_SIZE + assert_link "East-of-antimeridian-changeset", :count => PAGE_SIZE + end + end + private def create_visible_changeset(user, comment)