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.

Sign up for GitHub

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

Jump to bottom

gizmo plugin lag bugfix #9166

Merged
merged 2 commits into from
Jul 23, 2023
Merged

gizmo plugin lag bugfix #9166

merged 2 commits into from
Jul 23, 2023

Conversation

wpederzoli
Copy link
Contributor

@wpederzoli wpederzoli commented Jul 15, 2023 •
edited
Loading

Objective

Fixes #9156

@wpederzoli wpederzoli marked this pull request as ready for review July 15, 2023 12:42
@Selene-Amanita
Copy link
Member

I honestly don't know why the "run-examples" check was canceled, I guess a maintainer can relaunch it?

@Selene-Amanita Selene-Amanita added C-Bug An unexpected or incorrect behavior A-Gizmos Visual editor and debug gizmos labels Jul 15, 2023
@wpederzoli
Copy link
Contributor Author

I honestly don't know why the "run-examples" check was canceled, I guess a maintainer can relaunch it?

It exceeded the 30 min time limit and it got automatically canceled. But you're right, a maintainer can re-run it.
Thanks!

@wpederzoli wpederzoli changed the title run after transform system gizmo plugin lag bugfix Jul 15, 2023
@mockersf
Copy link
Member

relaunched! usually that job takes 15 to 20 minutes to run, the limit at 30 minutes should be enough...

mockersf
mockersf requested changes Jul 15, 2023
View reviewed changes
Copy link
Member

@mockersf mockersf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

TransformPropagate is in schedule PostUpdate, those systems are in Update

@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2023
@nicopap nicopap added this to the 0.11.1 milestone Jul 18, 2023
@nicopap nicopap removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2023
nicopap
nicopap suggested changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

This doesn't fix the issue, as mockersf mentioned, this can't be merged before the comments are addressed.

Also please add "Fixes #9156" to the PR description, so that github automatically closes the issue when this gets merged.

crates/bevy_gizmos/src/lib.rs Show resolved Hide resolved
@Selene-Amanita Selene-Amanita added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2023
@wpederzoli wpederzoli requested review from mockersf and nicopap July 18, 2023 14:57
@nicopap
Copy link
Contributor

nicopap commented Jul 22, 2023

@mockersf ping. Is this ready to merge?

@mockersf
Copy link
Member

mockersf commented Jul 22, 2023 •
edited
Loading

@mockersf ping. Is this ready to merge?

Technically the two other approvals than yours were when it was not fixed, so...

Did anyone check that it was fixed or is it approval "in theory"?

@mockersf mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 22, 2023
tim-blackbird
tim-blackbird approved these changes Jul 22, 2023
View reviewed changes
Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

Verified the fix by enabling aabb gizmos in the update_gltf_scene example.

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 22, 2023
Selene-Amanita
Selene-Amanita approved these changes Jul 22, 2023
View reviewed changes
Copy link
Member

@Selene-Amanita Selene-Amanita left a comment

Choose a reason for hiding this comment

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

Tested with custom code it does fix the bug

@mockersf mockersf added this pull request to the merge queue Jul 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 22, 2023
@james7132 james7132 added this pull request to the merge queue Jul 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 22, 2023
@mockersf mockersf added this pull request to the merge queue Jul 23, 2023
Merged via the queue into bevyengine:main with commit 593bebf Jul 23, 2023
@wpederzoli wpederzoli deleted the gizmo-plugin-aabb-lag branch July 23, 2023 12:04
cart pushed a commit that referenced this pull request Aug 10, 2023
@wpederzoli @cart
gizmo plugin lag bugfix (#9166)
891acb8 
# Objective
Fixes #9156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Selene-Amanita Selene-Amanita Selene-Amanita approved these changes

@tim-blackbird tim-blackbird tim-blackbird approved these changes

@nicopap nicopap nicopap approved these changes

@mockersf mockersf mockersf approved these changes

@james7132 james7132 james7132 approved these changes

@SpecificProtagonist SpecificProtagonist Awaiting requested review from SpecificProtagonist

Assignees
No one assigned
Labels
A-Gizmos Visual editor and debug gizmos C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.11.1
Development

Successfully merging this pull request may close these issues.

GizmoPlugin: AABBs are rendered with one frame lag
7 participants
@wpederzoli @Selene-Amanita @mockersf @nicopap @james7132 @tim-blackbird @SpecificProtagonist