Put together a slightly different worker model #844

samiwh · 2019-03-15T16:21:19Z

Description

Mocked up what a csharp GDK worker might look like.

jamiebrynes7

General thoughts - quite like the direction this is going but think we can consolidate some of the abstractions.

jamiebrynes7 · 2019-04-01T20:48:52Z

...kages/com.improbable.gdk.core/Worker/ConnectionHandlers/SpatialOSConnectionHandlerBuilder.cs

+
+        private ReceptionistConfig? receptionistConfig;
+        private LocatorConfig? locatorConfig;
+        private AlphaLocatorConfig? alphaLocatorConfig;


Wish we have C# unions 😕

Alternatively - perfect use case for https://github.com/mcintyre321/OneOf but sadly external dependencies 😭

We can make this a union if we really want to. I wasn't changing any of this code though, just rearranging it.

Also I think we can get rid of alpha locator though.

nope, not of the alpha locator :P we could get rid of the locator, if you wanna reduce complexity.

Discussed offline - can't get rid of the locator.

jamiebrynes7 · 2019-04-01T20:50:51Z

workers/unity/Packages/com.improbable.gdk.core/Worker/ConnectionUtils.cs

+            {
+                var deploymentList = await GetDeploymentList(locator);
+
+                var deploymentName = parameters.DeploymentListCallback(deploymentList);


Hmmm - we should allow for the possibility that this callback can be asynchronous.

I.e - user displays to the client and they select a deployment

we probably don't want that. the login token that is provided by the launcher will most likely time out by the time the user selects a deployment.
If users want to do that, they need to add quite a bit of additional logic and will most likely need to use the alpha locator together with dev auth flow or their own auth server.

This was just rearranging what was there. If we can't select a deployment without a token and we can't keep a token after selecting a deployment that is a problem though.

I agree, I had many discussions about this...

jamiebrynes7 · 2019-04-01T20:53:24Z

workers/unity/Packages/com.improbable.gdk.core/Worker/Worker.cs

-            string workerType,
-            ILogDispatcher logger,
-            Vector3 origin)
+        public static async Task<Worker> CreateWorkerAsync(IConnectionHandlerBuilder connectionHandlerBuilder, string workerType,


It feels like the connection builder should be extended into a WorkerBuilder. I.e. -

var worker = await new WorkerBuilder(workerType) .WithConnectionConfig(connectionConfig) .WithLogger(logDispatcher) .AsMultithreaded() .ConnectAsync();

I didn't do that because you might want different types of connection handler. The IConnectionHandlerBuilder interface is basically a type safe way of transferring ownership.

The current connection handler builder is to build the standard one you might one when running spatial. If you want a test one or a remoting one you want different options. We could write one massive builder that can encompass every useful permutation but that seems like a bad idea even if possible.

We could instead set it up so that there is a single connection handler with a pipeline of behaviours and we feed it in different way (ie what it considers the simulation can be rewritten). Then we can potentially just have a single builder and merge it with the worker creation process.

I have looked into doing this and have some rough design notes if you want to go over them. (It's also probably what I'd do if starting now)

Pls send design notes

Currently writing a doc for a slightly different version. Can go through that and work out how it might work in C# too.

jamiebrynes7 · 2019-04-01T20:55:51Z

workers/unity/Packages/com.improbable.gdk.playerlifecycle/PlayerLifecycleHelper.cs


-            return worker.Connection.GetWorkerId() == ownerId;
+            var ownerId = updateSystem.GetComponent<OwningWorker.Snapshot>(entityId).WorkerId;


Why would you grab this off the updateSystem rather than the view?

The view isn't currently public. The update system is how you access it.

That's bizarre

It's basically a stepping stone. Update system was written for something else. Made sense as an interface for a bit, and now doesn't. It's mostly why the class wasn't mentioned in the release notes. It would be best to remove it and make a new thing because it's just a badly named interface at the moment.

jessicafalk · 2019-04-02T14:04:21Z

workers/unity/Packages/com.improbable.gdk.core/Worker/ConnectionUtils.cs

+
+        private static string GetConnectionFailureReason(Connection connection)
+        {
+            return connection.GetConnectionStatusCodeDetailString() ?? "No reason found for connection failure";


can this actually be null?
what about the status code?

It shouldn't be. I didn't write this though, just moved it I think.

jessicafalk · 2019-04-02T14:09:33Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerConnector.cs

                var chosenService = GetConnectionService();
                var connectionParameters = GetConnectionParameters(workerType, chosenService);
+                SpatialOSConnectionHandlerBuilder connectionHandlerBuilder = new SpatialOSConnectionHandlerBuilder();


now that we have the ConnectionHandlerBuilder, it would be really nice to get rid of the WorkerConnector. If we can make it a WorkerBuilder as Jamie suggested, I think we can get rid of this and simply provide sample implementations of how to use it for PC vs Mobile.
This would allow us to finally get rid of the many levels of inheritance we have right now

I wanted to keep Unity and non Unity separate. The worker connector is glue between the worker and Unity, so I suspect that some form of it will still be useful.

jessicafalk · 2019-04-02T14:09:45Z

workers/unity/Packages/com.improbable.gdk.core/Worker/WorkerInWorld.cs

+        ///     The origin of the worker in global Unity space.
+        /// </summary>
+        public readonly Vector3 Origin;
+


nit: empty line

jamiebrynes7 · 2019-06-11T10:44:34Z

Rebased the branch - logging appears to be broken and some functionality that was added since this PR was opened needs to be added back in

jamiebrynes7 · 2019-06-19T17:12:45Z

Superseded by #981

* Added known issue to known-issues doc * Update known-issues.md * Update known-issues.md * Update SpatialGDK/Documentation/known-issues.md Co-Authored-By: aleximprobable <48994762+aleximprobable@users.noreply.github.com>

improbable-prow-robot added A: core Area: Core GDK A: player-lifecycle Area: Player lifecycle feature module size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. labels Mar 15, 2019

jamiebrynes7 reviewed Apr 1, 2019

View reviewed changes

jessicafalk reviewed Apr 2, 2019

View reviewed changes

jamiebrynes7 added this to the Release v0.2.4 milestone May 24, 2019

jamiebrynes7 force-pushed the feature/csharp-gdk-worker-structure branch from 3b4d754 to a6b5bbe Compare June 11, 2019 10:43

Put together a slightly different worker model

f345370

jamiebrynes7 force-pushed the feature/csharp-gdk-worker-structure branch from a6b5bbe to f345370 Compare June 11, 2019 10:52

fix logging + driveby of #753

75348ff

This was referenced Jun 12, 2019

Worker abstraction rework #981

Merged

Use Unity logger types in ForwardingDispatcher #987

Merged

jamiebrynes7 closed this Jun 19, 2019

jamiebrynes7 deleted the feature/csharp-gdk-worker-structure branch July 18, 2019 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put together a slightly different worker model #844

Put together a slightly different worker model #844

samiwh commented Mar 15, 2019

jamiebrynes7 left a comment

jamiebrynes7 Apr 1, 2019

samiwh Apr 15, 2019

samiwh Apr 15, 2019

jessicafalk Apr 15, 2019

jamiebrynes7 Apr 15, 2019

jamiebrynes7 Apr 1, 2019

jessicafalk Apr 2, 2019

samiwh Apr 15, 2019

jessicafalk Apr 15, 2019

jamiebrynes7 Apr 1, 2019

samiwh Apr 15, 2019

samiwh Apr 15, 2019

jamiebrynes7 Apr 15, 2019

samiwh May 2, 2019

jamiebrynes7 Apr 1, 2019

samiwh Apr 15, 2019

jamiebrynes7 Apr 15, 2019

samiwh May 2, 2019

jessicafalk Apr 2, 2019

samiwh Apr 15, 2019

jessicafalk Apr 2, 2019

samiwh Apr 15, 2019

jessicafalk Apr 2, 2019

jamiebrynes7 commented Jun 11, 2019

jamiebrynes7 commented Jun 19, 2019


		return worker.Connection.GetWorkerId() == ownerId;
		var ownerId = updateSystem.GetComponent<OwningWorker.Snapshot>(entityId).WorkerId;

Put together a slightly different worker model #844

Put together a slightly different worker model #844

Conversation

samiwh commented Mar 15, 2019

Description

jamiebrynes7 left a 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

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

Choose a reason for hiding this comment

jamiebrynes7 commented Jun 11, 2019

jamiebrynes7 commented Jun 19, 2019