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

Add display for double doors #138

Merged
merged 12 commits into from
Sep 4, 2023
Merged

Add display for double doors #138

merged 12 commits into from
Sep 4, 2023

Conversation

luca-della-vedova
Copy link
Member

New feature implementation

Implemented feature

This PR is part of the work towards #76, no animation is done yet to keep the PR simple but display support for double doors and asymmetric doors (left_right_ratio parameter) has been added.
Future work will be based on these changes and add animation on top to fully address the feature request.

Implementation description

A new DoorBodyType enum has been added to store the mesh entities for different door elements, the animation plugin will match this enum to figure out how to actuate doors.
Double doors will now spawn two meshes with a small gap inbetween, live updates for the left_right_ratio parameter will update the scale of the doors to match the value.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@@ -134,7 +134,7 @@ pub fn update_outline_visualization(
mut commands: Commands,
outlinable: Query<
(Entity, &Hovered, &Selected, &OutlineVisualization),
Or<(Changed<Hovered>, Changed<Selected>)>,
Or<(Changed<Hovered>, Changed<Selected>, Changed<Children>)>,
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 an unfortunate side effect of this PR. Changing a door from single to double will spawn an additional mesh but the system that updates the hovering and displays outlines will not be triggered since the Hovered and Selected components didn't change, meaning that the newly spawned door will not have its outline displayed until the mouse hovers on it again.

This approach works but the performance implication might not be great since now this system will be triggered much more often (Changed<Children> is fairly common).

Other approaches that I considered:

  • Just triggering a change detection by doing a mutable access to the root door entity's Hovered component when a new door is spawned. Works and only a few lines but a bit hacky.
  • Manually copying the Outline components when spawning a new door, probably the best option from a performance point of view but would introduce a fair bit of outline related code in the door module

Copy link
Member Author

@luca-della-vedova luca-della-vedova Jun 23, 2023

Choose a reason for hiding this comment

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

5461c93 is a tentative approach to optimize this, it's a new system that looks at newly spawned meshes, iterates over their parents and copies the outline components to the new mesh if any is found. It's a bit hard to benchmark performance but Added<Handle<Mesh>> should be triggered quite a bit less than Changed<Children>. Also AncestorIter goes up the parentage and should perform way less iterations than a full iteration of all descendents.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The outline copying system for new meshes is an interesting idea, but outline behavior is supposed to be based on VisualCue settings, not just simple propagation.

Overall I actually like the idea of dirtying the Hover component to trigger the recalculation. We do something similar here to update lifts using .set_changed().

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to manual set_changed to Hovered component e2b35dd

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

I think the changes in this PR are good, but I thought of something related that's been irking me for a while so I left a comment on it. We can address it in this PR or in a follow up, either is fine with me.

@@ -186,12 +186,26 @@ impl From<SingleSwingDoor> for DoorType {
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
pub struct DoubleSwingDoor {
pub swing: Swing,
/// Length of the left door divided by the length of the right door
pub left_right_ratio: f32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've always found this ratio to be kind of clunky and unintuitive. It's something we carried over from traffic editor, but I'm not sure it's the best representation of the split in doors.

I wonder if we should do a value 0.0 -> 1.0 representing what percentage of the frame width is for the "left" door, then the right door will just be 1.0 - x.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova merged commit 1c8cba7 into main Sep 4, 2023
5 checks passed
@luca-della-vedova luca-della-vedova deleted the luca/door_preview branch September 4, 2023 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants