diff --git a/change_bot.rb b/change_bot.rb index 28b7858..a2c79f2 100644 --- a/change_bot.rb +++ b/change_bot.rb @@ -51,6 +51,11 @@ def actions :unclean end + #puts + #puts "[#{obj.version}] status:#{status}" + #puts "[#{obj.version}] geom:#{obj.geom}" + #puts "[#{obj.version}] geom_patch:#{geom_patch}" + # if this is not a clean version, then the only part # of the patch we can apply is the deletions, by the # 'deletions are always OK' rule. @@ -89,6 +94,9 @@ def actions new_tags.delete(k) if new_tags[k] == v end + #puts "[#{obj.version}] new_geom:#{new_geom}" + #puts "[#{obj.version}] obj_geom:#{obj.geom}" + # if the result of applying the patches is any different # from the version we actually have, then the object is # in a state that we can't display, so redact it. @@ -104,6 +112,8 @@ def actions base_obj.geom = new_geom base_obj.tags = new_tags + #puts "[#{obj.version}] base_obj:#{base_obj}" + prev_obj = obj end diff --git a/geom.rb b/geom.rb index 12adcbd..5de7ee7 100644 --- a/geom.rb +++ b/geom.rb @@ -106,41 +106,83 @@ def self.create(a, b) end def empty? - @diff.all? {|source, elt| source == :c} + @diff.empty? end def only_deletes? - not @diff.any? {|source, elt| source == :b || source == :d} + @diff.all? {|op, idx, elt| op == :delete} end def apply(geom, options = {}) geom_idx = 0 - new_geom = Array.new - - @diff.each do |source, elt| - case source - when :a # exists only in previous - i.e: a delete - if geom[geom_idx] == elt - geom_idx += 1 + new_geom = geom.clone + # mapping instruction index (i.e: in old geom) to + # current index in new_geom. + mapping = Hash[(0...(new_geom.length)).map {|i| [i,i]}] + + only_delete = options[:only] == :deleted + + @diff.each do |op, idx, elt| + case op + when :delete + new_idx = mapping[idx] + # note: we ignore the role as far as comparing before + # applying the patch is concerned - it'll still delete + # the same element... + unless (new_idx.nil? || + new_geom[new_idx].nil? || + new_geom[new_idx].type != elt.type || + new_geom[new_idx].ref != elt.ref) + new_geom.delete_at(new_idx) + mapping.delete(idx) + mapping.each {|k,v| mapping[k] = v - 1 if v > new_idx} + #puts "delete: #{elt}@#{idx}[#{new_idx}] -> #{mapping}" end - when :b # exists only in new version - i.e: an add - new_geom << elt unless options[:only] == :deleted - - when :c # exists in both - i.e: unchanged - if geom[geom_idx] == elt - new_geom << elt - geom_idx += 1 + when :insert + unless only_delete + new_idx = mapping[idx] + new_idx = 0 if new_idx.nil? && mapping.empty? + new_idx = mapping.values.max + 1 if new_idx.nil? + + new_geom.insert(new_idx, elt) + mapping.each {|k,v| mapping[k] = v + 1 if v >= new_idx} + mapping[idx] = new_idx + 1 unless mapping.has_key? idx + #puts "insert: #{elt}@#{idx}[#{new_idx}] -> #{mapping}" end - when :d - cur = geom[geom_idx] - unless cur.nil? - from_role, to_role = elt.role - if cur.type == elt.type && cur.ref == elt.ref - new_role = (options[:only] == :deleted) ? cur.role : to_role - new_geom << OSM::Relation::Member[elt.type, elt.ref, new_role] - geom_idx += 1 + when :move + unless only_delete + old_idx_from, old_idx_to = idx + + new_idx_to = mapping[old_idx_to] + new_idx_from = mapping[old_idx_from] + unless new_idx_from.nil? || new_geom[new_idx_from] != elt + new_idx_to = 0 if new_idx_to.nil? && mapping.empty? + new_idx_to = mapping.values.max + 1 if new_idx_to.nil? + + new_geom.insert(new_idx_to, elt) + mapping.each {|k,v| mapping[k] = v + 1 if v >= new_idx_to} + mapping[old_idx_to] = new_idx_to + 1 unless mapping.has_key? old_idx_to + + # reset new_index_from after mapping change + new_idx_from = mapping[old_idx_from] + new_geom.delete_at(new_idx_from) + mapping.delete(old_idx_from) + mapping.each {|k,v| mapping[k] = v - 1 if v > new_idx_from} + end + end + + when :alter + unless only_delete + new_idx = mapping[idx] + # don't bother comparing the old value of the role, + # since that's what we're overwriting anyway. + unless (new_idx.nil? || + new_geom[new_idx].nil? || + new_geom[new_idx].type != elt[1].type || + new_geom[new_idx].ref != elt[1].ref) + new_geom[new_idx] = elt[1] end end end @@ -159,36 +201,70 @@ def to_s private def initialize(d) + a_idx = 0 @diff = Array.new - current = Array.new - - d.each do |source, elt| - # unchanged elements go straight into the diff - if source == :c - # along with any unmatched elements - @diff.concat(current) - current = Array.new - @diff << [source, elt] - - else - # if any of the current elements match this one - # except for the role, then consider it as a - # role move instead. - idx = current.index {|s,e| s != source && e.type == elt.type && e.ref == elt.ref} + + d.each do |src, elt| + case src + when :a # element only in A: a delete + @diff << [:delete, a_idx, elt] + a_idx += 1 + + when :b # element only in B: an insert + @diff << [:insert, a_idx, elt] - if idx.nil? - # otherwise, queue up the element - current << [source, elt] - - else - s, e = current.delete_at(idx) - roles = (source == :a) ? [elt.role, e.role] : [e.role, elt.role] - @diff << [:d, OSM::Relation::Member[elt.type, elt.ref, roles]] - end + when :c # element in both - ignore + a_idx += 1 end end - @diff.concat(current) + # try and find insert-delete pairs where the + # element is exactly the same. these are the + # moves. + moves = @diff. + group_by {|op,idx,elt| elt}. + select {|elt,vec| (vec.length > 1 && + vec.any? {|op,idx,el| op == :insert} && + vec.any? {|op,idx,el| op == :delete})} + + moves.each do |elt, vec| + # could be more than 2 - for simplicity + # just assume the first of each delete + # and insert. + from = vec.find {|op,idx,elt| op == :delete} + to = vec.find {|op,idx,elt| op == :insert} + + from_idx = @diff.find_index(from) + to_idx = @diff.find_index(to) + + #puts "from_idx:#{from_idx} to_idx:#{to_idx}" + @diff[from_idx] = [:move, [from[1], to[1]], elt] + @diff.delete_at(to_idx) + end + + # now try and detect alterations to the role + # which we treat separately from other changes. + alters = @diff. + select {|op,idx,elt| op != :move}. + group_by {|op,idx,elt| [(op == :insert) ? idx - 1 : idx, elt.type, elt.ref]}. + select {|x,vec| (vec.length > 1 && + vec.any? {|op,idx,el| op == :insert} && + vec.any? {|op,idx,el| op == :delete})} + + alters.each do |x, vec| + # could be more than 2 - for simplicity + # just assume the first of each delete + # and insert. + from = vec.find {|op,idx,elt| op == :delete} + to = vec.find {|op,idx,elt| op == :insert} + + from_idx = @diff.find_index(from) + to_idx = @diff.find_index(to) + + #puts "from_idx:#{from_idx} to_idx:#{to_idx}" + @diff[from_idx] = [:alter, from[1], [from[2], to[2]]] + @diff.delete_at(to_idx) + end end end end diff --git a/osm.rb b/osm.rb index f7ffdda..d5f98a1 100644 --- a/osm.rb +++ b/osm.rb @@ -159,6 +159,16 @@ def <=>(o) @role <=> o.role end + def hash + [@type, @ref, @role].hash + end + + def eql?(o) + @type.eql?(o.type) && + @ref.eql?(o.ref) && + @role.eql?(o.role) + end + def to_s "Member[#{@type.inspect},#{@ref},#{@role.inspect}]" end diff --git a/tags.rb b/tags.rb index c83e72a..e9cfaa3 100644 --- a/tags.rb +++ b/tags.rb @@ -23,10 +23,13 @@ def self.odbl_clean?(tags) # special case for this one misspelling, as it's fairly # common to find "obdl" and there's no chance that we're # confusing "obdl" with anything else. - if k.downcase == "odbl" or k.downcase == "obdl" or k.downcase == "oodbl" + if (k.downcase == "odbl" or + k.downcase == "obdl" or + k.downcase == "oodbl") val = tags[k].downcase # tag synonyms for "clean" in this context (val == "clean" || + val == "clear" || val == "true" || val == "yes" || val == "clear" || @@ -287,9 +290,13 @@ def self.significant_tag?(old_v, new_v) # now check for homophones (TODO: is this really appropriate?) return false if Text::Metaphone.metaphone(old) == Text::Metaphone.metaphone(new) - # finally, look for changes in abbreviation. + # look for changes in abbreviation. return false if Abbrev.equal_expansions(old, new) + # check if the strings are the same except for whitespace + # presence. this would be considered insignificant. + return false if old.gsub(/ /,"") == new.gsub(/ /,"") + # otherwise, just look at the strings... old != new end diff --git a/test.rb b/test.rb index 55f88c1..afb0736 100755 --- a/test.rb +++ b/test.rb @@ -14,6 +14,7 @@ require './test_tags' require './test_util' require './test_way' +require './test_geom' require './test_needs_clarity' require './test_auto_fail' diff --git a/test_geom.rb b/test_geom.rb new file mode 100644 index 0000000..dd98b5f --- /dev/null +++ b/test_geom.rb @@ -0,0 +1,116 @@ +#!/usr/bin/env ruby + +require './change_bot' +require './user' +require './changeset' +require './db' +require './actions' +require './geom' +require 'minitest/unit' + +class TestGeom < MiniTest::Unit::TestCase + + # checks that, under normal circumstances, the application of + # a bunch of patches derived from a sequence of versions form + # the same sequence when merged on top of each other. in other + # words, that the patch taking and application works. + # + def test_relation_diff_inserts + geoms = [[], + [ [OSM::Way,29336166]], + [ [OSM::Way,29336166], [OSM::Way,29377987]], + [[OSM::Way,9650915], [OSM::Way,29336166], [OSM::Way,29377987]], + [[OSM::Way,9650915], [OSM::Way,29336166], [OSM::Way,29377987], [OSM::Way,29335519]] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + def test_relation_diff_deletes + geoms = [[[OSM::Way,9650915], [OSM::Way,29336166], [OSM::Way,29377987], [OSM::Way,29335519]], + [[OSM::Way,9650915], [OSM::Way,29336166], [OSM::Way,29377987]], + [ [OSM::Way,29336166], [OSM::Way,29377987]], + [ [OSM::Way,29336166]], + [] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + def test_relation_diff_inserts_and_deletes + geoms = [[], + [[OSM::Way,9650915], [OSM::Way,29336166]], + [ [OSM::Way,29336166], [OSM::Way,29377987]], + [[OSM::Way,9650915], [OSM::Way,29336166], [OSM::Way,29377987]], + [[OSM::Way,9650915], [OSM::Way,29377987], [OSM::Way,29335519]] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + def test_relation_diff_moves + geoms = [[[OSM::Way,1], [OSM::Way,2], [OSM::Way,3], [OSM::Way,4]], + [[OSM::Way,2], [OSM::Way,1], [OSM::Way,3], [OSM::Way,4]], + [[OSM::Way,2], [OSM::Way,3], [OSM::Way,1], [OSM::Way,4]], + [[OSM::Way,2], [OSM::Way,3], [OSM::Way,4], [OSM::Way,1]], + [[OSM::Way,2], [OSM::Way,3], [OSM::Way,4], [OSM::Way,1]], + [[OSM::Way,1], [OSM::Way,2], [OSM::Way,3], [OSM::Way,4]] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + def test_relation_diff_moves_reverse + geoms = [[[OSM::Way,1], [OSM::Way,2], [OSM::Way,3], [OSM::Way,4]], + [[OSM::Way,4], [OSM::Way,1], [OSM::Way,2], [OSM::Way,3]], + [[OSM::Way,3], [OSM::Way,4], [OSM::Way,1], [OSM::Way,2]], + [[OSM::Way,2], [OSM::Way,3], [OSM::Way,4], [OSM::Way,1]], + [[OSM::Way,1], [OSM::Way,2], [OSM::Way,3], [OSM::Way,4]] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + def test_relation_diff_alter + geoms = [[[OSM::Way,1], [OSM::Way,2,"foo123"], [OSM::Way,3]], + [[OSM::Way,1], [OSM::Way,2,"bar456"], [OSM::Way,3]], + [[OSM::Way,1], [OSM::Way,2,"bat789"], [OSM::Way,3]] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + def test_relation_diff_alter_front + geoms = [[[OSM::Way,1,"foo123"], [OSM::Way,2], [OSM::Way,3]], + [[OSM::Way,1,"bar456"], [OSM::Way,2], [OSM::Way,3]], + [[OSM::Way,1,"bat789"], [OSM::Way,2], [OSM::Way,3]] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + def test_relation_diff_alter_back + geoms = [[[OSM::Way,1], [OSM::Way,2], [OSM::Way,3,"foo123"]], + [[OSM::Way,1], [OSM::Way,2], [OSM::Way,3,"bar456"]], + [[OSM::Way,1], [OSM::Way,2], [OSM::Way,3,"bat789"]] + ].map {|g| OSM::Relation[g]} + check_diff_apply(geoms) + end + + private + + def check_diff_apply(geoms) + x = OSM::Relation[[]] + x.geom = geoms.first.geom + + geoms.each_cons(2).each do |a, b| + d = Geom.diff(a, b) + x.geom = d.apply(x.geom) + #puts + #puts "a: #{a.members}" + #puts "b: #{b.members}" + #puts "d: #{d}" + #puts "x: #{x.members}" + assert_equal(b, x) + end + end +end + + +if __FILE__ == $0 + MiniTest::Unit.new.run(ARGV) +end +