Rotation version based on map transform #572
Conversation
…n bug: the include_str macro reads the string statically at compile time and thus should not be used.
|
retrigger-benchmark |
|
⏱️ Benchmark run finished in 38m 48s 📈 Compared against baseline: 135s 🚨 This PR drastically worsens generation time. You can retrigger the benchmark by commenting |
|
OK this is quite weird. 2 benchmarks, one with 2 translations + 2 rotations using 5 smooth steps, 6% more peak memory; one without any extra works in this module, 4% more peak memory and same time. Tests still finish within a few seconds locally. |
|
Don't worry about the benchmark bot, I've already seen that happening one time before. Another retrigger fixes it. I'll investigate that soon! Btw I'll get to your PR asap! :) |
louis-e
left a comment
louis-e
left a comment
There was a problem hiding this comment.
Incredibly solid implementation, and thanks for the good test coverage. I've briefly read the entire diff and don't have many code review remarks. I also learned a bit more about Rust now :D
Follow-up idea: After merge, let’s hook the GUI/CLI angle from #571 into this operator so rotation lives in one place (the transformation pipeline), and we reuse elevation smoothing & polygon bbox. However, as pointed out in #571 by me, I want to take both of your and @13raur0's input into account for this decision!
| let opjson_string = include_str!("../../tests/map_transformation/example_transformations.json"); | ||
| let opjson = serde_json::from_str(opjson_string) | ||
| .expect("Failed to parse map transformations config json"); | ||
| match fs::read_to_string("tests/map_transformation/map_transformations.json") { |
There was a problem hiding this comment.
Using fs::read_to_string instead of include_str! means that release builds won’t bundle defaults, and missing file silently skips transforms. I assume this is intended for now to make changes easier? Just double checking!
There was a problem hiding this comment.
I changed it back because in my tests include_str! seems to be reading it at compile time only and the content will be fixed. This will be taken good care of once we connect the GUI and this config file.
| XZPoint::new(bbox.max().x as i32, bbox.max().y as i32), | ||
| ) | ||
| .map_err(|e| format!("Polygon bounding box error:\n{}", e))?; | ||
|
|
There was a problem hiding this comment.
Rasterizing the polygon mask over the bounding rect is O(W*H); this is fine for normal areas but won't it balloon on huge selections?
There was a problem hiding this comment.
It might, but peak memory usage is only 2% higher in the benchmark. Since the elements on the map also scales with the area, I suppose this ratio will stay in the order of constant. I did this to increase inference performance, but if you want some more optimizations on it there are ways to compress it and balance between speed and memory.
|
Yes it sounds good, next step will be to link it to GUI and let's find out what's the best way to integrate them. Preliminary thoughts will be in the way how vscode config is designed, adding a general display GUI for json config file. But note that my computer ran into some hardware problems and might take some days before I can start coding. |
2 participants
There are multiple versions of rotation right now so some comparisons and discussions might be necessary before merging and combine the optimal part in each.
In this version, the rotation:
is implemented as a map transform operator, and thus can be flexibly combined with other operations
includes a polygon xzbbox, which will still be needed when doing other transforms like clipping. It seems there are no extra elements outside the region in another version, yet I got them out there and the generation of those needs to be deactivated when I tested in the past, so better understand what this is done now.
calculates the new elevation data by calling the existing ground method
includes a new interface for elevation data and ground based on Array2 which is a data structure more RAM/CPU efficient than Vec<Vec<...>>, maybe useful as a reference for future optimizations in other modules
includes a laplacian smooth on the rotated elevation data to deal with jagged edges due to coordinate resolution
includes settings: rotation center, angle, and smooth iterations for users
is based on json config only, and the config file is still fixed in path and cannot be specified at runtime
does not have GUI portal and other helper tools like degree calculator
truncates the floating point in more places for elevation, given the consideration of modularity, so introduces some numerical errors and diffusivities, but still acceptable compared to the resolution of mc coordinates and original elevation data.
is done on nodes, ways elements, relation is stay untouched, so I'd better make it clear the data structure and usages of relations.
Some important questions were bolded.
As one of the possible directions, the GUI might integrate the map transform module to provide a job workflow, which should not be too difficult since the current interface is already in json just like how vsode config do things, together with tools in other codes, this might give a pretty smooth user experience.
Some results:
Fig.1 Without any rotations
Fig.2 Rotation 45deg and rotate -45deg back, smooth iterations = 0 per rotation
Fig.3 Rotation 45deg and rotate -45deg back, smooth iterations = 3 per rotation
Fig.4 Rotation 45deg and rotate -45deg back, smooth iterations = 5 per rotation
Fig.5 Rotation 30deg, smooth iterations = 0 per rotation
Fig.6 Rotation 30deg, smooth iterations = 5 per rotation