Adds a setting for default lane size #135
Conversation
This PR adds a setting for default lane size. It can be found by going to `Edit -> Preferences...`. We need to find a way to persist this. Note this setting does not change per-lane widths as those should be handled differently. Signed-off-by: Arjo Chakravarty <[email protected]>
Moves preferences to `rmf_site_format` crate. This is necessary for saving items. `File > Open` does not work correctly for some reason. Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
There was a problem hiding this comment.
A good start on the feature, not sure whether it's ok for the long term but probed a bit around the implementation and found a few issues that should be addressed if we want to get this in.
It seems that when the default lane width is set and a new lane is created a fair bit of glitches appear, as in the video below:
Screencast.from.2023-06-06.15-23-32.webm
Specifically the beginning and end of each lane go back to the original width. Measurements are also scaled
| // TODO(arjo): Refactor so no panics | ||
| let preferences = site_properties | ||
| .get_single() | ||
| .unwrap_or(&Default::default()) | ||
| .preferences | ||
| .unwrap_or_default(); | ||
|
|
There was a problem hiding this comment.
This won't really work when there are multiple sites opened, a better approach would be to look at the CurrentWorkspace resource which points the Entity of the current site and get the SiteProperties component of that entity.
To break it just open a project, then Ctrl-O to open a new project, this feature will then not work anymore
| let preferences = site_properties | ||
| .get_single() | ||
| .unwrap_or(&Default::default()) | ||
| .preferences | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Same as the other case of single query for site properties (a lot of these throughout the code, will not point out all the other ones)
| if props.preferences.is_none() { | ||
| props.preferences = Some(Default::default()); | ||
| println!("Setting default value"); | ||
| } |
There was a problem hiding this comment.
I'm not sure about the approach of setting the Option to Some(Default::default()) as soon as it's detected to be None, it feels like it kinda defeats the purpose of it being an option in the first place and we could just have a Default implementation with the current default lane width for preferences?
| ui.horizontal(|ui| { | ||
| ui.label("Default Lane Width: "); | ||
| let lane_width_entry = DragValue::new(&mut preferences.default_lane_width) | ||
| .clamp_range(0.0..=10000.0); |
There was a problem hiding this comment.
0.0 is a bit of a risky lower bound that would make it completely invisible, I'd suggest making it something that is not completely invisible but still covers the lowest common case (maybe 0.1 m?)
| @@ -7,6 +7,7 @@ use crate::{ | |||
| Navigation, OrientationConstraint, PixelsPerMeter, Pose, RankingsInLevel, ReverseLane, | |||
| Rotation, Site, SiteProperties, DEFAULT_NAV_GRAPH_COLORS, | |||
| }; | |||
| use bevy::prelude::default; | |||
There was a problem hiding this comment.
This is unnecessary, you can just use Default::default() below, also every bevy specific instruction in the rmf_site_format crate should be behind a bevy feature flag, we are trying to keep the basic serialization / deserialization bevy independent so third party crates can use the format without pulling in such a large dependency
|
Will fix these however in another conversation with @mxgrey he mentioned the following crate: https://github.com/ForesightMiningSoftwareCorporation/bevy_polyline This removes the need for this feature. Will probably use this instead. |
|
Closing in favor of #152 |
This PR adds a setting for default lane size. It can be found by going to
Edit -> Preferences.... We need to find a way to persist this. Note this setting does not change per-lane widths as those should be handled differently.