Less confusing doc with the new introduced networked-hand-controls #362

vincentfretin · 2022-09-09T10:56:09Z

The networked-hand-controls component introduced in #355 and #358 is auto creating the template, networked schema and networked component on the corresponding entity. This was apparently confusing for some users, who mixed the old approach and the new component.
I already committed directly on master moving the new section up without changes e81604b and fixed wrong param custom hand model param 32651f3
In this PR I added some explanation to better understand the difference between hand-controls with custom templates and the new networked-hand-controls.

But thinking about it, We maybe shouldn't auto create the template, schema, networked component if the user specified it themself, but the way we create it it's not actually possible to check if the user defined it themself. They may want to add additional component to the networked schema, and currently I don't think it's possible.
I'm not sure auto creating all that to simplify the developer experience is actually good, because we actually need to explain in the documentation what's going on behind the scene, otherwise the developer is attaching the networked component and creating the templates themself and not understanding why they have issues.
What's your thoughts @kylebakerio on this?

…ates and the new networked-hand-controls

vincentfretin · 2022-09-09T13:48:58Z

I recently came across the term of principle of least surprise when designing an API. For example here the networked-hand-controls component setting a networked component in init may be unexpected from the developer perspective. Other components like networked-audio-source doesn't have this behavior.

You could also prefer using one rig schema that is synchronizing the rig position/rotation, avatar model y rotation (set from camera rotation via a specific component for local avatar) as a child of the rig, and hands, so not using nested networked entities. That's actually what I did in a previous project. The current behavior of auto creating things prevents you now from doing this, because if you specify to sync hands with the new networked-hand-controls component in your rig schema, you will get the update twice, one from the rig root networked entity and a second time from the auto created nested networked entity because you can't prevent the creation of those extra networked nested entities.
Example of schema you may want to use:

{
  template: '#avatar-template',
  components: [
    "player-info",
    {
      component: "position",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.001)
    },
    {
      component: "rotation",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.5)
    },
    {
      selector: ".model",
      component: "rotation",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.5),
    },
    {
      selector: ".left-controller",
      component: "position",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.001)
    },
    {
      selector: ".left-controller",
      component: "rotation",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.5)
    },
    {
      selector: ".left-controller",
      component: "visible"
    },
    {
      selector: ".left-controller",
      component: "networked-hand-controls"
    },
    {
      selector: ".right-controller",
      component: "position",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.001)
    },
    {
      selector: ".right-controller",
      component: "rotation",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.5)
    },
    {
      selector: ".right-controller",
      component: "visible"
    },
    {
      selector: ".right-controller",
      component: "networked-hand-controls"
    }
  ]
}

with:

<a-entity id="rig" player-info movement-controls spawn-in-circle="radius:3" networked="template:#avatar-template;">
  <a-entity
    id="player"
    camera
    position="0 1.6 0"
    look-controls
    visible="false"
  ></a-entity>
  <a-entity class="model" gltf-model="avatar.glb" y-rotation-from-cam></a-entity>
  <a-entity class="left-controller" networked-hand-controls="hand:left"></a-entity>
  <a-entity class="right-controller" networked-hand-controls="hand:right"></a-entity>
</a-entity>

vincentfretin · 2022-09-09T14:03:41Z

With what I said above, I would prefer reverting the auto creation of the template, schema, networked component and properly document how to specify it in the documentation like I just did in the PR.

kylebakerio · 2022-09-10T23:31:36Z

I am familiar with the principle of least surprise, but of course there's always a balance; the least surprise is attained by doing everything manually from scratch. :)

They may want to add additional component to the networked schema, and currently I don't think it's possible.

That's an interesting thought. I think the real problem is that we should consider overhauling our schema adding API.

Originally I was going to include instructions for doing it manually, but it seemed like bad design to have to just have developers duplicate the same settings in multiple places to make it work. I also wanted this component to match the networked-audio-source as just being a 'set it and it is done' setup.

I'm not sure if there are really so many use cases where one would want to do this manually; the schema you give an example of is a good example, but perhaps we should just build that in to the default, and maybe add a property to allow configuring the update rates?

