Worker Inspector window & Entity List #1375

jamiebrynes7 · 2020-05-27T11:01:02Z

Description

This PR is the first in a series for the Worker Inspector window. In it, we find:

The window definition itself
A UI element for listing and selecting entities.
Searching over the list of entities by entity ID and entity type (metadata)
Relatively efficient list rebuilding
- Only rebuilds when entities have been added/removed.
- With 10k SpatialOS entities in the Editor => ~2-3ms for a full rebuild
- With 100k SpatialOS entities in the Editor => ~20-30ms for a full rebuild
- (On a i7 7700k)
- Possible optimizations in the future would be to use the EntityManagerDiffer to track changes, but this would require the underlying data storage to be equipped for fast additions/removals. For now, I feel what we have is acceptable performance
- Or alternatively jobify the list building.
Some other misc changes (sortable EntityId structs, hacky ViewVersion for diff tracking)

A quick screenshot, the right hand panel will be used for displaying the detail for a given entity.

Tests

Some quick perf tests on my local machine, and for searching.
Unit tests for the searching.

Documentation

I'll add a changelog once the rest is implemented

workers/unity/Packages/io.improbable.gdk.core/Systems/EntitySystem.cs

workers/unity/Packages/io.improbable.gdk.debug/Improbable.Gdk.Debug.asmdef

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityList.cs

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/WorkerInspectorWindow.cs

zeroZshadow · 2020-05-27T11:38:25Z

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/WorkerInspectorWindow.cs

+
+        private void OnEntitySelected(EntityId entityId)
+        {
+            UnityEngine.Debug.Log($"Selected {entityId}");


Debug stuff that is removed later?

Yes, just a placeholder for now.

austinkelmore · 2020-05-27T11:35:46Z

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityList.cs

+
+        public EntityList()
+        {
+            const string uxmlPath = "Packages/io.improbable.gdk.debug/WorkerInspector/Templates/EntityList.uxml";


Do we have any kind of system or central place to put hard coded references to other packages/assets? Not saying we need to do that here if it's a one-off, but seeing this made me think of it.

Nope we don't. Tbh I'd be against a central place since its only referenced here and the locality is nice.

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityList.cs

austinkelmore · 2020-05-27T12:17:55Z

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityList.cs

+            {
+                unchecked
+                {
+                    return (EntityId.GetHashCode() * 397) ^ (Metadata != null ? Metadata.GetHashCode() : 0);


Is 397 a magic number? I'm not familiar with what it means here.

This is the magic auto-impl of GetHashCode() by Rider. I believe 397 is a prime number which plays some role in this.

austinkelmore · 2020-05-27T12:20:07Z

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/WorkerInspectorWindow.cs

+            rootVisualElement.styleSheets.Add(stylesheet);
+
+            worldSelector = rootVisualElement.Q<WorldSelector>();
+            worldSelector.OnWorldChanged += OnWorldChanged;


Do we need to manually remove these callbacks if the window is closed?

If the window is closed, the world selector element is removed along with it, so its all GC'ed

...ckages/io.improbable.gdk.debug/Tests/Editmode/WorkerInspector/EntitySearchParametersTests.cs

paulbalaji · 2020-05-29T11:51:31Z

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityListData.cs

+using Improbable.Gdk.Core;
+using Unity.Collections;
+using Unity.Entities;
+using Unity.Profiling;


still need this?

Yup, there are ProfilerMarkers on L12-13.

SonarCloud will flag unused imports now as a code smell 😁

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityList.cs

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityListData.cs

zeroZshadow · 2020-05-29T11:57:10Z

workers/unity/Packages/io.improbable.gdk.debug/WorkerInspector/EntityListData.cs

+        private EntityQuery query;
+        private EntitySearchParameters searchParameters;
+
+        public void ApplySearch(EntitySearchParameters searchParameters)


ApplyFilter?

I prefer ApplySearch since the parameter is of type EntitySearchParameters

sonarqubecloud · 2020-05-29T13:12:04Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

39.3% Coverage
0.0% Duplication

improbable-prow-robot added jira/no-ticket Indicates a PR has no corresponding JIRA ticket do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 27, 2020

improbable-prow-robot requested review from austinkelmore and zeroZshadow May 27, 2020 11:01

improbable-prow-robot added A: core Area: Core GDK size/XL Denotes a PR that changes 300-599 lines, ignoring generated files. labels May 27, 2020

zeroZshadow suggested changes May 27, 2020

View reviewed changes

austinkelmore reviewed May 27, 2020

View reviewed changes

improbable-prow-robot added size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 300-599 lines, ignoring generated files. labels May 27, 2020

jamiebrynes7 force-pushed the feature/worker-inspector-window branch from bf86df6 to 5489e0c Compare May 27, 2020 15:51

jamiebrynes7 marked this pull request as ready for review May 27, 2020 15:51

improbable-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2020

paulbalaji reviewed May 27, 2020

View reviewed changes

...ckages/io.improbable.gdk.debug/Tests/Editmode/WorkerInspector/EntitySearchParametersTests.cs Show resolved Hide resolved

jamiebrynes7 requested review from austinkelmore, zeroZshadow and paulbalaji May 28, 2020 10:37

jamiebrynes7 mentioned this pull request May 28, 2020

Add first class support for 'Editor-only' generated code #1376

Merged

2 tasks

jamiebrynes7 added 6 commits May 29, 2020 12:24

make EntityId comparable

ac8c7e0

add worker inspector window

42a8c2a

add entity list backing data

1d88ea1

add entity list UI element

27c520e

add tests

f57c8c0

meta & asmdef that I forgot to put in the other commits

f36e21e

jamiebrynes7 force-pushed the feature/worker-inspector-window branch from 5489e0c to f36e21e Compare May 29, 2020 11:24

paulbalaji approved these changes May 29, 2020

View reviewed changes

zeroZshadow reviewed May 29, 2020

View reviewed changes

CR

fa117ac

zeroZshadow approved these changes May 29, 2020

View reviewed changes

jamiebrynes7 merged commit 8e37957 into develop May 29, 2020

improbable-prow-robot deleted the feature/worker-inspector-window branch May 29, 2020 13:23

jamiebrynes7 mentioned this pull request Jun 1, 2020

Add entity detail pane #1379

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Worker Inspector window & Entity List #1375

Worker Inspector window & Entity List #1375

jamiebrynes7 commented May 27, 2020 •

edited

Loading

zeroZshadow May 27, 2020

jamiebrynes7 May 27, 2020

austinkelmore May 27, 2020

jamiebrynes7 May 27, 2020

austinkelmore May 27, 2020

jamiebrynes7 May 27, 2020

austinkelmore May 27, 2020

jamiebrynes7 May 27, 2020

paulbalaji May 29, 2020

jamiebrynes7 May 29, 2020

zeroZshadow May 29, 2020

jamiebrynes7 May 29, 2020

sonarqubecloud bot commented May 29, 2020

Worker Inspector window & Entity List #1375

Worker Inspector window & Entity List #1375

Conversation

jamiebrynes7 commented May 27, 2020 • edited Loading

Description

Tests

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 29, 2020

jamiebrynes7 commented May 27, 2020 •

edited

Loading