-
-
Notifications
You must be signed in to change notification settings - Fork 763
Add rotate-and-expand with GUI and CLI #571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- GUI: added rotation angle input (form + slider, up to 0.01° precision) - Map: added Polyline drawing button to measure angles from two points - generate_world: apply rotation before generation via rotate_and_expand - allow editing elevation_data in ground - expand bbox to fit all rotated coordinates (may increase chunk count) - padding generated at height -62 - CLI: added angle option (--help for details)
|
Your GUI poly line part is unique and seems very useful, but in the core of rotation there are some differences with my implementation. I will open my version and try to explain the reason of every part of my code. You can do something similar. Let's make sure we didn't miss something important and try to combine the useful parts. |
XianlinSheng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to make sure some details. The new xzbbox seems to be the circumscribed rectangle of the rotated one, because it must be xz aligned. How is it guaranteed that the .contains(point) is inside the polygon? I introduced the contains() function because it seems that not all fetched elements are exactly inside the selected region, but including all elements that are partially inside. Did you encounter this situation? I ran the test to check the fetched data long time ago and am not sure what it is done now.
|
Thank you for checking the PR! In my implementation, Elements outside the original xzbbox range are never added, and partially contained elements remain unchanged. |
|
you may try cargo fmt and cargo clippy to clean up the code. I'll try to run some local tests and make sure the differences in our codes pose no problems in the understanding of the data structure. |
|
I did some tests on this PR and mine. First of all, I appreciate that this reminded me some details about transforms on relations that I forgot. Secondly, I found out that either the API or processed elements seem to behave a little different from what I tested couple of months ago, about whether the API will truncate the way elements at the edge of bounding box. From my previous tests and knowledge on this API, it should return complete relations and ways, but now both PR showed that the elements are truncated at the bbox edge. Truncation reduces half of the workload on transformation because XZBBox has less requirements but might introduce more complexity for batching algorithms. Either case, I will say that let's make this understanding clear first. |
|
Hi there, thanks for the great discussion! I'm really busy with work at the moment, but I promise I will review all the open pull requests as soon as possible. Sorry for the delay! :) |
louis-e
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding GUI/CLI rotation, this is super useful. I can also see this in the release with an automatic building alignment detection which we could add later. I have a few code review remarks and I'd like to have more of @XianlinSheng's input to decide what can be merged of both PRs to get the best out of it, before we can merge. PR #572 introduces a more general rotation operator with polygonal bboxes. I’d love to reuse that core and keep this PR focused on the GUI/CLI plumbing. But I want to take both of your and @XianlinSheng's input into account for this decision :)
| map_transformation::transform_map(&mut parsed_elements, &mut xzbbox, &mut ground); | ||
|
|
||
| //rotate_and_expand | ||
| let _ = rotate_and_expand::rotate_and_expand_world_data(&mut parsed_elements,&mut xzbbox,&mut ground, args.rotation_angle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a validator (or clamp) on Args::rotation_angle? GUI clamps to 0–90; CLI should match
| const angle = calculateAngleGeo(latlngs[0], latlngs[1]); | ||
| const rotation = getRotationToNearestAxis(angle); | ||
|
|
||
| alert('Rotation for input (counterclockwise): ' + rotation.toFixed(2) + '°'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find a better user feedback than using alert?
|
|
||
| <!-- Rotation Angle Input --> | ||
| <div class="settings-row"> | ||
| <label for="rotation-angle-slider" data-localize="rotation_angle">Rotation Angle (°)</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the data-localize tag cause troubles during a GUI language switch if the tag has no translations in the yaml files? If yes we can just add the tag later again!
|
Thanks for the feedback! |
This PR adds support for rotating the world before generation.
The purpose is to allow users to specify a rotation angle to fix jagged map features.
For example, in Manhattan, most roads and buildings appear jagged because the city is tilted on the map.
Although a similar feature has been proposed before, I am sharing my code for reference.
This is mostly experimental; feedback is welcome.
Note: "expand" here means adding padding to fit all rotated coordinates, not scaling the map itself.
I’m a fan of Arnis and look forward to its future development!