I also might suggest that auto-add-template is a property. We can even have it be 'false' by default, and show it being set as 'true' in the demo, but make it clear that if they want to customize their template they can remove that and add it themselves?

I'm surprised anyone was confused by this, as the instruction and demo is pretty clear. But by adding it behind a flag like auto-add-template, we allow the convenience while still allowing those who don't immediately understand to do it 'the old way' without causing any issues. We also make it more clear what we're doing to those users, as well.

I would like to see an improved/optimized default template added though, with parameters for precision on the component, perhaps--maybe a single number from 0 to 1 that scales your defaults down?

vincentfretin · 2022-09-12T13:17:36Z

That's an interesting thought. I think the real problem is that we should consider overhauling our schema adding API.

If you have something concrete in mind, you can create a draft PR with some code changes so we can discuss it further.

Originally I was going to include instructions for doing it manually, but it seemed like bad design to have to just have developers duplicate the same settings in multiple places to make it work. I also wanted this component to match the networked-audio-source as just being a 'set it and it is done' setup.

The networked-audio-source and networked-video-source components don't have anything to sync really, so this is simpler. The only thing those components do is getting the owner from the networked component set on the same entity or a parent.
I will do some changes in networked-video-source to allow it to be placed on a shared plane in the scene, that will require it to be synchronized, so the component will need to be added in a networked schema.

I may be wrong, but from our exchange here, it seems you have in mind a more granular way to sync data, more at the component level instead of the entity level with a networked schema? This is unfortunately not how naf currently work.
If you didn't see those hubs streams Hubs New Entity Component System and Dev Stream: Building a networked interactive animated component in Hubs hubs is switching from aframe / networked-aframe to bitECS with a custom netcode that currently mimic naf without interpolation, but the transport will change from json to binary in the future. Their changes allow a more granular way to sync data at the component level and avoiding creating a schema to sync similar things.
It may give you ideas, but I currently didn't thought much how we could modify naf to go in that direction. Something like networked-transform that is synchronizing position, rotation (the 4 values for the quaternion instead of the current euler YXZ), scale. Maybe sync automatically all component having the prefix networked- and not using networked schema at all? We could also work on a binary transport instead of serializing/parsing json. This is probably a huge refactoring of the code and a totally different way of doing things but this is an interesting subject.

I'm not sure if there are really so many use cases where one would want to do this manually; the schema you give an example of is a good example, but perhaps we should just build that in to the default, and maybe add a property to allow configuring the update rates?

The update rates can already be configured with NAF.options.updateRate, default value is 15, meaning 15 times per second, so every 1/15=67ms.

For the default schema syncing position and rotation

networked-aframe/src/Schemas.js

Lines 13 to 16 in fec4c84

    
           components: [ 
        
             'position', 
        
             'rotation', 
        
           ]

could be replaced by a better default

components: [ 
    {
      component: "position",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.001)
    },
    {
      component: "rotation",
      requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.5)
    }
]

I'm not against it, PR welcome and some wording in the README needs to be changed as well.
But you will still need to create some custom networked schema if you have other components you want to sync, scale, player-info, room-state etc.

I also might suggest that auto-add-template is a property. We can even have it be 'false' by default, and show it being set as 'true' in the demo, but make it clear that if they want to customize their template they can remove that and add it themselves?

I'm surprised anyone was confused by this, as the instruction and demo is pretty clear. But by adding it behind a flag like auto-add-template, we allow the convenience while still allowing those who don't immediately understand to do it 'the old way' without causing any issues. We also make it more clear what we're doing to those users, as well.

Yes we can probably do that. I have another idea of changes in the current code that could fix the issue of overriding template with same id and not auto creating the networked component. I'll test that shortly.

I would like to see an improved/optimized default template added though, with parameters for precision on the component, perhaps--maybe a single number from 0 to 1 that scales your defaults down?

This is more or less what vectorRequiresUpdate(precision) is doing. Having a single value 0-1 for both position and rotation doesn't make sense, you need different values.

…t in init

vincentfretin · 2022-09-12T17:38:59Z

