Hubs-Foundation / hubs

Duck-themed multi-user virtual spaces in WebVR. Built with A-Frame.
https://hubsfoundation.org
Mozilla Public License 2.0
2.13k stars 1.42k forks source link

Initialization bug in networked-aframe #3623

Open gfodor opened 3 years ago

gfodor commented 3 years ago

I noticed a rather conspicuous bug in networked-aframe's routines for handling the initial sync message:

https://github.com/MozillaReality/networked-aframe/blob/master/src/NetworkEntities.js#L32

A long time ago the component data was converted from an object with properties like "position" to an array of values (with the schema determining which property corresponds to which entry in the array.) As such NAF drops the initial position/rotation on the floor, which may explain a lot of weird issues like objects not spawning for people until they send a subsequent update. (Honestly I am not sure how objects appear in Hubs on first spawn, probably worth understanding why, in Jel there's no longer an analagous code path.) Also the scale is notably absent from this initialization so if the first and only update to scale is non-identity it will not be applied.

I fixed the code as follows:

   initPosition(entity, componentData) {
-    var hasPosition = componentData.hasOwnProperty('position');
-    if (hasPosition) {
-      var position = componentData.position;
-      entity.setAttribute('position', position);
+    if (componentData[0]) {
+      entity.setAttribute('position', componentData[0]);
     }
   }

   initRotation(entity, componentData) {
-    var hasRotation = componentData.hasOwnProperty('rotation');
-    if (hasRotation) {
-      var rotation = componentData.rotation;
-      entity.setAttribute('rotation', rotation);
+    if (componentData[1]) {
+      entity.setAttribute('rotation', componentData[1]);
+    }
+  }
+
+  initScale(entity, componentData) {
+    if (componentData[2]) {
+      entity.setAttribute('scale', componentData[2]);
     }
   }

It felt presumptuous to open a PR without a Hubs integration test so I'll let you take it from here :)

┆Issue is synchronized with this Jira Task

gfodor commented 3 years ago

Ah this is less severe than it seems I think. the code is definitely wrong, but it seems like its just dead code. properly initializing these happens downstream in the networked component. (in my case I had a separate bug that led me to believe this was the root cause before further investigation.)