core

core copied to clipboard

Regarding: https://github.com/OsmSharp/core/blob/17223c21089b8afa45b919b3dc5c877da2e63d32/src/OsmSharp/Streams/OsmStreamExtensions.cs#L218-L226 Since longitudes increase when going "left" (A picture for reference), I think the comparison operators here are backward. Would you consider a pull request which:

• replaces { left, right, up, down } with a single Bounds parameter
• fixes the comparisons
• I could fix "TODO: take into account the 180/-180 thing."

I think this is right :)

return x.Latitude.Value <= left && x.Longitude <= top &&
                    x.Latitude.Value >= right && x.Longitude >= bottom;

And I think your backwards, you mean increases

Since our interactions have been limited, I'm having trouble judging if you're serious or not. I stair endlessly at that smiley face you added, hoping for it to reveal a secret, but its lips are sealed.

Your picture was a upside down and thus your statement was incorrect as I assume it was based on that picture, when "Since longitudes increase when going left" I think you mean "Since longitudes decrease when going left".

My smiley face was because my code contains a typo in its current form and should be the other way around for left and top but it's hard to tell since you say increase as you go left so you might find it correct as it is...

Thank you for clarifying, I found my mistake. The reference image had the horizontal access labeled with 'W' and increased leftward. I didn't notice that 'W' is the negative of 'E'. I have revised my assumptions and I now agree that Longitudes increase to the right and the code is correct in the repo.

And with my embarrassment in mind, would it be worth the effort to add an overload which accepts Bounds?

public static OsmStreamSource FilterBox(this IEnumerable<OsmGeo> source, Bounds bounds, bool completeWays = false) 
{
   ....
}

Maybe...

E.g. if Bounds used a Vector or Vector4 then I can see the justification in changing that so the class is faster for operations but given that you need to access all 4 values to do the calculation and they are only compares it might not be worth it here...

If nothing else I think I can buy an overload of FilterBox which just delegates to the existing method for ease of use? but I honestly don't know how much use the Bounds class gets vs Envelope https://github.com/OsmSharp/core/search?l=C%23&q=Bounds

public static OsmStreamSource FilterBox(this IEnumerable<OsmGeo> source, Bounds bounds, bool completeWays = false) 
{
 return  FilterBox(source, bounds.MinLat, bounds.MinLong, bounds.MaxLat, bounds.MaxLng, completeWays)
}

I don't like the bounds class, it defines MinLongitude but that doesn't mean anything because you could define a bbox where the left longitude > right longitude wrapping around 180. That part still needs implementing though.