Formal BBox struct #377
Formal BBox struct #377
Conversation
fabd9d0 to
4e65ed6
Compare
0faf517 to
aa7a51d
Compare
|
Blocked on #378 |
aa7a51d to
4776f60
Compare
4776f60 to
a6310f5
Compare
a6310f5 to
ad642c9
Compare
f705619 to
cbf8c53
Compare
e82b6aa to
bf66730
Compare
|
This may or may not work with the GUI. I don't know, because I don't build or test the GUI. Soooo yeah, do with that knowledge what you will. |
|
My bbox code was a mess hahha! It doesn't run in GUI mode yet, but I'll take care of that and merge it! |
|
Alright, it's 3:30am now - let me know if this looks good to you or if I messed something up in your code! :D |
|
Does it sound good to represent the position data in two spaces: one real world system (in maybe 'spherical.rs' using LLPoint(the GeoCoord here) and LLBBox (the BBox here)), and a transformed mc system (in cartesian.rs currently, using XZPoint and XZBBox). This will make coordinate transformation much flexible and clearer. I just created the XZBBox struct and based the world editor on XZPoint, XZBBox, so it does not have to generate at (0,0), and I reserved the possibility to extend for more complex bbox shapes. |
|
Sure. You can merge this PR first and I will keep my #409 updated. These two are the components in a bigger architecture, and I will try to make them work seamlessly. |
benjamin051000
left a comment
There was a problem hiding this comment.
I'm not able to request changes against my own PR, interesting. Well this is what I'm seeing.
My understanding is that we want this merged soon. I think I'll reset back to the first commit, rebase off main, and then reapply relevant changes to resolve the issues I have with the code changes.
src/bbox.rs
Outdated
| let parts: Vec<&str> = s.split(|c| c == ',' || c == ' ').collect(); | ||
|
|
||
| if parts.len() != 4 { | ||
| return Err(format!("Invalid BBox format: expected 4 values, got {}", parts.len())); | ||
| } | ||
|
|
||
| // Parse the four floating point values | ||
| let mut values = [0.0; 4]; | ||
| for (i, part) in parts.iter().enumerate() { | ||
| values[i] = part.parse::<f64>().map_err(|e| { | ||
| format!("Failed to parse coordinate value '{}': {}", part, e) | ||
| })?; | ||
| } | ||
|
|
||
| let [min_lat, min_lng, max_lat, max_lng] = values; |
There was a problem hiding this comment.
I'd like to go back to the old logic if possible.
Can we just do one of these:
.split([',', ' '])for both chars (but do we really need to support both space and comma-delim values? Maybe we should just pass the delimiter into the constructor here.- IIRC
.try_into()will supply a good-enough error that it was unable to convert into an array of 4 floats. - The new float-parsing logic adds several lines.
src/bbox.rs
Outdated
| let [min_lat, min_lng, max_lat, max_lng] = values; | ||
| Self::new(min_lat, min_lng, max_lat, max_lng) | ||
| let [min_lng, min_lat, max_lng, max_lat] = values; | ||
| Self::new(min_lng, min_lat, max_lng, max_lat) |
There was a problem hiding this comment.
Hmm, I thought I had this right. What caused the need to change this? I definitely could have been wrong before, but we should make sure we understand what the ground truth is and probably write it down somewhere so there isn't any more confusion 😅
| // Input is valid; trigger the event | ||
| const bboxText = `${lat1} ${lng1} ${lat2} ${lng2}`; | ||
| // Input is valid; trigger the event with consistent comma-separated format | ||
| const bboxText = `${lat1},${lng1},${lat2},${lng2}`; |
There was a problem hiding this comment.
If we do this, then we shouldn't need space-delimited strings in the Bbox constructor, right?
2061e43 to
6efdf8d
Compare
|
Made some updates. |
6efdf8d to
42b7399
Compare
8682172 to
95bfdc9
Compare
|
@louis-e ready for review, please see the comments I wrote |
95bfdc9 to
e47f594
Compare
`BBox` represents a *valid* bounding box. It's only possible to have a BBox object that's valid.
e47f594 to
c3ce882
Compare
|
Thanks a lot for your work, looks good to me!! :) |
We need a more standardized way to represent bounding boxes, as we kind of just throw the string around and parse it into the four corresponding values when we need to.
Fixes #375
To Do