Fix fill_queue to avoid invalid Rect initialization

[nick-parker]

I mostly fixed this for myself, but I'm not making a PR yet because geojson needs to version-bump geo_types before your test fixtures will work with this change.

https://github.com/nick-parker/rust-geo-booleanop

The current version of geo_types::rect::Rect enforces validity, so the sbbox and cbbox initializations with inf and neg_inf panic. It's easy enough to fix by just passing separate min, max mut coords through fill_queue and then constructing the bboxes afterward.

[bluenote10]

Yes good point, we can probably just introduce a module local BoundingBox struct for passing around the values without any logic involved.

[nick-parker]

I went ahead and did as you suggested, it's fairly clean:

https://github.com/nick-parker/rust-geo-booleanop/tree/internal-rect

Unfortunately the tests module doesn't compile because geojson hasn't bumped to geo-types 0.5.x yet, but this is all ready to go whenever they get around to it.

I'm hoping it doesn't happen til the next numbered release of geo-types, because I need this commit included for my project:

georust/geo@302d17d

[bluenote10]

I had a quick look into how Rect has changed, and I actually think we might be better off just using our own primitive Rect{min: Coordinate<T>, max: Coordinate<T>) throughout the algorithm. The changes to Geo's Rect add complexity that we don't need internally. In particular:

• min / max are now non-inline getters -- having real function calls will probably hurt performance.
• the valid bounding box check is redundant to the check we have to do on our side anyway, and I think it is unlikely the compiler can optimize it away. I tried to keep the number of float comparisons as low as possible in that part (they are more costly then I expected), so it makes me cringe to see two redundant ones ;).

Note that in your branch you are setting both min and max to positive infinite, so don't be surprised if you get weird results ;).

And doesn't your approach panic with an empty polygon? When you call set_min/set_max the temporary bbox will still contain infinity values and fail the validity check?

[nick-parker]

Right on all counts!

I didn't know about the limitations of automatic inlining across crates, I definitely assumed those getters would compile to the same code as before. The cost of the check hadn't occurred to me either, I'll see about converting everything to an internal bbox structure.