On Wed, 2009-09-16 at 10:01 +0100, A Morris wrote: > I ran josm with your patch. Here are some initial general observations > which do not include any analysis of the algorithm itself. > > It seems the QuadBuckets class is referenced only through the > Collection<Node> interface, and none of the features (i.e. fast > searching within a bbox) are actually used?
Yes, you are correct. I actually tested it inside the validator plugin's UnconnectedWays test. It was developed wholly outside of the JOSM codebase and crammed back in later, actually. *Not* using its search features inside of JOSM is actually a plus at this point. Since it can't notice changes in node coordinates, it actually breaks if a node moves. > Rather than adding dead code (presumably with a view to enabling it > later), an easier path into the trunk might be to first refactor josm > to use a "SpatialIndex" interface that initially is backed by a > List/Collection (i.e. low risk and no change to current performance), > and adding working indexes later (initially via plugins for testing). It's actually not dead code. It isn't seeing wide use, but I have at least one user. :) > Another observation: It looks like QuadBuckets only works with actual > Lat/Longs (i.e. no numbers above 360). It would be nice to handle > arbitrary coordinates - two use cases that come to mind are: > > 1. index screen coordinates (e.g. so that a highlight can follow the > nearest object to the cursor and indicate what would be selected if > the mouse was clicked). > 2. index projected coordinates I agree with you in general. If you compare my code to the ruby quad tiling code that I copied from initially, you'll notice that I took great care to make the code more generic. Also, in moving from 32 to 48 bits of precision, I made the code much more flexible along the way. > Finally, a spatial index implementation should be generic enough to be > used in multiple places in the code (e.g. an index of way segments, or > whatever), so implementing Collection<Node> is too specialised. Heh. I did this in the last 4 seconds before I posted the patch, just so it would do *something*. Personally, I subscribe to the release early, release often, open source philosophy. This was an early release. I'd much rather have it integrated into JOSM now so that I can spend less time on cramming the patch back into the source every few weeks and spend more time on making it more generic, for instance. At this point, it doesn't *hurt* anything in JOSM. I'm not going anywhere and I'll be around to fix it up. As soon as we have proper OsmPrimitive change notifiers, I'll hook this code into it and we can start using it more widely. -- Dave _______________________________________________ josm-dev mailing list josm-dev@openstreetmap.org http://lists.openstreetmap.org/listinfo/josm-dev
