Skip to content
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

Split out workcell editor #239

Draft
wants to merge 26 commits into
base: luca/model_workflows
Choose a base branch
from

Conversation

luca-della-vedova
Copy link
Member

New feature implementation

Implemented feature

Split workcell editor out into separate app, new app in https://github.com/luca-della-vedova/rmf_workcell/. Based on #238 so that should be merged first.

Implementation description

WIP, currently cleaning up

Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Comment on lines +154 to +161
.init_resource::<CurrentEditDrawing>()
.init_resource::<CurrentLevel>()
.insert_resource(HighlightAnchors(false))
.add_event::<ChangePick>()
.add_event::<MoveTo>()
.add_event::<GizmoClicked>()
.add_event::<SpawnPreview>()
.add_event::<ToggleLiftDoorAvailability>()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resource initialization has been moved here from other plugins since the InteractionPlugin will panic if used in a third party app without them.
An alternative solution could be to split the interaction plugin into several sub-plugins and allow users to only include the ones they need, but that would have been a much larger refactor so I left it out for now.

.add_plugins((
CameraControlsPlugin,
ModelPreviewPlugin,
UserCameraDisplayPlugin::default(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to resource initialization, the interaction plugin panics if this plugin is not present, so I added it here rather than keeping it in the widgets to decouple widget plugin and interaction plugin.

Comment on lines -65 to +58
pub fn place_object_2d(&mut self, object: Model, level: Entity) {
pub fn place_object_2d(&mut self, object: Model) {
let Some(level) = self.current_level.0 else {
warn!("Unble to create [object:?] outside a level");
return;
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of my effort to reduce code duplication. I refactored both 2d and 3d object placement to take the same input (just a Model).
Specifically for the fuel widget, it was really the same between this app and the workcell editor, with the only different that the spawning function was different. I introduced a field in the plugin that allows users to pass a spawning function that takes as an input Commands and a Model, and will be called when the Spawn model button is clicked. This allowed reusing the code between the two applications but comes with the drawback that this minor API change is necessary.

pub struct FuelAssetBrowserPlugin {
pub model_spawner: fn(&mut Commands, Model),
}
impl Default for FuelAssetBrowserPlugin {
fn default() -> Self {
Self {
model_spawner: |commands: &mut Commands, model: Model| {
commands.place_object_2d(model);
},
}
}
}

Comment on lines +17 to +19
// Bevy plugins that are public dependencies, mixing versions won't work for downstream users
pub use bevy_egui;
pub use bevy_mod_raycast;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another interesting one that I'm considering pushing further.
If users need to use any of these plugins in their downstream applications they need to also include them in the exact same version in their Cargo.toml or might end up with very cryptic errors if two different versions are included in their application vs the site editor (since the two different versions will compile to types with the same name that are actually different and not interchangeable).

So my approach was to just make these dependencies public so users will just import them through the site editor. These two are the bare minimum needed for the workcell editor and probably most applications (bevy_egui is needed to create custom widgets, bevy_mod_raycast for raycasting for intersections with meshes / ground planes) but I'm happy to make more dependencies public if this approach sounds good.

Comment on lines -331 to -333
handle_update_fuel_cache_requests,
read_update_fuel_cache_results,
reload_failed_models_with_new_api_key,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just refactored these and the matching resources into a FuelPlugin to make it easier to add it to apps.

Comment on lines +356 to +382
#[cfg(feature = "urdf")]
impl From<Pose> for urdf_rs::Pose {
fn from(pose: Pose) -> Self {
urdf_rs::Pose {
rpy: match pose.rot {
Rotation::EulerExtrinsicXYZ(arr) => urdf_rs::Vec3(arr.map(|v| v.radians().into())),
Rotation::Yaw(v) => urdf_rs::Vec3([0.0, 0.0, v.radians().into()]),
Rotation::Quat([x, y, z, w]) => {
let (z, y, x) = glam::quat(x, y, z, w).to_euler(glam::EulerRot::ZYX);
urdf_rs::Vec3([x as f64, y as f64, z as f64])
}
},
xyz: urdf_rs::Vec3(pose.trans.map(|v| v as f64)),
}
}
}

#[cfg(feature = "urdf")]
impl From<&urdf_rs::Pose> for Pose {
fn from(pose: &urdf_rs::Pose) -> Self {
Pose {
trans: pose.xyz.map(|t| t as f32),
rot: Rotation::EulerExtrinsicXYZ(pose.rpy.map(|t| Angle::Rad(t as f32))),
}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these have to be implemented here, I added a default off urdf feature to remove the dependency unless needed.

Comment on lines -66 to +65
update_changed_values::<T>
.run_if(AppState::in_displaying_mode())
.in_set(SiteUpdateSet::ProcessChanges),
update_changed_values::<T>.in_set(SiteUpdateSet::ProcessChanges),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky one, similar change is in the recall plugin.
The issue we are trying to tackle is that if users wanted to use this plugin in a third app they would need to include the rmf_site_editor AppState or the run_if function would panic.
The main drawback here is that I believe this changes the system to always run. Now it could be argued that the net difference in this case (and others through this PR) is that they will additionally run in AppState::MainMenu, where there are very few entities, pretty much no work to be done and very little CPU usage, so I'd argue it's not a big issue, but still worth considering.

Comment on lines +23 to +32
pub struct WorkspaceMenuPlugin {}

impl Plugin for WorkspaceMenuPlugin {
fn build(&self, app: &mut App) {
app.init_resource::<WorkspaceMenu>().add_systems(
Update,
handle_workspace_menu_events.run_if(AppState::in_displaying_mode()),
);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new widget for workspace creation, now the top menu bar is fully modular and doesn't have any "prespawned" buttons.
The reason this is needed is again modularity and code duplication. The top menu bar buttons forced the widget to rely on internal workspace creation / file loading / file saving events, this meant that users that wanted custom workflows for these functions would need to duplicate the whole top menu bar.
Moving them to a classic child system means that users just need to add their children to the file menu instead.

This is necessary because these kind of file operations will most likely have to be customized in different apps. For example a file loading might need custom extension filters, a different loaded file handler etc.

Comment on lines -187 to -203
.add_event::<SaveWorkspace>()
.add_event::<SaveWorkcell>()
.add_event::<LoadWorkcell>()
.init_resource::<CurrentWorkspace>()
.init_resource::<RecallWorkspace>()
.init_resource::<SaveWorkspaceChannels>()
.init_resource::<FileDialogServices>()
.init_resource::<WorkspaceLoadingServices>()
.init_resource::<WorkspaceSavingServices>()
.add_systems(
Update,
(
dispatch_new_workspace_events,
sync_workspace_visibility,
workspace_file_save_complete,
),
(dispatch_new_workspace_events, sync_workspace_visibility),
);
#[cfg(not(target_arch = "wasm32"))]
app.add_systems(Update, dispatch_save_workspace_events);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several changes to this file, I'll just highlight them in this snippet that offers an overview.

Model saving now uses workflows, furthermore I separated the rfd file dialog spawning into a new FileDialogServices that provides workflows that allow users to pick files for loading / saving (optionally with filters for extensions), as well as picking folders for exporting (useful for SDF / URDF exporting).

Apps that need to handle workspaces will most likely need to create their own loading and saving services, however now they can reuse the FileDialogServices, as well as avoid having to worry about rfd so we can keep it as a private dependency.
Note that there are other parts in the codebase where we could use the new service rather than rfd (specifically three instances, saving lights, saving navgraphs and browsing local asset sources) however I left them out for now to try to limit the scope of this PR, I added TODOs in their place.

Comment on lines +98 to +100
pub mod primitive_shape;
pub use primitive_shape::*;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved primitive shapes in the root of rmf_site_format from the workcell module. This is necessary since SDFs might contain primitive shapes (usually for collision meshes)

@mxgrey mxgrey self-requested a review October 10, 2024 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

1 participant