This project is read-only.

Change to fix Union problem

Jan 19, 2011 at 11:58 PM

I got an error trying to do a fairly simple Union of 2 polygons.  I was able to fix it by adding the || double.IsNaN(c.Z) to Coordinate.Equals() shown below.  I was wondering if folks think this is a safe change:


        public override bool Equals(object obj)
            if (obj is Coordinate == false)
                ICoordinate ic = obj as ICoordinate;
                if (ic == null) return false;
                if (double.IsNaN(Z)) return (ic.X == X && ic.Y == Y);
                return (ic.X == X && ic.Y == Y && ic.Z == Z);
            Coordinate c = (Coordinate)obj;
            if (double.IsNaN(Z) || double.IsNaN(c.Z)) return (c.X == X && c.Y == Y);
            return (c.X == X && c.Y == Y && c.Z == Z);

The problem was that the coordinates being compared were a mixture of 2D and 3D:



+  p {(-84.6976494321476, 41.6997514050474, 0)} DotSpatial.Topology.Coordinate
+  p0 {(-84.6976494321476, 41.6997514050474)} DotSpatial.Topology.Coordinate


It was way down in the bowels of the Topology Union code... a stack trace too long to put in the forum.


Jan 20, 2011 at 12:11 AM

Yes, pleas commit this update.  In fact, however, it should also be applied to the test above it in the instance where the item is an ICoordinate, so that it would read:

if(double.IsNaN(Z_) || double.IsNaN(c.Z)) return (ic.X == X && ic.Y == Y);

Thanks a bunch kyle.  Your fixes are truly appreciated.



Jan 20, 2011 at 2:05 PM

Will do.  One flag that is going up for me though is why the mixture of 2D and 3D coordinates way down in the guts of Topology.  The polygons I passed into union were 2D.  I don't have time to investigate that right now since the simple fix gets me past the current issue, but it is sort of strange.


Jan 20, 2011 at 4:29 PM

The coordinate has been badly messed with by me over time.  I'm very undecided as to the best approach.  The coordinate class was tinkered with to support 3D when I was using the DirectX 3D map.  The coordinate class acquired some 3D functionality, but the vast scope and swath of topology is 2D.  In at least one implementation, the of coordinate was N dimensional.  These things are still open to future tinkering.  One option is to have a 2D coordinate definition in topology and then have a 3D coordinate that inherits from the 2D coordinate.  The same inheritance pattern could also work for temporal coordinates.  I have been on and off again about having the ICoordinate interface implemented by the Coordinate class.  In modern code this should not cause much of a performance hit since the code should be optimized when it is compiled, but the jury is still out on that one.  The potential big hit to performance is making X and Y properties.  Again, theoretically this is optimized out by the compiler.  If they are properties then there is no problem implementing ICoordinate.  It might be faster if X and Y are public fields instead of properties, with no ICoordinate implementation.  Also there are parts of code that do an either or comparison currently like either ICoordinate or Coordinate, even creating separate overloads for the two cases.  Those bits of code would have to be updated if Coordinate implements ICoordinate becuase they become ambiguous.  So making the change would be a breaking change and require a fair amount of sleuthing to find the places where the fix is required.

Anyway, if anyone else wants to weigh in on "the way things should be" pertaining to coordinate I'm open to suggestions.



Jan 20, 2011 at 7:45 PM

What I meant was that one of the coordinates had a z value and the other did not:

{(-84.6976494321476, 41.6997514050474, 0)}


{(-84.6976494321476, 41.6997514050474)}

But, regarding your other comments, Ted, I would suppose that having a 3D coordinate derived from a 2D coordinate seems like a good approach.  Then, the virtual Equals method would know what to do.  I'm almost always in favor of performance over ease of use if you can't have both.  But, I'm no big fan of breaking changes either ;-)


Jan 20, 2011 at 8:02 PM

Yes, I tend to get sidetracked =).  The fix you added to check for Z is of course necessary.  In defense of inheritance, we are testing the model with the new Extents class in Data.  It has subclasses that support M and Z, but the base Extents is 2D only.  Speed is not usually much of an issue that I've seen, but when you start having hundreds of thousands of shapes loaded, memory becomes a noticeable concern, so for performance, I think anything that reduces the memory required per coordinate is probably a good thing.  On the dark side, most folks out there struggle with concepts like inheritance, and even if your comfortable with it, you might not even realize that there is such a class out there as an ExtentsZ or whatever.  I could go either way with coordinate, but my instinct for now is to leave it as is unless there is a bug that seriously needs fixing in an effort not to break stuff. 

However, I think there is an outstanding issue to try to reduce the dependency on "Topology" for stuff in higher libraries.  I think there is a frustration that people experience having to add a reference to Topology when they are working with unrelated things just because Coordinate and Envelope are defined down there.  I already have a Vertex class in Data.  What I can't remember is if I have added classes that inherit from Vertex to add M and Z abilities, or if I just added them to the vertex or what.  Anyway, before we break coordinate, we might try to perfect things using Vertex and see how well we can make it work first.  Then, the next step is to shift some content at higher levels to using more Vertex analysis and less Coordinate analysis so we can reduce the referencing to Topology.  But anyway, these are just ideas off the top of my head.  I'm hoping there are other voices here that will contribute some good ideas as well that might open up some doors.

There are always cool helper tricks like using Extension methods.  For instance, a Coordinate doesn't know a thing about a DotSpatial.Data, and so can't possibly support a "ToVertex()" method on it.  But that's exactly what people want to do.  But we can add an extension method in DotSpatial.Data, so that the ToVertex method shows up on coordinates if you have a reference to DotSpatial.Data.  A Vertex of course knows about a coordinate and has no trouble going in reverse.  Anyway, these are just thoughts for the moment.  (I may actually have already implemented that one actually, I can't remember though.)