-
Notifications
You must be signed in to change notification settings - Fork 13
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
Use workflows for mouse interactions #236
Conversation
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
…tors Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
This feature is necessary for workcell calibration, the sequence is usually the following:
If we cannot reparent a mesh to a different anchor we can't do this calibration workflow, so the functionality is needed. The most important point however, is in the redesign of the parenting UX. It is a deliberate design choice in the current workcell editor that the only entity that can be a parent of another entity is a
I'm attaching a videos of a few of the rough edges that came up while allowing models to be parents of other entities: For example when spawning a model as a child of a model the model disappears, frames that are children of models seem to be embedded in the model when saving and reloading. TBH I would just only allow frames as parents to avoid all of these and make the URDF interoperability easier. Additionally, there was a check in place to make sure we only do the "mesh snapping" when hovering over models. This is necessary to avoid snapping to the anchor meshes as the video below (which is probably not something that a normal user would want): Screencast.from.2024-08-22.12-19-04.webmI need to setup the rest of the infrastructure to test the rest of the workcell editor pipeline, but these are the points I could find immediately Edit: Ah I just noticed I didn't capture the mouse pointer :| Apologies for that! I hope the videos still make sense |
I see, the only reason I didn't include it in this PR is because I wasn't sure how to preview it to the user. I thought we'd want to snap the model back to the cursor while selecting the new parent, and that would've been very difficult to propagate properties correctly. What you've described, where the object remains at the same global transform while we reparent, will be very easy to implement, so I'll go ahead and add that in now.
This should be very easy to fix. I just need to drill up to the first ancestor frame of the selected parent before inserting the object into the world. I think down the road we should consider whether it's possible to automatically consider every tangible object in the scene to be a frame, but that's obviously outside the scope of this PR. |
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
The vanishing model issue was fixed with e9a36cf. I also tested a round-trip of saving and reloading, and everything appeared to be working: everything loaded back up, and all the parent-child relationships appeared to be in tact. I believe this addresses all the current feedback, but please let me know if I missed anything. |
Signed-off-by: Michael X. Grey <[email protected]>
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.
It's a lot to go through, I left a few minor feedback around and rough edges during my testing.
It's probably quite clear given that the only real code feedback I gave is on a small isolated module that this change is very complex to understand and I admittedly don't understand most of it. I think what is most valuable is trying to document as much as possible the complex workflows that are introduced, what their steps do, how users are supposed to configure them and a bit more explanation on the actual complex workflow APIs used.
Imo this is important especially since bevy_impulse
is in very early stages with very little applications. This is pretty much the first complex application and will probably be referenced a lot by people when trying to design their own workflows.
I'll also have to understand this in quite a lot of detail when trying to work on the workcell editor mode but I suspect that will take some weeks and it's probably not worth it to keep this PR in limbo until then.
pub fn build_select_workflow( | ||
inspector_service: Service<(), ()>, | ||
new_selector_service: Service<(), (), StreamOf<RunSelector>>, | ||
) -> impl FnOnce(Scope<(), ()>, &mut Builder) -> DeliverySettings { |
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.
It would be helpful to have some documentation on what this function is meant to achieve and what the steps do, otherwise a quite deep knowledge of bevy_impulse
will be needed to decode it
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.
pub fn build_anchor_selection_workflow<State: 'static + Send + Sync>( | ||
anchor_setup: Service<BufferKey<State>, SelectionNodeResult>, | ||
state_setup: Service<BufferKey<State>, SelectionNodeResult>, | ||
update_preview: Service<(Hover, BufferKey<State>), SelectionNodeResult>, | ||
update_current: Service<(SelectionCandidate, BufferKey<State>), SelectionNodeResult>, | ||
handle_key_code: Service<(KeyCode, BufferKey<State>), SelectionNodeResult>, | ||
cleanup_state: Service<BufferKey<State>, SelectionNodeResult>, | ||
anchor_cursor_transform: Service<(), ()>, | ||
anchor_select_stream: Service<(), (), (Hover, Select)>, | ||
keyboard_just_pressed: Service<(), (), StreamOf<KeyCode>>, | ||
cleanup_anchor_selection: Service<(), ()>, | ||
) -> impl FnOnce(Scope<Option<Entity>, ()>, &mut Builder) { |
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.
This is also a fairly complex function, maybe a breakdown of what each service that users are expected to set is supposed to do would help?
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.
Service expectations outlined here.
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Last nit is that this PR introduces a bunch of extra warnings, like unused variables and imports that increase noise and make it more likely that we will miss important ones in the future. Should make sure to clean them up before merging |
Ah yeah actually quite a few warnings that have been lingering were kept on purpose as reminders for issues that need to be addressed in the old mouse selection state machine, so this is a good opportunity to clean up any remaining warnings. But I don't look forward to the day we try to apply clippy to this project... |
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
I think all the latest feedback is addressed. I also realized that I accidentally got rid of the ability to deselect the current inspected item by pressing the |
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 iterating!
This PR removes the
InteractionMode
state machine and replaces it with a high-level workflow for switching between mouse interaction services. This high-level workflow uses service injection to allow custom mouse interaction services to be plugged in by downstream users of the site editor.We introduce several different mouse interaction services that can activated by the high-level workflow. Each of these mouse interaction services is implemented as its own workflow:
Some other mouse interaction workflows that we should consider adding in the future (left out of this PR since they were not already available or not well supported):
This PR introduces some major changes to 3D object placement for the workcell editor. Placing a 3D object now involves an optional step of selecting a parent reference frame for the object. The workflow goes like this:
All of this is communicated to the user through a combination of cursor-attached previewing and a new cursor-attached tooltip feature.
This PR also introduces some crucial fixes to model loading which had some reliability issues around potentially random system ordering and incomplete property propagation.