Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

Fix for transform subscriptions when actor is an attachment #745

Conversation

norybiak
Copy link
Contributor

This fix allows transform subscriptions to pass to the app when actor is an attachment.

Issue #739

This PR is similar to #740 but for the correct branch (red).

Copy link
Member

@tombuMS tombuMS left a comment

Choose a reason for hiding this comment

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

Some protection regarding potentially undefined syncActor needs to be added. See comments

@@ -305,8 +305,10 @@ export const Rules: { [id in Payloads.PayloadType]: Rule } = {
message: Message<Payloads.ActorCorrection>
) => {
const syncActor = session.actorSet.get(message.payload.actorId);
if (syncActor && ((client.authoritative && !syncActor.grabbedBy)
|| (syncActor.grabbedBy === client.id))) {
const attachment = syncActor.initialization.message.payload.actor.attachment;
Copy link
Member

Choose a reason for hiding this comment

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

It is possible for syncActor to be undefined here which will cause this to throw an exception. Getting the attachment will need to be moved into the if block below it where syncActor will already be tested for undefined. Alternatively you can do optional chaining on the syncActor like:

const attachment = syncActor?.initialization.message.payload.actor.attachment;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. See latest commit.

@@ -402,8 +404,10 @@ export const Rules: { [id in Payloads.PayloadType]: Rule } = {
message: Message<Payloads.ActorUpdate>
) => {
const syncActor = session.actorSet.get(message.payload.actor.id);
const attachment = syncActor.initialization.message.payload.actor.attachment;
Copy link
Member

Choose a reason for hiding this comment

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

See same comment above about syncActyor being undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. See latest commit.

@tombuMS tombuMS merged commit 59c975a into microsoft:red Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants