Remove the View #1364

zeroZshadow · 2020-05-15T11:53:56Z

Description

Experimental change to remove the View and ViewStorage classes.
This also removes our support for AuthorityImminentLoss.

Might move the WorkerFlags from the WorkerSystem to the Worker class. Opinions welcome.

Documentation

This will need a rather heavy upgrade guide on:

Reading worker flags.
Deprecating AuthorityImminentLoss.
The view is gone, its just gone.

sonarqubecloud · 2020-05-15T12:50:18Z

Kudos, SonarCloud Quality Gate passed!

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

71.1% Coverage
0.0% Duplication

...ackages/io.improbable.gdk.core/Subscriptions/CommandSenderReceiverSubscriptionManagerBase.cs

workers/unity/Packages/io.improbable.gdk.core/Subscriptions/Reader.cs

workers/unity/Packages/io.improbable.gdk.core/Subscriptions/WriterSubscriptionManager.cs

workers/unity/Packages/io.improbable.gdk.core/Subscriptions/Reader.cs

workers/unity/Packages/io.improbable.gdk.core/Subscriptions/WriterSubscriptionManager.cs

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

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

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

workers/unity/Packages/io.improbable.gdk.core/Dynamic/ComponentDatabase.cs

jamiebrynes7 · 2020-08-05T12:19:45Z

workers/unity/Packages/io.improbable.gdk.core/Subscriptions/Reader.cs

