[josm-dev] Jumbo Patch
Dave Hansen
dave at sr71.net
Sat Dec 15 19:09:02 GMT 2007
On Sat, 2007-12-15 at 10:13 +0100, Gabriel Ebner wrote:
> (Commenting on the core changes.)
Thanks for the comments!! They're much appreciated.
> On Fri, Dec 14, 2007 at 04:45:56PM -0800, Dave Hansen wrote:
> > OK, this is in the open source spirit of "release early release often".
> > I've been hacking on JOSM for the purpose of making the TIGER merging
> > easier. Frederik, this implements some of the "reverse lookup" stuff
> > that we were talking about a few weeks ago. It seems to work OK,
> > although it's not heavily tested.
>
> We were somehow planning to remove all optimization stuff from OsmPrimitive in
> the mid^Wlong term, so I don't know if this is a step in the right direction,
> especially as it adds introduces quite a lot of changes in unrelated code.
Yeah, it definitely needs a lot of "unrelated" changes. But, this is
more a result from having Way.nodes so accessible in the past (and used
in many different ways) than my code itself truly causing too much
splatter.
> Additionally it seems to me that, once created, a way is never removed from
> the cache. When you split a way[1], JOSM keeps both the old one and new ones in
> memory so it can undo the change. When you upload it then, that cache is
> simply cleared; we just call commands.clear(). Currently, the old way is then
> no longer referenced and can be freed by the GC. However we'd still keep a
> reference in Node.wayList then, effectively creating a leak. (Same goes for
> temporary ways; e.g. in CombineWayAction we create a way before we ask the
> user to merge the tags. If the user says no, we just return and don't clean
> up the way.)
>
> [1] I just saw that you modified SplitWayAction not to do this, still, the
> general point holds true.
I do think there's a basic problem with the ReplaceCommand. It is tied
to an individual Java object rather than all of the particular objects
that may have been instantiated to represent the OSM object. Just as
you say, JOSM keeps old and new copies in memory when a way is removed
(or modified).
Take this way:
http://sr71.net/~dave/osm/josm/selfintersecting.osm
and try to have the JOSM validator "fix" it. You'll see the result of
this.
> Couldn't we just keep a hash table in DataSet (from nodes to lists of ways)
> and update that in *Command?
> (Incidentally, Node.waysUsing() is only used once, and that's the only way to
> access this cache.)
In the core, but take a look at the validator plugin. I use it
everywhere!
> > @@ -216,6 +213,8 @@
> > JOptionPane.showMessageDialog(Main.parent, tr("The way cannot be split at the selected nodes. (Hint: Select nodes in the middle of the way.)"));
> > return;
> > }
> > + System.out.println("wayChunks.size(): " + wayChunks.size());
> > + System.out.println("way id: " + selectedWay.id);
> >
> > // build a list of commands, and also a new selection list
> > Collection<Command> commandList = new ArrayList<Command>(wayChunks.size());
>
> Idea: How about adding Main.debug() and a --debug flag, we wouldn't need to
> remove all the debugging code and add new ad-hoc output every time we want
> verbose output.
Cool. I could do that.
> > @@ -223,12 +222,16 @@
> >
> > Iterator<List<Node>> chunkIt = wayChunks.iterator();
> >
> > - // First, change the original way
> > - Way changedWay = new Way(selectedWay);
> > - changedWay.nodes.clear();
> > - changedWay.nodes.addAll(chunkIt.next());
> > - commandList.add(new ChangeCommand(selectedWay, changedWay));
> > newSelection.add(selectedWay);
> > + // First, pull the first chunk off, it stays in
> > + // the "original" way.
> > + List<Node> firstChunk = chunkIt.next();
> > + for (Node n : selectedWay.allNodes()) {
> > + if (firstChunk.contains(n))
> > + continue;
> > + System.out.println("removing node: " + n + " from way: " + selectedWay.id);
> > + commandList.add(new ReplaceSubObjectCommand(selectedWay, n, null));
> > + }
> >
> > // Second, create new ways
> > while (chunkIt.hasNext()) {
>
> This might turn out to create tons of ReplaceSubObjectCommands. Say the user
> wants to split a two-node way off a way with 5k nodes, then there's a 50
> percent probability that we add 4999 ReplaceSubObjectCommands.
Yeah, that is true. There's certainly room for optimization here.
My biggest goal with ReplaceSubObject was to demonstrate that having
more granular or more _specific_ commands than simply "copy this object
and replace it with this one" was feasible.
We could pretty easily have a SplitWayCommand that took a way, and a
node number to split on, then kept track of the resultant ways so that
it could re-merge them at undo.
> BTW, there doesn't seem to be a ReplaceSubObjectCommand.java in the patch.
> Depending on the implementation it might be a bad idea to actually implement
> spltting ways that way, as we might want to split a way that contains a node
> multiple times, and I see no way to tell ReplaceSubObjectCommand what node to
> replace.
I've attached my current version, and added it to my local SVN so I
won't forget to diff it in the future.
> > Index: core/src/org/openstreetmap/josm/actions/search/SearchCompiler.java
> > ===================================================================
> > --- core/src/org/openstreetmap/josm/actions/search/SearchCompiler.java (revision 485)
> > +++ core/src/org/openstreetmap/josm/actions/search/SearchCompiler.java (working copy)
> > @@ -1,4 +1,3 @@
> > -// License: GPL. Copyright 2007 by Immanuel Scholz and others
> > package org.openstreetmap.josm.actions.search;
> >
> > import java.io.IOException;
>
> Yes, I know these copyright notices aren't the best (heck, it doesn't even say
> what version of the GPL applies, or whether we have an "or any later version"
> clause), but that belongs IMHO to another patch.
Oopsee. Not indended :(
> > Index: core/src/org/openstreetmap/josm/actions/MergeNodesAction.java
> > ===================================================================
> > --- core/src/org/openstreetmap/josm/actions/MergeNodesAction.java (revision 485)
> > +++ core/src/org/openstreetmap/josm/actions/MergeNodesAction.java (working copy)
> > @@ -169,7 +169,37 @@
> > Map<String, JComboBox> components = new HashMap<String, JComboBox>();
> > JPanel p = new JPanel(new GridBagLayout());
> > for (Entry<String, Set<String>> e : props.entrySet()) {
> > - if (e.getValue().size() > 1) {
> > + if (e.getKey().equals("tiger:tlid")) {
> > + TreeMap tlids = new TreeMap();
> > + for (String tlid_list: e.getValue()) {
> > + for (String tlid_str: tlid_list.split(":")) {
> > + Integer tlid = new Integer(tlid_str);
> > + tlids.put(tlid, 1);
> > + }
> > + }
> > + String combined = "";
> > + for (Object _tlid : tlids.keySet()) {
> > + Integer tlid = (Integer)_tlid;
> > + if (combined.length() > 0)
> > + combined += ":";
> > + combined += tlid;
> > + }
> > + newNode.put(e.getKey(), combined);
> > + } else if (e.getKey().indexOf("tiger:") != -1) {
> > + TreeMap attrs = new TreeMap();
> > + for (String attr_list: e.getValue()) {
> > + for (String attr_str: attr_list.split(":")) {
> > + attrs.put(attr_str, 1);
> > + }
> > + }
> > + String combined = "";
> > + for (Object attr : attrs.keySet()) {
> > + if (combined.length() > 0)
> > + combined += ":";
> > + combined += attr;
> > + }
> > + newNode.put(e.getKey(), combined);
> > + } else if (e.getValue().size() > 1) {
> > JComboBox c = new JComboBox(e.getValue().toArray());
> > c.setEditable(true);
> > p.add(new JLabel(e.getKey()), GBC.std());
>
> I'd like to keep the GUI code free of such merging logic if it's easily
> possible. It isn't easy to read as it is now, we don't have to make worse.
How about something like this:
http://sr71.net/~dave/osm/josm/tigerutils.patch
It's much more compact.
> > @@ -56,6 +58,11 @@
> > * Send the dataset to the server. Ask the user first and does nothing if he
> > * does not want to send the data.
> > */
> > + static int MSECS_PER_SECOND = 1000;
> > + static int SECONDS_PER_MINUTE = 60;
> > + static int MINUTES_PER_HOUR = 60;
> > + static int HOURS_PER_DAY = 24;
> > + static int MSECS_PER_MINUTE = MSECS_PER_SECOND * SECONDS_PER_MINUTE;
> > public void uploadOsm(Collection<OsmPrimitive> list) throws SAXException {
> > processed = new LinkedList<OsmPrimitive>();
> > initAuthentication();
>
> You should at least make these constants final, private would be even better.
OK. I don't think Java _had_ final the last time I used it :) I know
it didn't have the iterator for-each loops!
> > @@ -65,13 +72,31 @@
> >
> > NameVisitor v = new NameVisitor();
> > try {
> > + long start = System.currentTimeMillis();
> > + Time startTime = new Time(start);
> > for (OsmPrimitive osm : list) {
> > if (cancel)
> > return;
> > + long now = System.currentTimeMillis();
> > + long elapsed = now - start;
> > + if (elapsed == 0)
> > + elapsed = 1;
> > + int progress = Main.pleaseWaitDlg.progress.getValue();
> > + float uploads_per_ms = (float)progress / elapsed;
> > + float uploads_left = list.size() - progress;
> > + int ms_left = (int)(uploads_left / uploads_per_ms);
> > + int minutes_left = ms_left / MSECS_PER_MINUTE;
> > + int seconds_left = (ms_left / MSECS_PER_SECOND) % SECONDS_PER_MINUTE ;
> > + String time_left_str = Integer.toString(minutes_left) + ":";
> > + if (seconds_left < 10)
> > + time_left_str += "0";
> > + time_left_str += Integer.toString(seconds_left);
> > osm.visit(v);
> > - Main.pleaseWaitDlg.currentAction.setText(tr("Upload {0} {1} ({2})...", tr(v.className), v.name, osm.id));
> > + Main.pleaseWaitDlg.currentAction.setText(tr("Upload {0} {1} (id: {2}) {3}% {4}/{5} ({6} left)...",
> > + tr(v.className), v.name, osm.id, 100.0*progress/list.size(), progress, list.size(), time_left_str));
> > osm.visit(this);
> > Main.pleaseWaitDlg.progress.setValue(Main.pleaseWaitDlg.progress.getValue()+1);
> > + Main.pleaseWaitDlg.progress.setValue(progress+1);
> > }
> > } catch (RuntimeException e) {
> > e.printStackTrace();
>
> IMHO we have too much run-together code as-is, maybe it would be better to put
> that into a formatDuration function or something.
Yeah. That was a pretty bad hack. I'll clean it up.
> > + public HashSet<Pair<Node,Node>> getNodePairs(boolean sort) {
> > + HashSet<Pair<Node,Node>> chunkSet = new HashSet<Pair<Node,Node>>();
> > + Node lastN = null;
> > + for (Node n : this.nodes) {
> > + if (lastN == null) {
> > + lastN = n;
> > + continue;
> > + }
> > + Pair<Node,Node> np = new Pair<Node,Node>(lastN, n);
> > + if (sort) {
> > + Pair.sort(np);
> > + }
> > + chunkSet.add(np);
> > + lastN = n;
> > + }
> > + return chunkSet;
> > + }
>
> I have used this lastN hack quite a few times to iterate over way segments,
> maybe it would be useful to keep this, perhaps change it to use a list though,
> otherwise the segments get all shuffled.
OK. I've changed it over to a list.
> > Index: core/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java
> > ===================================================================
> > --- core/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java (revision 485)
> > +++ core/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java (working copy)
> > @@ -114,6 +114,12 @@
> > public boolean incomplete = false;
> >
> > /**
> > + * This is to help determine when individual objects are modified.
> > + * When making changes, a user may increment this count.
> > + */
> > + public int version = 0;
>
> I have already said before that this is a hack, and that if you want to
> prevent objects from being overwritten, you should fix the code that does
> that.
The real problem here is a disconnection between the times when a
Command is created and when it is executed. How do we ever tell that a
command is still _valid_? In the Way.replaceSubObject() code, I worked
around this by ensuring that an attempt to delete an already deleted
node is considered success. Should we have a way for commands to
self-validate before performing their actions?
> > + public Object replace(OsmPrimitive x, OsmPrimitive y) {
> > + return this.replace(x, y, null);
> > + }
> > + public Object replace(OsmPrimitive x, OsmPrimitive y, Object cookie) {
> > + System.out.println("Called OsmPrimitive::replace() on: " + this.getClass().getName());
> > + return null;
> > + }
>
> OsmPrimitive.replace isn't used anywhere in the patch.
It's used in the mysterious ReplaceSubObject, which I've attached.
> > @@ -254,6 +270,9 @@
> > (keys == null ? osm.keys==null : keys.equals(osm.keys));
> > }
> >
> > + public boolean mayDelete() {
> > + return false;
> > + }
>
> OsmPrimitive.mayDelete isn't used anywhere else either.
That one is used in the equally mysterious ConditionalDelete, also
attached. :)
Again, thanks for all the insight. Please keep it coming!
-- Dave
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ReplaceSubObjectCommand.java
Type: text/x-java
Size: 1763 bytes
Desc: not available
URL: <http://lists.openstreetmap.org/pipermail/josm-dev/attachments/20071215/eeb3a9b6/attachment.java>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ConditionalDeleteCommand.java
Type: text/x-java
Size: 2245 bytes
Desc: not available
URL: <http://lists.openstreetmap.org/pipermail/josm-dev/attachments/20071215/eeb3a9b6/attachment-0001.java>
More information about the josm-dev
mailing list