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

Properly sync actors created from glTF #304

Merged
merged 7 commits into from
Apr 16, 2019
Merged

Conversation

stevenvergenz
Copy link
Contributor

Prior to this PR, patches to actors created from glTF (other than the root actor) were getting stored but never sent.

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.

:shipit:

@@ -683,9 +683,10 @@ export const Rules: { [id in Payloads.PayloadType]: Rule } = {
created: {
message: {
payload: {
type: 'actor-update',
Copy link
Contributor

@eanders-ms eanders-ms Apr 15, 2019

Choose a reason for hiding this comment

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

type: 'actor-update', [](start = 44, length = 21)

Interesting. Agreed the payload needs a type, but this is going to break in the sync layer. You'll need to filter these messages out of the set returned by session.creatableChildrenOf.

There may be other places in the code that assumes the message is a create actor type message.

If this message going to sometimes be an actor-update message, the design should clearly reflect that. The created field is no longer a good name, for instance. Renaming this field might be a good way to find all the places in the code that may need some adjustment. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I see how this will get the desired result. In the create-actors stage of synchronization, these actor-update messages will be queued for delivery to the client, and will be sent once the create-actors stage completes. Maybe this design is ok after all.

Interesting to note that even if these actor-update messages were sent during the create-actors sync stage, things would still work due to message queuing in the client. actor-updates synchronization rules should be revisited. We might want to always allow them through now that they'll be queued properly client side.

Please add a comment here clearly describing how/why this works.


In reply to: 275422153 [](ancestors = 275422153)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe instead of "created", we should name the messages "initialized". This works because gltf-created actors are always children of the gltf root, so by the time we sync them, clients will have already received and queued the "create-from-gltf" that generates them. they already exist on the client, so all we have to do at this stage is update their states. Our current sync mechanism is inadequate because we sync state by updating the create message, but these actors' states are not in the create message.


In reply to: 275430377 [](ancestors = 275430377,275422153)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe

initialization: {
    message: {
         payload: {
            ...
        }
    }
}

In reply to: 275453527 [](ancestors = 275453527,275430377,275422153)

Copy link
Contributor

@eanders-ms eanders-ms Apr 15, 2019

Choose a reason for hiding this comment

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

On a related note, the rule for actor-update may have a bug. Here's how its synchronization behavior is configured right now:

        synchronization: {
            stage: 'create-actors',
            before: 'ignore',
            during: 'queue',
            after: 'allow'
        },

What this does is drop actor-update messages before the create-actors sync stage, which I think is wrong. I think it should be set to allow. The actors being updated won't exist on the client yet, but that's ok. The messages will be queued.

I also think that during should be set to allow, which I why I'm bringing this up now. As configured today it will queue these messages, this includes the actor-update payloads created by previous object-spawned replies. Now that we can queue them in the client, we don't need to wait to send them. It's fine for the client to receive them while the glTF is still loading. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before: allow wouldn't work though, because even though the messages are queued, they'd potentially be queued before the create-from-gltf message. Shouldn't we session queue them?


In reply to: 275546962 [](ancestors = 275546962)

Copy link
Contributor

@eanders-ms eanders-ms left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Contributor

@eanders-ms eanders-ms left a comment

Choose a reason for hiding this comment

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

:shipit:

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.

3 participants