+        public bool HasAuthority
        {
            get
            {
                if (!IsValid)
                {
-                    throw new InvalidOperationException("Cannot read authority when Reader is not valid");
+                    throw new InvalidOperationException($"Cannot read {nameof(HasAuthority)} when Reader is not valid");
                }

-                return ComponentUpdateSystem.GetAuthority(EntityId, ComponentId);
+                return EntityManager.HasComponent(Entity, ComponentAuthType);


On balance, should we make this breaking change?

What is the breaking change here?

Both the type and name of the property are changing.

We could keep this backwards compatible by changing the return to:

return EntityManager.HasComponent(Entity, ComponentAuthType) ? Authority.Authoritative : Authority.NotAuthoritative;

Since the endgame is to remove auth loss imminent, I think it's fine to make this breaking change

Right, but removing auth loss imminent doesn't mandate or require this change

I guess I just don't see the value in keeping backwards compatibility given we'll be imposing that Authority is essentially nothing but true or false

Sure it's breaking but it does simplify logic for users (no more ==/!= comparisons)

My opinion is that the bar for making a breaking change should be relatively high and be done for good reason. With this case I don't feel the rationale is strong enough, but if you two both feel otherwise, I'm not gonna die on this hill.

workers/unity/Packages/io.improbable.gdk.core/Dynamic/ComponentDatabase.cs

CHANGELOG.md

UPGRADE_GUIDE.md

paulbalaji · 2020-08-06T14:46:00Z

workers/unity/Packages/io.improbable.gdk.core/Subscriptions/Reader.cs

+        public bool HasAuthority
        {
            get
            {
                if (!IsValid)
                {
-                    throw new InvalidOperationException("Cannot read authority when Reader is not valid");
+                    throw new InvalidOperationException($"Cannot read {nameof(HasAuthority)} when Reader is not valid");
                }

-                return ComponentUpdateSystem.GetAuthority(EntityId, ComponentId);
+                return EntityManager.HasComponent(Entity, ComponentAuthType);


Since the endgame is to remove auth loss imminent, I think it's fine to make this breaking change

BryanJY-Wong

Death to ImminentLoss (and the View)!

Remove Authority from the view Remove AuthImminentLoss support

Remove ViewStorage types

UPGRADE_GUIDE.md

Co-authored-by: Paul Balaji <[email protected]>

Co-authored-by: Jamie Brynes <[email protected]>

sonarqubecloud · 2020-08-07T13:06:23Z

Kudos, SonarCloud Quality Gate passed!

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

53.5% Coverage
0.0% Duplication

zeroZshadow requested a review from jamiebrynes7 May 15, 2020 11:53

improbable-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/no-ticket Indicates a PR has no corresponding JIRA ticket size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. labels May 15, 2020

zeroZshadow force-pushed the feature/remove-view branch from c31ea31 to 41ffbf7 Compare May 15, 2020 11:54

improbable-prow-robot requested review from austinkelmore and paulbalaji May 15, 2020 11:55

improbable-prow-robot added A: core Area: Core GDK A: game-object-creation Area: Gameobject creation feature module A: player-lifecycle Area: Player lifecycle feature module A: playground Area: Playground A: transform-synchronization Area: Transform synchronization feature module labels May 15, 2020

zeroZshadow changed the title ~~Remove the View~~ (Experiment) Remove the View May 15, 2020

zeroZshadow force-pushed the feature/remove-view branch from 90ff8fa to c4abda6 Compare July 10, 2020 10:30

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

zeroZshadow force-pushed the feature/remove-view branch from c4abda6 to 77f0d99 Compare August 4, 2020 10:26

zeroZshadow changed the title ~~(Experiment) Remove the View~~ Remove the View Aug 4, 2020

jamiebrynes7 suggested changes Aug 4, 2020

View reviewed changes

zeroZshadow requested review from jamiebrynes7, BryanJY-Wong and seanjparker August 5, 2020 11:37

zeroZshadow commented Aug 5, 2020

View reviewed changes

workers/unity/Packages/io.improbable.gdk.core/Dynamic/ComponentDatabase.cs Show resolved Hide resolved

jamiebrynes7 reviewed Aug 5, 2020

View reviewed changes

zeroZshadow marked this pull request as ready for review August 6, 2020 13:44

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 Aug 6, 2020

zeroZshadow requested a review from jamiebrynes7 August 6, 2020 13:44

paulbalaji reviewed Aug 6, 2020

View reviewed changes

zeroZshadow requested a review from paulbalaji August 6, 2020 15:32

paulbalaji approved these changes Aug 6, 2020

View reviewed changes

BryanJY-Wong approved these changes Aug 7, 2020

View reviewed changes

zeroZshadow added 9 commits August 7, 2020 13:40

Remove Components from the view

e086d8e

Remove Authority from the view Remove AuthImminentLoss support

Remove entities from View

d341651

Move WorkerFlags to WorkerSystem

749b81b

Remove ViewStorage types

Making Sonar happy

05b61a2

Fix up WorkerInspector to no longer use View

7fab6b6

Feedback Jamie

71cefbd

Move worker flags to Worker

b417a0f

Fix code smells

24b5e46

Changelog + Upgrade guide

eddc861

jamiebrynes7 approved these changes Aug 7, 2020

View reviewed changes

UPGRADE_GUIDE.md Outdated Show resolved Hide resolved

Apply suggestions from code review

fb89211

Co-authored-by: Paul Balaji <[email protected]>

zeroZshadow force-pushed the feature/remove-view branch from e49bc95 to fb89211 Compare August 7, 2020 12:42

Update UPGRADE_GUIDE.md

abaa5e4

Co-authored-by: Jamie Brynes <[email protected]>

zeroZshadow merged commit 9ea8fd4 into develop Aug 7, 2020

improbable-prow-robot deleted the feature/remove-view branch August 7, 2020 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the View #1364

Remove the View #1364

zeroZshadow commented May 15, 2020

sonarqubecloud bot commented May 15, 2020 •

edited

Loading

jamiebrynes7 Aug 5, 2020

zeroZshadow Aug 5, 2020

jamiebrynes7 Aug 5, 2020

paulbalaji Aug 6, 2020

jamiebrynes7 Aug 6, 2020

paulbalaji Aug 6, 2020

jamiebrynes7 Aug 6, 2020

paulbalaji Aug 6, 2020

BryanJY-Wong left a comment

sonarqubecloud bot commented Aug 7, 2020

Remove the View #1364

Remove the View #1364

Conversation

zeroZshadow commented May 15, 2020

Description

Documentation

sonarqubecloud bot commented May 15, 2020 • edited Loading

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

BryanJY-Wong left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 7, 2020

sonarqubecloud bot commented May 15, 2020 •

edited

Loading