-
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
Add display for double doors #138
Merged
Merged
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
59e7eac
Add dynamics based door behavior
luca-della-vedova 9b71961
Remove rapier simulation, add update from double to single door
luca-della-vedova 365d74f
Restore rapier to only workcell mode
luca-della-vedova 23356fc
Cleanup
luca-della-vedova 9ac1f72
Add stub for door model to avoid todo panics
luca-della-vedova 9c6ab74
Add support for door left_right_ratio
luca-della-vedova 2d567db
Minor cleanup
luca-della-vedova 5461c93
Optimize outline update for new meshes
luca-della-vedova 7248fc1
Merge branch 'main' into luca/door_preview
luca-della-vedova 5b6e2fa
Merge remote-tracking branch 'origin/main' into luca/door_preview
luca-della-vedova e2b35dd
Change outline update to use set_changed()
luca-della-vedova 2ab9f36
Merge remote-tracking branch 'origin/main' into luca/door_preview
luca-della-vedova File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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
andSelected
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:
Hovered
component when a new door is spawned. Works and only a few lines but a bit hacky.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 moduleThere 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.
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 thanChanged<Children>
. AlsoAncestorIter
goes up the parentage and should perform way less iterations than a full iteration of all descendents.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.
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()
.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.
Changed to manual
set_changed
toHovered
component e2b35dd