Skip to content

Conversation

@modrimkus
Copy link
Contributor

Purpose of this PR

This PR fixes an issue where some Gizmos Menu items would be missing in projects that have the ProBuilder package installed.

Links

Fogbugz: https://fogbugz.unity3d.com/f/cases/1332226

Comments to Reviewers

Sequence of events causing this issue:

  • User selects to install PB package or opens a project that contains PB
  • Domain reload is triggered
  • GizmoSetup (C++) receives OnBeforeDomainReload and clears all gizmo renderers and setups
  • AnnotationManager (C++) receives OnDomainReload and sets its dirty flag
  • [InitializeOnLoad] attributes are processed and PB HierarchyListener's constructor is called which in turn calls AnnotationUtility.GetAnnotations()
  • AnnotationManager.GetAnotations() internally calls AnnotationManager.Refresh() which, due to the dirty flag being set previously, attempts to rebuild annotations from an empty GizmoSetup and clears the dirty flag
  • When eventually OnDidReloadDomain triggers, GizmoSetup rebuilds gizmo renderers and setups
  • User opens Gizmos Menu
  • AnnotationManager.Refresh() (C++) is called but it early exits due to the previously cleared dirty flag. This is when AnnotationManager would normally rebuild annotations but, due to HierarchyListener forcing an early AnnotationManager.Refresh(), the dirty flag has the wrong state.

Proposed fix:

Delay HierarchyListener's call to AnnotationUtility.GetAnnotations() by registering for AssemblyReloadEvents.afterAssemblyReload callback during HierarchyListener's initialization. Using EditorApplication.delayCall delays it further and ensures there's no race condition between HierarchyListener and GizmoSetup's handling of OnDidReloadDomain.

Tested opening existing projects that contain PB and adding/removing PB in open projects with 2019.4, 2020.3 and 2021.2.012a

@modrimkus modrimkus requested a review from karl- as a code owner April 29, 2021 13:13
Copy link
Contributor

@karl- karl- left a comment

Choose a reason for hiding this comment

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

nice detective work 🥇

Copy link
Collaborator

@lopezt-unity lopezt-unity left a comment

Choose a reason for hiding this comment

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

Looks good, nice catch

@karl- karl- added the backport label Apr 29, 2021
@OndrejPetrzilka
Copy link

Nice fix! I was expecting some "initialization order issue", but wasn't able to find it.

Copy link
Contributor

@JoelFortin JoelFortin left a comment

Choose a reason for hiding this comment

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

This one bothers me since a while ;)
LGTM.

@modrimkus modrimkus merged commit 68cd619 into master Apr 30, 2021
@modrimkus modrimkus deleted the bugfix/1332226-missing-gizmos-menu-items branch April 30, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants