feat: Custom viewshed polygon reconstructor#67
Conversation
|
Current state of bencmarks: Correct: This PR: |
tombh
force-pushed
the
tombh/adjacent-polar-viewshed-builder
branch
from
60f7aac to
d343bc7
Compare
At higher angle counts a traditional polygon unioning of all the polar segments suffers from O(N²). For example, even in Rust a 3600 angled viewshed takes ~20s to create its GeoJSON. And many minutes in browser JS. Therefore we have to design an entirely new algorithm that takes advantage of the fact that we are only ever adding new pieces to the growing viewshed in a very specific way. Namely we only ever add polar segments, that are always contained within the arc of a single angle, onto the _side_ of a polygon. This returns polygon reconstruction to a ~200ms for 3600 angled viewsheds.
tombh
force-pushed
the
tombh/adjacent-polar-viewshed-builder
branch
from
d343bc7 to
ac0d147
Compare
|
Benchmark viewshed is now pixel-perfect with the existing one. Though note that the file size is slightly smaller due to a tidier algorithm. |
ryan-berger
left a comment
ryan-berger
left a comment
There was a problem hiding this comment.
I'm going to leave this for some changes responses, and dig into the algorithm a lot heavier tomorrow
|
|
||
| /// Keeps track of active and completed polygons within a viewshed. | ||
| #[derive(Default)] | ||
| pub struct Joiner { |
There was a problem hiding this comment.
To my eyes this shouldn't be public, and this file should be a single function, the idea of completed and active are internal implementation details, no?
In other words you can just have a fn join_segments([Vec<Segment>]) -> Result<geo::MultiPolygon>
There was a problem hiding this comment.
I've made a separate PR addressing all the pub issues: #68
But I'm not sure how you mean the file should be a single function? Where should the other code go? In that PR I've separated the Joiner definition into its own file so that the implementation files can inherit it and this avoid the need for any visibility declarations at all, maybe that's what you mean?
| .get_mut(starting_polygon_index) | ||
| .context("Bad polygon index")? | ||
| .vertices | ||
| .clone() |
There was a problem hiding this comment.
Why clone here? Can the borrow checker not figure out the borrow in both arms?
There was a problem hiding this comment.
It can figure it out. But that's not the issue. Inside the iterator that is borrowing the vertices there's a call to a &mut self method self.join_final_polygon().
There was a problem hiding this comment.
Oh yeah, that does complicate things. Let me re-review this with that in mind and think about the self borrow...
| } | ||
| } | ||
|
|
||
| if let Some(touching_start) = maybe_touching_start |
There was a problem hiding this comment.
This can be turned into:
let range = match (maybe_touching_start, maybe_touching_end) {
(Some(start), Some(end)) => start..end
_ => { return Ok(false) }
}
match (maybe_joining_polygon) {
(Some(joining_polygon) => { /* join_non_starting */ }
None => { /* join segment */ }
}
Ok(true)| /// polygon as follows: | ||
| /// * `Opening::NewStart/NewEnd` become `Opening::Start/End`. | ||
| /// * `Opening::Start/End` become `Opening::Null`. | ||
| pub fn downgrade_openings(&mut self) -> Result<()> { |
There was a problem hiding this comment.
This seems like a good panic! function?
There was a problem hiding this comment.
I'm actually going to convert the whole thing to asserts/panics. For compiling to WASM there's no point having fancy error handling just increasing the bundle size.
|
|
||
| /// A vertex is a single point in a polygon. | ||
| #[derive(Debug, Clone)] | ||
| pub struct Vertex { |
There was a problem hiding this comment.
Probably can be pub(crate) no?
| let mut maybe_touching_start = None; | ||
| let mut maybe_touching_end = None; | ||
|
|
||
| let (base_polygon, maybe_joining_polygon) = |
There was a problem hiding this comment.
Can be turned into:
let base_polygon = self
.active
.get_mut(base_polygon_index)
.context("Bad polygon index")?;
let maybe_joining_polygon = maybe_joining_polygon_index
.and_then(|index| { /* get completed code */ })| let completed = self | ||
| .active | ||
| .extract_if(.., |polygon| !polygon.is_touched) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
I don't think you have to collect() before you extend()
Adds the following lints: unreachable_pub = "warn" redundant_pub_crate = "allow" This enforces the practice of explicitly defining visibility. Therefore, if a symbol isn't truly public to the outside world then it should be qualified with `pub(crate)`, `pub(super)`, etc. This is slightly more verbose but helps reasoning about API surfaces and also safeguards against unexpected symbols becoming public when parent visibility is changed.
2 participants
At higher angle counts a traditional polygon unioning of all the polar segments suffers from O(N²). For example, even in Rust a 3600 angled viewshed takes ~20s to create its GeoJSON. And many minutes in browser JS.
Therefore we have to design an entirely new algorithm that takes advantage of the fact that we are only ever adding new pieces to the growing viewshed in a very specific way. Namely we only ever add polar segments, that are always contained within the arc of a single angle, onto the side of a polygon. This returns polygon reconstruction to
200ms80ms for 3600 angled viewsheds. There's actually huuuuge amount of room for speeding up this approach, but seeing as its already so much faster than the old method, this is good enough for now.TODO:
Apologies, this is rather big PR to review. Most of the changes are hidden in collapsed file views in the Github Diff view.