Add comments to BoolOps implementation. #1102

andriyDev · 2023-10-29T05:53:23Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This part of the code relies on a lot of assumptions that are not obviously until you really dig into the code, and have a lot of components that are not really clear (what's the difference between a Ring and a Snake?).

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 · 2023-10-29T16:27:31Z

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 · 2023-10-29T16:36:46Z

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.

Wow these are tricky to explain the difference.

andriyDev · 2023-12-08T01:15:46Z

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.

andriyDev · 2023-12-08T01:18:38Z

geo/src/algorithm/bool_ops/op.rs

    idx: usize,
+    /// The region of this edge when the piece is going in the same direction as


I'm pretty sure this is wrong. I can sorta understand the intent, but it doesn't make sense to me exactly what the check below means.

@rmanoka Can you shed any light on this?

frewsxcv · 2023-12-19T03:55:21Z

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

andriyDev · 2023-12-19T04:10:57Z

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

frewsxcv · 2023-12-25T17:19:01Z

geo/src/algorithm/bool_ops/assembly.rs

@@ -85,14 +98,15 @@ impl<T: GeoFloat> RegionAssembly<T> {
                        parent_snake_idx = Some(
                            iter.prev_active(d)
                                .map(|(_, seg)| seg.snake_idx.get())
+                                // TODO


Can you remove these TODOs?

I can, but these TODOs really need an explanation. They make no sense to me. I tried to remove these defaults as well and tests started to fail, so clearly they are necessary but I don't have a logical reason how.

@rmanoka Do you recall why this .unwrap_or(0) is here?

I don't think the number here matters. If the expression is not Some(), then the "snake" will complete to an exterior ring, which doesn't need a parent idx. I tried verifying by setting random numbers inside .unwrap_or(), and tests seem to pass.

@andriyDev Are you OK with this explanation?

Yes, sorry, I'd lost track of this. I will update the comment tonight.

As an aside, I'd much prefer this case to be handled another way (if the id is unused we shouldn't need to unwrap it). But that's not the point of this PR, that kind of change should be another PR.

I've added the explanation to the comments. I believe this is ready to go then (unless there are other concerns). :)

urschrei · 2024-01-10T17:26:45Z

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!

andriyDev · 2024-01-10T17:58:32Z

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 · 2024-01-10T18:00:58Z

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 · 2024-01-10T18:58:50Z

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".

frewsxcv

@andriyDev thank you so much for adding these comments!

andriyDev force-pushed the comments branch from 5a23dd0 to bcd1fe1 Compare October 29, 2023 05:55

andriyDev force-pushed the comments branch from bcd1fe1 to f4075ba Compare December 8, 2023 01:10

andriyDev added 5 commits December 7, 2023 17:11

Add comments for functions and structs in assembly.rs.

f69a94b

Add inline comments for functions and structs in assembly.rs.

67598f2

Add comments and inline comments for functions and structs in spec.rs.

30fefd0

Add function and inline comments for op.rs.

aaff547

Try to explain the difference between Proc and Spec.

a30b500

Wow these are tricky to explain the difference.

andriyDev force-pushed the comments branch from f4075ba to a30b500 Compare December 8, 2023 01:11

andriyDev commented Dec 8, 2023

View reviewed changes

andriyDev marked this pull request as ready for review December 19, 2023 04:10

urschrei mentioned this pull request Dec 21, 2023

Panic in RegionAssembly reproducibly #1127

Closed

frewsxcv reviewed Dec 25, 2023

View reviewed changes

andriyDev and others added 2 commits January 10, 2024 17:36

Remove remaining TODOs in assembly.rs comments.

f599a5c

Merge branch 'main' into comments

a201bb0

frewsxcv approved these changes Jan 23, 2024

View reviewed changes

frewsxcv enabled auto-merge January 23, 2024 00:27

frewsxcv added this pull request to the merge queue Jan 23, 2024

Merged via the queue into georust:main with commit 4456a9d Jan 23, 2024
15 checks passed

andriyDev deleted the comments branch February 3, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add comments to BoolOps implementation. #1102

Add comments to BoolOps implementation. #1102

andriyDev commented Oct 29, 2023

rmanoka commented Oct 29, 2023

andriyDev commented Oct 29, 2023

andriyDev commented Dec 8, 2023

andriyDev Dec 8, 2023

urschrei Jan 10, 2024

frewsxcv commented Dec 19, 2023

andriyDev commented Dec 19, 2023

frewsxcv Dec 25, 2023

andriyDev Dec 29, 2023

frewsxcv Dec 31, 2023

rmanoka Jan 2, 2024

urschrei Jan 10, 2024

andriyDev Jan 10, 2024

andriyDev Jan 11, 2024

urschrei commented Jan 10, 2024 •

edited

Loading

andriyDev commented Jan 10, 2024 •

edited

Loading

urschrei commented Jan 10, 2024

andriyDev commented Jan 10, 2024

frewsxcv left a comment

		idx: usize,
		/// The region of this edge when the piece is going in the same direction as

Add comments to BoolOps implementation. #1102

Add comments to BoolOps implementation. #1102

Conversation

andriyDev commented Oct 29, 2023

rmanoka commented Oct 29, 2023

andriyDev commented Oct 29, 2023

andriyDev commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv commented Dec 19, 2023

andriyDev commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urschrei commented Jan 10, 2024 • edited Loading

andriyDev commented Jan 10, 2024 • edited Loading

urschrei commented Jan 10, 2024

andriyDev commented Jan 10, 2024

frewsxcv left a comment

Choose a reason for hiding this comment

urschrei commented Jan 10, 2024 •

edited

Loading

andriyDev commented Jan 10, 2024 •

edited

Loading