search

georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.53k stars 198 forks source link

Add comments to BoolOps implementation. #1102

Closed andriyDev closed 8 months ago

andriyDev commented 11 months ago

I've taken a stab at documenting these things and adding a few more inline comments to hopefully make it more clear to future readers.

There are a couple parts I still don't really understand, which I've marked as TODOs. Hopefully with a review, someone else might have more insight and we can document this even better. I will also likely expand the scope of comments as I make more progress (so far this is just assembly.rs, but I may try to capture more of the BoolOps).

rmanoka commented 11 months ago

Thanks @andriyDev ; the poor docs in this impl. is primarily my fault. We built this impl. incrementally, trying to fix up the floating point precision issues. I somewhat gave-up midway, as I couldn't figure how to provably fix the Martinez-Rueda family of algorithms to work with f64/f32.

That said, the Assembly module, iirc, does not have floating point precision issues as it doesn't actually need to break any segments. In fact, I think, we can even get this module to work without using GeoFloat at all (only GeoNum).

I'll do my best to review this in the next week or two.

andriyDev commented 11 months ago

Hey no blame! The only way to make the code better is to work together :)

As far floating point issues, I had been working on my own Martinez-Rueda implementation and had also been struggling heavily with floating point issues myself. I've got an idea for why those are happening so I'll create a separate issue.

andriyDev commented 10 months ago

I rebased onto main, and I added more comments for the bool_ops folder.

There are many changes we could make to simplify the code, but I think just documenting it is the first step.

In addition, this isn't the whole story - the main thing that's missing now is documenting CrossingsIter which mostly means going through Sweep with a fine-toothed comb.

frewsxcv commented 9 months ago

@andriyDev This is still marked as draft. Is that intentional or is this ready for review?

andriyDev commented 9 months ago

I'm happy to call this ready for review. More comments can always be another PR :)

urschrei commented 8 months ago

I'd really like to see this merged in order to facilitate further investigation into RegionAssembly's tendency to panic (see https://github.com/georust/geo/issues/1104 for a possible avenue to investigate). I think there's only one minor question left to be resolved!

andriyDev commented 8 months ago

I'd really like to see this merged in order to facilitate further investigation into RegionAssembly's tendency to panic (see #1104 for a possible avenue to investigate). I think there's only one minor question left to be resolved!

I would be shocked if this code is "responsible" for the panic. The RegionAssembly is fairly straightforward. What's more likely in my mind is that bad data is coming in from Proc or Spec in boolean ops since those are much more complex IMO. But that's just my speculation haha.

Regardless we (mostly I) should push this through to make progress on that.

urschrei commented 8 months ago

Regardless we (mostly I) should push this through to make progress on that

Either that or outright replacement with your Martinez-Rueda implementation, but that's all a separate discussion.

andriyDev commented 8 months ago

Either that or outright replacement with your Martinez-Rueda implementation, but that's all a separate discussion.

To be clear, mine is also riddled with panics LMAO. Maybe one day I'll find the "smoking gun".