Formal BBox struct by benjamin051000 · Pull Request #377 · louis-e/arnis

benjamin051000 · 2025-02-20T03:26:46Z

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

Get GUI working with this
Fix compile errors
Fix render bug

benjamin051000 · 2025-02-22T03:45:36Z

Blocked on #378

src/bbox.rs

src/main.rs

src/retrieve_data.rs

benjamin051000 · 2025-04-10T21:18:07Z

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.

louis-e · 2025-04-12T23:17:19Z

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!

louis-e · 2025-04-13T01:30:14Z

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

XianlinSheng · 2025-04-13T18:09:54Z

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.

louis-e · 2025-04-13T19:10:32Z

That sounds interesting, thanks for your idea. Can we merge this PR and follow up with your suggestion in #409 or a seperate PR? It would be cool to cover #29 with this and enable multiple generations in a single world, since we can also programmatically set the player spawn point.

XianlinSheng · 2025-04-13T19:23:54Z

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

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.

benjamin051000 · 2025-04-20T18:19:51Z

src/bbox.rs

+        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;


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.~~ I see that 1d31dfe fixed this

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

benjamin051000 · 2025-04-20T18:23:20Z

src/bbox.rs

-        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)


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 😅

benjamin051000 · 2025-04-20T18:34:36Z

gui-src/js/main.js

-              // 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}`;


If we do this, then we shouldn't need space-delimited strings in the Bbox constructor, right?

benjamin051000 · 2025-04-21T02:31:33Z

Made some updates. ~~Moving back to draft until it's working again~~ it's ready for review again.

benjamin051000 · 2025-04-22T02:00:12Z

@louis-e ready for review, please see the comments I wrote

`BBox` represents a *valid* bounding box. It's only possible to have a BBox object that's valid.

louis-e · 2025-04-26T13:00:06Z

Thanks a lot for your work, looks good to me!! :)

benjamin051000 changed the title ~~WIP: BBox struct~~ RFC: Formal BBox struct Feb 20, 2025

benjamin051000 force-pushed the formalize-bbox branch 3 times, most recently from fabd9d0 to 4e65ed6 Compare February 22, 2025 03:26

benjamin051000 marked this pull request as ready for review February 22, 2025 03:27

benjamin051000 force-pushed the formalize-bbox branch 2 times, most recently from 0faf517 to aa7a51d Compare February 22, 2025 03:31

benjamin051000 changed the title ~~RFC: Formal BBox struct~~ Formal BBox struct Feb 22, 2025

benjamin051000 force-pushed the formalize-bbox branch from aa7a51d to 4776f60 Compare February 22, 2025 04:37

benjamin051000 commented Feb 22, 2025

View reviewed changes

src/bbox.rs Show resolved Hide resolved

benjamin051000 commented Feb 22, 2025

View reviewed changes

src/bbox.rs Show resolved Hide resolved

benjamin051000 force-pushed the formalize-bbox branch from 4776f60 to a6310f5 Compare February 22, 2025 04:56

benjamin051000 commented Feb 22, 2025

View reviewed changes

src/main.rs Show resolved Hide resolved

benjamin051000 force-pushed the formalize-bbox branch from a6310f5 to ad642c9 Compare February 22, 2025 05:02

benjamin051000 commented Feb 22, 2025

View reviewed changes

src/retrieve_data.rs Outdated Show resolved Hide resolved

benjamin051000 force-pushed the formalize-bbox branch 3 times, most recently from f705619 to cbf8c53 Compare February 24, 2025 02:25

benjamin051000 force-pushed the formalize-bbox branch 2 times, most recently from e82b6aa to bf66730 Compare April 10, 2025 21:09

XianlinSheng mentioned this pull request Apr 20, 2025

Open up a map editing architecture in the workflow for transformation, and enables generating at non-origin #409

Merged

benjamin051000 commented Apr 20, 2025

View reviewed changes

benjamin051000 force-pushed the formalize-bbox branch from 2061e43 to 6efdf8d Compare April 21, 2025 02:20

benjamin051000 marked this pull request as draft April 21, 2025 02:31

benjamin051000 force-pushed the formalize-bbox branch from 6efdf8d to 42b7399 Compare April 21, 2025 03:51

benjamin051000 marked this pull request as ready for review April 21, 2025 03:51

benjamin051000 force-pushed the formalize-bbox branch 2 times, most recently from 8682172 to 95bfdc9 Compare April 21, 2025 12:17

benjamin051000 force-pushed the formalize-bbox branch from 95bfdc9 to e47f594 Compare April 25, 2025 23:00

benjamin051000 added 2 commits April 25, 2025 20:28

Formal bounding box type BBox.

a3f0d5e

`BBox` represents a *valid* bounding box. It's only possible to have a BBox object that's valid.

Update GUI bbox to use comma delim. Add gui bbox errors.

c3ce882

benjamin051000 force-pushed the formalize-bbox branch from e47f594 to c3ce882 Compare April 26, 2025 00:28

louis-e merged commit 2fb09d1 into louis-e:main Apr 26, 2025
3 checks passed

benjamin051000 deleted the formalize-bbox branch April 27, 2025 02:30

Uh oh!

Conversation

benjamin051000 commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To Do

Uh oh!

benjamin051000 commented Feb 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjamin051000 commented Apr 10, 2025

Uh oh!

louis-e commented Apr 12, 2025

Uh oh!

louis-e commented Apr 13, 2025

Uh oh!

XianlinSheng commented Apr 13, 2025

Uh oh!

louis-e commented Apr 13, 2025

Uh oh!

XianlinSheng commented Apr 13, 2025

Uh oh!

benjamin051000 left a comment

Choose a reason for hiding this comment

Uh oh!

benjamin051000 Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjamin051000 Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

benjamin051000 Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

benjamin051000 commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamin051000 commented Apr 22, 2025

Uh oh!

louis-e commented Apr 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjamin051000 commented Feb 20, 2025 •

edited

Loading

benjamin051000 Apr 20, 2025 •

edited

Loading

benjamin051000 commented Apr 21, 2025 •

edited

Loading