From: mmd-osm <mmd.osm@gmail.com>
Date: Tue, 13 Feb 2018 22:07:30 +0000 (+0100)
Subject: Remove BBOX expansion performance optimization
X-Git-Tag: live~3125
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/cea403dce084a82c0bff017e7ca23526ff43d1c4?ds=sidebyside

Remove BBOX expansion performance optimization

Closes #1749
Fixes #1742
---

diff --git a/app/models/changeset.rb b/app/models/changeset.rb
index 2ec26ecee..502637c2a 100644
--- a/app/models/changeset.rb
+++ b/app/models/changeset.rb
@@ -55,9 +55,6 @@ class Changeset < ActiveRecord::Base
 
   before_save :update_closed_at
 
-  # over-expansion factor to use when updating the bounding box
-  EXPAND = 0.1
-
   # maximum number of elements allowed in a changeset
   MAX_ELEMENTS = 10000
 
@@ -127,12 +124,9 @@ class Changeset < ActiveRecord::Base
   end
 
   ##
-  # expand the bounding box to include the given bounding box. also,
-  # expand a little bit more in the direction of the expansion, so that
-  # further expansions may be unnecessary. this is an optimisation
-  # suggested on the wiki page by kleptog.
+  # expand the bounding box to include the given bounding box.
   def update_bbox!(bbox_update)
-    bbox.expand!(bbox_update, EXPAND)
+    bbox.expand!(bbox_update)
 
     # update active record. rails 2.1's dirty handling should take care of
     # whether this object needs saving or not.
@@ -141,8 +135,7 @@ class Changeset < ActiveRecord::Base
 
   ##
   # the number of elements is also passed in so that we can ensure that
-  # a single changeset doesn't contain too many elements. this, of course,
-  # destroys the optimisation described in the bbox method above.
+  # a single changeset doesn't contain too many elements.
   def add_changes!(elements)
     self.num_changes += elements
   end
diff --git a/test/controllers/changeset_controller_test.rb b/test/controllers/changeset_controller_test.rb
index cb584759a..71fb71ca2 100644
--- a/test/controllers/changeset_controller_test.rb
+++ b/test/controllers/changeset_controller_test.rb
@@ -581,7 +581,6 @@ CHANGESET
     changeset_id = @response.body.to_i
 
     # upload some widely-spaced nodes, spiralling positive and negative to cause
-    # largest bbox over-expansion possible.
     diff = <<CHANGESET.strip_heredoc
       <osmChange>
        <create>
@@ -619,7 +618,7 @@ CHANGESET
     assert cs.min_lon >= -180 * GeoRecord::SCALE, "Minimum longitude (#{cs.min_lon / GeoRecord::SCALE}) should be >= -180 to be valid."
     assert cs.max_lon <= 180 * GeoRecord::SCALE, "Maximum longitude (#{cs.max_lon / GeoRecord::SCALE}) should be <= 180 to be valid."
     assert cs.min_lat >= -90 * GeoRecord::SCALE, "Minimum latitude (#{cs.min_lat / GeoRecord::SCALE}) should be >= -90 to be valid."
-    assert cs.max_lat >= 90 * GeoRecord::SCALE, "Maximum latitude (#{cs.max_lat / GeoRecord::SCALE}) should be <= 90 to be valid."
+    assert cs.max_lat <= 90 * GeoRecord::SCALE, "Maximum latitude (#{cs.max_lat / GeoRecord::SCALE}) should be <= 90 to be valid."
   end
 
   ##
@@ -1568,11 +1567,10 @@ CHANGESET
     # get the bounding box back from the changeset
     get :read, :params => { :id => changeset_id }
     assert_response :success, "Couldn't read back changeset for the third time."
-    # note that the 3.1 here is because of the bbox overexpansion
     assert_select "osm>changeset[min_lon='1.0000000']", 1
-    assert_select "osm>changeset[max_lon='3.1000000']", 1
+    assert_select "osm>changeset[max_lon='3.0000000']", 1
     assert_select "osm>changeset[min_lat='1.0000000']", 1
-    assert_select "osm>changeset[max_lat='3.1000000']", 1
+    assert_select "osm>changeset[max_lat='3.0000000']", 1
   end
 
   ##