With the latest commit, I think I fixed my main concerns, which are:

allow to use a single networked component on the avatar and not using nested networked components for hands, but still allow to use and sync networked-hand-controls (I should create an example with this type)
allow to override template and networked schema for hands when using nested networked
don't hide any template and schema having the name left-hand-template/right-hand-template that may have been already defined in an existing project. That's why I renamed them to left-hand-default-template/right-hand-default-template

I also fixed some issues I had because the this.local logic is now async. I fixed an issue with hand colors, this.data.handColor definitely didn't exist. Can you please double check my changes? Thanks.

kylebakerio · 2022-09-12T22:13:25Z

src/components/networked-hand-controls.js

@@ -201,17 +203,19 @@ AFRAME.registerComponent('networked-hand-controls', {
      this.el.setObject3D(this.str.mesh, newMesh);

      const handMaterial = newMesh.children[1].material;
-      handMaterial.color = new THREE.Color(this.data.handColor);
+      handMaterial.color = new THREE.Color(this.data.color);
+      this.el.sceneEl.systems.renderer.applyColorCorrection(handMaterial.color);


Why duplicate this code instead of calling updateHandMeshColor() ?

We already have the handMaterial variable here and we set the color already, calling updateHandMeshColor() to get again the mesh, setting the color that didn't change just to convert the color with applyColorCorrection seems the real duplication to me.

I defined this.rendererSystem is init for better readability.

kylebakerio · 2022-09-12T22:18:14Z

src/components/networked-hand-controls.js

    }
-    else {
-      this.el.classList.add('naf-remote-hand');


can I ask why we remove this? this makes it easier to view in the a-frame inspector at a glance, if nothing else. I am a fan of things getting classes and IDs where possible to make things more viewable.

I didn't see it used anywhere so I removed it. I almost never use the aframe inspector, so I didn't see the value of it, but you make a good point. I'll put it back.

kylebakerio · 2022-09-12T22:35:21Z

src/components/networked-hand-controls.js

    components: [
      'position',
      'rotation',
      'networked-hand-controls',
    ]
  };
-  NAF.schemas.templateCache[`#${hand}-hand-template`] = templateOuter;
+  NAF.schemas.templateCache[`#${hand}-hand-default-template`] = templateOuter;


I should have done this, but good to fix now: this should probably be

NAF.schemas.templateCache[`#${templateOuter.id}] = templateOuter;

Sure, I introduced

const refTemplateId = `#${templateOuter.id}`;

and used it everywhere.

kylebakerio · 2022-09-12T22:52:37Z

src/components/networked-hand-controls.js


-    if (this.local) {


by removing this condition, we do the binding unnecessarily for networked entities that don't respond to events. not a huge deal, but it's an unnecessary inefficiency. Since you moved/improved the this.local detection, you could move these method bindings to be async / done after line 91.

kylebakerio · 2022-09-13T01:22:57Z

Overall these changes look good and the fixes correct, though I noticed a few small things.

However, for the default template that is added, I was wondering if it made sense to update this:

  NAF.schemas.schemaDict[`#${hand}-hand-default-template`] = {
    template: `#${hand}-hand-default-template`,
    components: [
      'position',
      'rotation',
      'networked-hand-controls',
    ]
  };

to also include values like these:

  {
    component: "position",
    requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.001)
  },
  {
    component: "rotation",
    requiresNetworkUpdate: NAF.utils.vectorRequiresUpdate(0.5)
  },
  ```
by default; and _perhaps_ in this component to have a value that allows tweaking them _all_ with a single factor. So, you'd get something like this, maybe:

```html
<a-entity class="right-controller" networked-hand-controls="hand:right; precision:.5"></a-entity>

where precision = 1 would be equal to .001 (1mm) and precision = .01 or below would be equal to the minimum reasonable precision for position in our opinion, which may be .05 (5cm), and likewise some specified range for rotation, and would derive those from that one 'precision' parameter.

This is what I was trying to communicate before when I wrote

I would like to see an improved/optimized default template added though, with parameters for precision on the component, perhaps--maybe a single number from 0 to 1 that scales your defaults down?

Just a thought--if you feel like including it, great, but these changes are also worthwhile as-is.

kylebakerio · 2022-09-13T02:33:28Z

I may be wrong, but from our exchange here, it seems you have in mind a more granular way to sync data, more at the component level instead of the entity level with a networked schema? This is unfortunately not how naf currently work.
If you didn't see those hubs streams Hubs New Entity Component System and Dev Stream: Building a networked interactive animated component in Hubs hubs is switching from aframe / networked-aframe to bitECS with a custom netcode that currently mimic naf without interpolation, but the transport will change from json to binary in the future. Their changes allow a more granular way to sync data at the component level and avoiding creating a schema to sync similar things.
It may give you ideas, but I currently didn't thought much how we could modify naf to go in that direction. Something like networked-transform that is synchronizing position, rotation (the 4 values for the quaternion instead of the current euler YXZ), scale. Maybe sync automatically all component having the prefix networked- and not using networked schema at all? We could also work on a binary transport instead of serializing/parsing json. This is probably a huge refactoring of the code and a totally different way of doing things but this is an interesting subject.

How interesting, I missed that talk. Lots of interesting things to think about.

vincentfretin · 2022-09-13T08:51:25Z

For position and rotation with requiresNetworkUpdate, definitely make sense, but I'll make that change in another PR with also the same change in the default schema and the corresponding section in the documentation.

vincentfretin added 2 commits September 9, 2022 12:34

explain better the difference between hand-controls with custom templ…

74e3689

…ates and the new networked-hand-controls

use rows instead of columns to define available properties

e03f895

rename default templates and schema, don't use set networked componen…

7f1ddcd

…t in init

kylebakerio reviewed Sep 12, 2022

View reviewed changes

kylebakerio requested changes Sep 13, 2022

View reviewed changes

vincentfretin added 4 commits September 13, 2022 10:22

put back the naf-remote-hand class

7052160

Bind all functions only if local avatar

bf872d5

use a refTemplateId variable

15b7a4d

define this.rendererSystem for better readability

a88029a

vincentfretin requested a review from kylebakerio September 13, 2022 08:51

put back the id on the hands

8d0624d

vincentfretin merged commit 2989fbe into master Sep 13, 2022

vincentfretin deleted the doc-networked-hand-controls branch September 13, 2022 12:20

vincentfretin mentioned this pull request Sep 17, 2022

Better default schema with the requiresNetworkUpdate optimization #367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Less confusing doc with the new introduced networked-hand-controls #362

Less confusing doc with the new introduced networked-hand-controls #362

vincentfretin commented Sep 9, 2022

vincentfretin commented Sep 9, 2022

vincentfretin commented Sep 9, 2022

kylebakerio commented Sep 10, 2022

vincentfretin commented Sep 12, 2022 •

edited

vincentfretin commented Sep 12, 2022

kylebakerio Sep 12, 2022

vincentfretin Sep 13, 2022

vincentfretin Sep 13, 2022

kylebakerio Sep 12, 2022

vincentfretin Sep 13, 2022

vincentfretin Sep 13, 2022

kylebakerio Sep 12, 2022

vincentfretin Sep 13, 2022

kylebakerio Sep 12, 2022

vincentfretin Sep 13, 2022

kylebakerio commented Sep 13, 2022 •

edited

kylebakerio commented Sep 13, 2022

vincentfretin commented Sep 13, 2022

Less confusing doc with the new introduced networked-hand-controls #362

Less confusing doc with the new introduced networked-hand-controls #362

Conversation

vincentfretin commented Sep 9, 2022

vincentfretin commented Sep 9, 2022

vincentfretin commented Sep 9, 2022

kylebakerio commented Sep 10, 2022

vincentfretin commented Sep 12, 2022 • edited

vincentfretin commented Sep 12, 2022

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

kylebakerio commented Sep 13, 2022 • edited

kylebakerio commented Sep 13, 2022

vincentfretin commented Sep 13, 2022

vincentfretin commented Sep 12, 2022 •

edited

kylebakerio commented Sep 13, 2022 •

edited