Tracked Controllers Example Overhaul

[kylebakerio]

• Updated environment to be more appealing, and suggest that we update all updated examples to this environment both for visual appeal, but also to make it clear that these new examples are separate from the old examples.
• Removed particles.
• Made tracked hands actual hand models.
• Added basic illustration of how to add hands dynamically.
• Added comments.
• Removed unnecessary schema for hands because position and rotation are tracked by default... could have removed avatar as well, but decided to not touch that, as it's nice to show that you can add multiple separate schemas this way. I think this example should perhaps also be updated to include the fix for the fast-schema-bug you recently found how to work around, though?
• Added hand model assets.
• Removed lighting,sky,grid,ground--all now handled by environment component, making the example less distracting.
• Changed avatar template code style to be more visually clear (no need to add emphasis to the avatar geometry with newlines).
• Changed avatar proportions so that hands are not swallowed by them, and changed proportions slightly to make them somewhat less dorky looking--I also suggest this modification across all examples to create a new coherent visual style for current examples, although I'm open to better options.
• Added joystick movement controls and flying ability via aframe-extras.
• Added comments mentioning room for future development.

I'd like to go through the examples and update all avatars and environments and examples to match the various changes I've made here, if you're comfortable with that.

Also, would be good to share editing rights with me on glitch so I can make the corresponding updates there to the canonical examples.

You can try this out here:
https://glitch.com/edit/#!/aug2021-naf?path=examples%2Ftracked-controllers.html%3A42%3A0

[vincentfretin]

Thanks for starting to modernize the tracked controllers example. The hands tracking definitely didn't exist when this example was created, this has since been implemented in WebXR and included in latest aframe, so it will be a nice addition. There is also a demand for this kind of example #233.

First I'll share my thoughts on the examples and the maintenance of them, then I'll comment of some specific points of your list.
Note that I didn't test your example, I don't have currently the Quest with me.

As a maintenance point of view of this repository, I prefer we depend less on external libraries in the basic examples. I asked about removing particles dependency in the examples in #273, in the end I didn't want to change drastically the examples, so I just updated the url to a fork. I'm open to remove it completely and keep the examples simpler. Similar to that, in the end I also removed the a-saturday-night example (#254) because we couldn't update it to latest aframe. So we need to be careful of external dependencies we use in the examples.
Although the aframe environment is great and may be more appealing, I think using the aframe primitives a-sky, the plane, lights and declaring the assets may be better for educational purpose, and the entities can be easily modified. Also I may be nostalgic of the tron like environment if we remove it. :)
I'm not against a more complex and appealing example though, we can have an example with hands like your PR combined with physics. We can even combine what I started in #270 with yours and use the aframe environment component for this one.

You're right about the schema workaround, we need to put it in all examples now.
For glitch, I never had access to the old examples. I only have access to the naf-project I created, and I already did the schema workaround on this one.

I think the tracked-controllers example is the only one where we use nested networked entities, although [position, rotation] is the default schema, I think it's ok to have it explicitly. Having them already defined is nice if you decide to modify the example and add an additional component you want to sync to your left arm. :)
I see the examples as more educational examples where the developer can build upon.
I often compared two basic examples to see what lines needs to be changed to support a given use case, so I definitely prefer that the examples stay basic and that we have the same html indentation between them. In my opinion having all attributes of an html tag on the same line make it more difficult to compare. Also my personal preference is to have an attribute per line, it's way better with git diff :D

So in summary I'd prefer we update the tracked controllers with the new hands model, but without the aframe-extras and environment if possible, and with the same indentation that we have in the other examples.
Then we can add a new full example with environment, aframe-extras with animations with animation-mixer, even physics.
What do you think?

[kylebakerio]

I'd like to expand this to a component that could transmit specific controller state, like is requested in #233, and plan to implement that at some point for my own app as well. At some point, we'll want to actually include hand-tracking, as in a full networked hand skeleton, but both of those are milestones that follow after this one, I think.

Dependies: That makes sense. I generally agree. I think that movement-control is pretty standard and a far more valuable (allows experiencing networked movement a bit more) and reliable dependency than a particle component, though. I'd suggest we keep it and add it to all examples, but I'm open to keeping it out.

I do think the particles look a bit cheesy, though, and would prefer we didn't keep those. They also result in extra code to remove them on mobile.

Although the aframe environment is great and may be more appealing, I think using the aframe primitives a-sky, the plane, lights and declaring the assets may be better for educational purpose, and the entities can be easily modified. Also I may be nostalgic of the tron like environment if we remove it. :)

My thoughts are that they are distracting to newcomers. I think it's a significant amount of extra code that has nothing to do with NAF. I'd rather we educate users on NAF-specific things (e.g. movement-controls inclusion) than general A-Frame things don't affect NAF. To that point, the environment component makes the code for the environment be completely undistracting, while also more appealing. Also, those files referenced are files we don't control and don't maintain, and imgur could pull those at any time, for example. (I also sometimes noticed graphical artifacts from the old style design, iirc?)

I think the old environment looks quite out of date, personally. It reminds me of the old A-Frame 0.3 examples. It's also too dark. But if you prefer Tron, there's a Tron preset in the environment component as well, which I can switch to. I guess that would have more continuity with some kind of 'tradition'? :)

You're right about the schema workaround, we need to put it in all examples now.

Ideally we fix the issue so that we have a better, consistent API that doesn't look like a hack, long term. We should probably fix that before we go inject the workaround to all the examples, only to have to update them all again afterwards.

For glitch, I never had access to the old examples. I only have access to the naf-project I created, and I already did the schema workaround on this one.

Sure, I just mean the glitch referred to in the readme now. I assume we're just using the one you created as the official one now.

I often compared two basic examples to see what lines needs to be changed to support a given use case, so I definitely prefer that the examples stay basic and that we have the same html indentation between them. In my opinion having all attributes of an html tag on the same line make it more difficult to compare. Also my personal preference is to have an attribute per line, it's way better with git diff :D

In general I understand and agree, however, I think in this case, that huge amount of indentation and so many lines makes it feel visually overwhelming and draws a lot of focus to parts of the code that don't matter. It's much easier to see the structure of what is going on with this style than with a bunch of custom geometry and material attributes laid out to craft a 3d model for an avatar in code. In 'real' projects, I definitely separate out the attributes per-line as well, I just think that pushing those details aside makes for better NAF educational content. I feel really strongly about this; the way the original examples show nested components with all those lines of attributes makes it really hard to absorb what is happening, in my opinion. I'd say if you insist on not changing to this style, then we should consider just making the avatars far simpler--the way they are now is a real problem, I think.

I think the tracked-controllers example is the only one where we use nested networked entities, although [position, rotation] is the default schema, I think it's ok to have it explicitly. Having them already defined is nice if you decide to modify the example and add an additional component you want to sync to your left arm. :)

I was a bit undecided on this. On the one hand, I don't want to make users think those need to be explicitly decalred when they aren't needed. On the other hand, I agree, it is nice to see how it works. I think I like the idea of leaving it there commented out, with it explained that we could declare it but it would be redundant?

Then we can add a new full example with environment, aframe-extras with animations with animation-mixer, even physics.

I think it makes sense to add animations /animation-mixer to this one, eventually, by creating a 'networked-hand-controls' component that syncs hand animation across devices, which I'm happy to do. When we do the physics example, I think we should probably keep the animations out of it (unless they're abstracted away to a networked-hand-controls component), and focus on the minimum to get networked physics working--probably just something like having the ability to knock towers of cubes over.

Then, separately, we can have a more advanced physics example that incorporates super-hands, ability to grab things, etc.?

[kylebakerio]

I've updated the environment to a tron-similar preset that I hope is a decent compromise, added comments showing how the other networked components can be optionally declared but also showing that because of the defaults they aren't needed, and added many other educational code comments to the existing code, and also cleaned up the component a bit.

[kylebakerio]

I have to say, the way the sphere is declared with visible:false both in the schema but also in the scene itself is pretty confusing for a basic example. It is interesting to show the specific case of modifying attributes that aren't visible on your own local copy of a template, but it seems like it would be more intuitive to have that #head sphere added by the template and then be modified in JS after it is instantiated.

It's just kind of a confusing bit of code to use for an educational demo. I've never used this kind of functionality. I kind of feel like this old-school example should only exist in a single demo showing this concept, and the other demos should just have simple gltf avatars (which are more applicable to common use case) and text usernames tracked under the avatars instead of relying on random-color.

Thoughts?

[kylebakerio]

I also feel like having onConnect be called if the global is present is just a terrible API and that should definitely be moved to an event or an exposed promise on the NAF global... I cringe every time I see that.

[kylebakerio]

https://8-2021-naf.glitch.me/tracked-controllers.html

[vincentfretin]

I like the tron environment from aframe-environment. Ok, let's use that. :) You can change it in all examples in a separate PR so I can merge just this change for now (remove the snow particles, remove the imgur images, plane, lights).

For the indentation part of the template, for this particular case, I changed my mind, I'm ok with putting all components on the same line, but please don't put two spaces between attributes and before the end of the tag. You can do a separate PR just for this too if you want or in the same as the one above so I can quickly review and merge the changes.

I have to say, the way the sphere is declared with visible:false both in the schema but also in the scene itself is pretty confusing for a basic example. It is interesting to show the specific case of modifying attributes that aren't visible on your own local copy of a template, but it seems like it would be more intuitive to have that #head sphere added by the template and then be modified in JS after it is instantiated.

This can indeed be difficult to grasp why we do this. The visible:false sphere is just there so that the random-color component that set material.color is synchronized to the other participants. We don't need to create eyes or eventually other parts of the avatar, that's why we use attachTemplateToLocal:false and define only the head sphere. If you set attachTemplateToLocal:true with visible:false does it works correctly? Maybe this would be simpler to understand?
If yes, you can do a PR just to fix that in the examples. I like small PR, easier to review and merge if no further discussion is needed. ;)

It's just kind of a confusing bit of code to use for an educational demo. I've never used this kind of functionality. I kind of feel like this old-school example should only exist in a single demo showing this concept, and the other demos should just have simple gltf avatars (which are more applicable to common use case) and text usernames tracked under the avatars instead of relying on random-color.

Putting the old example https://glitch.com/edit/#!/naf-nametags in the repo and updating it would be great.

I also feel like having onConnect be called if the global is present is just a terrible API and that should definitely be moved to an event or an exposed promise on the NAF global... I cringe every time I see that.

Ah ah I also find it weird that a global function named onConnect would be called like that out of the blue. :)
We have this API like forever so we can't really change that without breaking code used in the wild.
This is done here

networked-aframe/src/components/networked-scene.js

Lines 35 to 37 in f1c62b4

	if (this.hasOnConnectFunction()) {
	this.callOnConnect();
	}

networked-aframe/src/components/networked-scene.js

Lines 56 to 58 in f1c62b4

	callOnConnect: function() {
	NAF.connection.onConnect(window[this.data.onConnect]);
	},

The global onConnect is the callback function called on the "connected" event you have on document.body, see

networked-aframe/src/NetworkConnection.js

Lines 60 to 68 in f1c62b4

	onConnect(callback) {
	this.onConnectCallback = callback;
	if (this.isConnected()) {
	callback();
	} else {
	document.body.addEventListener('connected', callback, false);
	}
	}

So you could register your own listener instead of relying on the global onConnect, it's just not documented. And you need to be sure you call your function if you are already connected like it's done above.

[kylebakerio]

Great, thanks for the responses!

I like the tron environment from aframe-environment. Ok, let's use that. :) You can change it in all examples in a separate PR so I can merge just this change for now (remove the snow particles, remove the imgur images, plane, lights).

For the indentation part of the template, for this particular case, I changed my mind, I'm ok with putting all components on the same line, but please don't put two spaces between attributes and before the end of the tag. You can do a separate PR just for this too if you want or in the same as the one above so I can quickly review and merge the changes.

So, to be clear: my idea with this pull request is to establish a 'new standard' for what our demos should look like. Once we've hammered it all out for this one, I'd like to do another pull request to apply the updates here to all the other demos.

This can indeed be difficult to grasp why we do this. The visible:false sphere is just there so that the random-color component that set material.color is synchronized to the other participants. We don't need to create eyes or eventually other parts of the avatar, that's why we use attachTemplateToLocal:false and define only the head sphere. If you set attachTemplateToLocal:true with visible:false does it works correctly? Maybe this would be simpler to understand?
If yes, you can do a PR just to fix that in the examples. I like small PR, easier to review and merge if no further discussion is needed. ;)

I understand--after having used this library for some time, now, and having stared at that code for some time--what is going on, but I think NAF is just really weird here. The idea that you let NAF manage creating and syncing your avatar, and tell it to instantiate its invisible copy... but then the user, separately, outside of NAF, makes a child component that is supposed to just happen to match the template, which NAF just happens to somehow know to listen for (probably because of the class and hierarchy), and changes to that user-created child of the NAF-created (but not attached to local, and invisible) template instance just happen to correctly be propagated?

Personally, I think it's poor design. Even if this does work, this shouldn't be how it's taught to be done. It would make far more sense to write a component that only sets color if it's the user's local avatar but not a networked avatar--similar to to logic employed in networked-audio-source.

Whatever we do in regards to the API going forward for handling this, I think in our demos it would make more sense to update color-randomizer to handle this logic internally and to just be applied to the template directly (and to have logic inside that lets it determine whether it should update the color because it is local, or not update the color because it isn't local/the owner), than to demonstrate doing this kind of thing in HTML, which should imo be discouraged, and is just awkward and unclear and hard to follow and reason about.

Putting the old example https://glitch.com/edit/#!/naf-nametags in the repo and updating it would be great.

Agreed, and I'm glad to do this. I'd perhaps like to go as far as updating all of our demos to use nametags, possibly instead of colors, though, instead of having one example for nametags. I personally think nametags are a better choice than colors, because it's obvious that the nametag value comes locally from the user themselves, and that other nametags from other users come from networked (and not local) sources (which as far as I can tell, is supposed to be the point of including the color-randomizing concept in the demos).

Ah ah I also find it weird that a global function named onConnect would be called like that out of the blue. :)
We have this API like forever so we can't really change that without breaking code used in the wild.
This is done here

Ah! Well, if that functionality already exists, can we switch to presenting the listener-based functionality instead in our demos, and updating our documentation to encourage that pattern? I've started a new issue on this topic as well, #293.

[vincentfretin]

I understand--after having used this library for some time, now, and having stared at that code for some time--what is going on, but I think NAF is just really weird here. The idea that you let NAF manage creating and syncing your avatar, and tell it to instantiate its invisible copy... but then the user, separately, outside of NAF, makes a child component that is supposed to just happen to match the template, which NAF just happens to somehow know to listen for (probably because of the class and hierarchy), and changes to that user-created child of the NAF-created (but not attached to local, and invisible) template instance just happen to correctly be propagated?

From your long paragraph, I don't know if you understood it or not. :)
Which components and data of those components that will be synchronized is defined by the networked schema, not the template. You can have some entities with specific components in the template that are not synchronized, if there are not defined in the networked schema.

Personally, I think it's poor design. Even if this does work, this shouldn't be how it's taught to be done. It would make far more sense to write a component that only sets color if it's the user's local avatar but not a networked avatar--similar to to logic employed in networked-audio-source.

How do you check if this is the local avatar or a remote avatar? In one of my app, I'm doing sometimes the logic in the component instead of the html and I'm using `if (this.el.id=== 'cameraRig') to check if it's the local avatar, this does the job, but this is not something we should encourage on a component in a library, this breaks reusability of the component.

Whatever we do in regards to the API going forward for handling this, I think in our demos it would make more sense to update color-randomizer to handle this logic internally and to just be applied to the template directly (and to have logic inside that lets it determine whether it should update the color because it is local, or not update the color because it isn't local/the owner), than to demonstrate doing this kind of thing in HTML, which should imo be discouraged, and is just awkward and unclear and hard to follow and reason about.

See my previous comment.

Putting the old example https://glitch.com/edit/#!/naf-nametags in the repo and updating it would be great.

Agreed, and I'm glad to do this. I'd perhaps like to go as far as updating all of our demos to use nametags, possibly instead of colors, though, instead of having one example for nametags. I personally think nametags are a better choice than colors, because it's obvious that the nametag value comes locally from the user themselves, and that other nametags from other users come from networked (and not local) sources (which as far as I can tell, is supposed to be the point of including the color-randomizing concept in the demos).

I'll keep the colors for the simple reason it's easy to test multi users with several tabs in the same browser.
If you have only nametag, probably store the nametag in localStorage to not reenter it each time, you will end up with the same nametag for the avatars on all your tabs, unless you modify the nametag in a form that is accessible while in the room.

[kylebakerio]

From your long paragraph, I don't know if you understood it or not. :)
Which components and data of those components that will be synchronized is defined by the networked schema, not the template. You can have some entities with specific components in the template that are not synchronized, if there are not defined in the networked schema.

This exactly matches my understanding. You seem to be missing my point. (I'm also not sure if it's because you're not a native english speaker, but your use of a smiley here feels pretty condescending...?)

There are three declarations of the colored body sphere in our example:

1. The schema
2. The template
3. The HTML declaration in the body

Currently, for the color sphere:

1. You declare in schema what should be synced
2. You declare in template a consistent pattern that can be generated by all clients by NAF
3. You declare in HTML that for your own local instance, don't actually generate a copy, but do generate a copy on all other users' environments.

This creates the problem for the user: how do I set/update the syned color of my sphere, since I didn't attach the template to local? (The first time learner reading the code has to understand that this problem even exists, of course, before they can understand that we're showing the solution to that problem). The solution we create for this problem is:

4. You also declare in HTML a duplicate of a part of what would have been generated according to the template, and add a component that modifies that value. (It effectively only modifies the value because it's run in the body and not in the template, and so every user runs that once for their own avatar that is declared in their body. We don't explain this to the user, we just expect them to pick this up from the example.)

The solution we give in our demo is... declare an invisible duplicate of what is delcared in the template, in the HTML itself, and modify the synced property there. As long as you give it the right class and put it in the matching order of where the template would have put it, it'll magically be synced as expected.

IMO, this is really poor dev UI. I imagine most users who look at this source struggle to understand what is going on here. I know I did when I first looked at it. From this example, it isn't clear that you generaly don't need to declare a duplicate of what is in the template within the HTML--just a reference to it is far more typical (I've never used this pattern from the demos in my code). It also takes some thought to figure out how/why that sphere is kept in sync--one would expect that only the entities generated by NAF according to the template are kept in sync, not that some other matching entity the user creates happens to be correctly picked up and synced as well. This behavior isn't explained anywhere, either--it's just given as a code example, and is in the only example we have.

Personally, I find it unnecessary to even make an API to handle this through the HTML directly, and would prefer we show how to do this in JS. I don't recall/know off the top of my head if NAF already creates the given elements even when attachTemplateToLocal is false and just makes them invisible, or if it doesn't add them at all, but NAF could either:

1. add them and just add visible=false, not sync the visible property, and thus allow the user to modify them as needed (i.e., automatically do what the user is doing here behind the scenes).
2. expose its own API for updating synced elements/components via JS that performs this element creation behind the scenes if needed (so, same as 1, but on demand--adds more complexity, but allows higher efficiency by only creating entities if needed)
3. more efficient version of 2, where it doesn't actually do the creation, but just propogates the necessary changes. this is more code complexity, though, as we now have to keep track of syncing these virtual entities that are not attached to local. that said, this isn't crazy and should be pretty reasonable.

Here's the thing, though: while I think it's useful to include a single demo that shows how to manage this scenario (syncing elements/components that aren't there because of attachTemplateToLocal: false), I don't think we should show this pattern in every one of our demos. We should sync something that is visible for all users, including for the user themself. This is yet another reason why a nametag could be a good example--no need to add attachTemplateToLocal: false, which just makes the demo more complex and confusing, instead of showing the basic case.

Another idea: if we add the usernames, we could also add an in-world mirror.

How do you detect who the owner is?

networked-audio-source.js does this:

    NAF.utils.getNetworkedEntity(this.el).then((networkedEl) => {
      const ownerId = networkedEl.components.networked.data.owner;

      if (ownerId) {
        // not owned by user
      } else {
        // owned by user
      }
    });

When I wrote the networked-video-source component, I had dug into this code deeply and knew the behavior off the top of my head, but I haven't touched NAF specific code in several months now. But just quickly glancing here, it seems that NAF doesn't add networked.data.owner to local components of networked elements. (Off the top of my head I would assume you match the owner to your own NAF ID, but it seems this is how the library currently does it.)

I just also checked how I do this in a component I wrote--I leverage a 'userdict' that all users create for themselves and sync with each other when they join a room. The assign-avatar component I wrote looks to the userdict to see which gltf is should use, and waits for that userdict to be updated if it isn't present yet. No logic is necessary to determine if local user is owner or not--same logic applies to local user as every other user.

I'll keep the colors for the simple reason it's easy to test multi users with several tabs in the same browser.
If you have only nametag, probably store the nametag in localStorage to not reenter it each time, you will end up with the same nametag for the avatars on all your tabs, unless you modify the nametag in a form that is accessible while in the room.

You don't need to store nametag in localstorage for the demos, and you can generate default random nametags that the user can override if desired. You can show a prompt that requests a username (and is prefilled with a default) prior to network connection. In fact, making it even more clear, we could add an overlay saying what user's perspective you have--currently, you can't tell what color you are unless you look at yourself from another user's screen. Nametags would be more clear than this, especially with a simple overlay showing one's own nametag value.

[vincentfretin]

This exactly matches my understanding. You seem to be missing my point. (I'm also not sure if it's because you're not a native english speaker, but your use of a smiley here feels pretty condescending...?)

Not at all my intend. I usually put smiley to indicate I mean no harm in the thing I just said, because sometimes I think my reply could be interpreted the wrong way by the other person. I had no idea it could be interpreted even worse with a smiley.
I genuinely didn't know if you understood or not how the things was working, maybe because of the user story way you phrased it and ending your sentence with a question mark.
From your long reply, it makes no doubt now you understand the issue well. And I'm looking forward to have a lively conversation with you on the subject. I'll reply to other parts later.

[vincentfretin]

I agree with you with the poor developer experience on this subject. I also struggled to understand it by just reading the example.

I don't recall/know off the top of my head if NAF already creates the given elements even when attachTemplateToLocal is false and just makes them invisible, or if it doesn't add them at all

It doesn't create the entities when attachTemplateToLocal is false.

but NAF could either:

1. add them and just add visible=false, not sync the visible property, and thus allow the user to modify them as needed (i.e., automatically do what the user is doing here behind the scenes).
2. expose its own API for updating synced elements/components via JS that performs this element creation behind the scenes if needed (so, same as 1, but on demand--adds more complexity, but allows higher efficiency by only creating entities if needed)
3. more efficient version of 2, where it doesn't actually do the creation, but just propogates the necessary changes. this is more code complexity, though, as we now have to keep track of syncing these virtual entities that are not attached to local. that said, this isn't crazy and should be pretty reasonable.

For 1, As a developer I don't expect those invisible entities to be created if attachTemplateToLocal is false. Depending on your template, this could have side-effects like downloading some resources like textures you didn't expect, and those may never be used for rendering. This really depends on your app use case though.
For 2, I understand it, but I'm not sure how this API could look like. Do you have an API design in mind?
For 3, if I understand this correctly, for example creating the sphere for the head with material component (because we sync .head material.color) but not the two spheres of the eyes based on what is synchronized in the networked schema? This seems a good idea, without the drawbacks of 1. I'm not sure how complex this implementation would be.

You don't need to store nametag in localstorage for the demos, and you can generate default random nametags that the user can override if desired. You can show a prompt that requests a username (and is prefilled with a default) prior to network connection. In fact, making it even more clear, we could add an overlay saying what user's perspective you have--currently, you can't tell what color you are unless you look at yourself from another user's screen. Nametags would be more clear than this, especially with a simple overlay showing one's own nametag value.

I talked about localStorage because I have the connectOnLoad:true in mind and also as a user I don't want to reenter my name every time, but yes for demos we can simply generate random nametags and allow to change it when in the room via an overlay.
If you change to connectOnLoad:false, you can prompt for a nametag and color before joining the room.

[kylebakerio]

Not at all my intend. I usually put smiley to indicate I mean no harm in the thing I just said, because sometimes I think my reply could be interpreted the wrong way by the other person. I had no idea it could be interpreted even worse with a smiley. I genuinely didn't know if you understood or not how the things was working, maybe because of the user story way you phrased it and ending your sentence with a question mark. From your long reply, it makes no doubt now you understand the issue well. And I'm looking forward to have a lively conversation with you on the subject. I'll reply to other parts later.

Thanks for the clarification. I actually went and looked it up after this, and it's actually a common misunderstanding: https://www.bustle.com/life/what-does-the-smiley-face-emoji-happy-or-passive-aggressive

(That article makes it sound like it's mostly very young people, but I'm nearly 32, and my girlfriend--who is 28, and not a native english speaker--also felt the same way.)

For me, saying something like "I don't know if you understand it" and then putting a smiley face feels passive aggressive, but I understand that's not how you meant it. Glad I asked and we cleared that up.

Also, I see why you might be confused with my ? at the end. That question mark was supposed to convey a feeling of 'seriously? This is what the user is supposed to do?', not to express that I was unsure about it. It's a feeling of finding it absurd.

Similar usage might be someone in a movie saying, "And now, after all I've done for you, you just want me to go away?"--not because they aren't sure, but because they find it hard to believe.

[kylebakerio]

It doesn't create the entities when attachTemplateToLocal is false.

I thought so, thanks for clarifying.

For 1, As a developer I don't expect those invisible entities to be created if attachTemplateToLocal is false. Depending on your template, this could have side-effects like downloading some resources like textures you didn't expect, and those may never be used for rendering. This really depends on your app use case though.

Yeah, this could incur some level of performance cost in theory in a few ways, but I think the effect is pretty minor. As long as visible=false, I don't think it will incur rendering burden, which is the most important thing, I'd think. It does create the best developer experience, though, imo. This is similar to what you suggested a few comments back, when you said:

If you set attachTemplateToLocal:true with visible:false does it works correctly? Maybe this would be simpler to understand?¹

(The difference is that the dev shouldn't have to manually add a partial copy of the template, though)

Nothing special is needed. We could rename attachTemplateToLocal to something else to be more clear--we could also just add this as an alternate mode and leave both, depending on what is needed by the dev and to prevent breaking existing code. attachTemplateToLocal=false for when the dev wants to make sure the template is completely not present / incurs no burden, and something like hideLocalTemplate=true as an alternative that just uses visible=false so they can propagate networked changes using the standard A-Frame component apis while still hiding it locally.

For 2, I understand it, but I'm not sure how this API could look like. Do you have an API design in mind?

This is just a quick draft off the top of my head; I really appreciate how NAF in most cases works so that nothing like this is necessary, but I also really dislike how it can need something like shown in the demos to share changes when attachTemplateToLocal is false, and I think this is a reasonable stab at a compromise.

Here's two idea for how this could look, along with an example scenario to make it clear:

<a-scene>
    <a-assets>
      <template id="net-sphere">
        <a-entity class="outer-sphere" geometry="primitive: sphere" material="color: red">
           <a-entity class="inner-box" geometry="primitive: box" material="color: yellow"></a-entity>
        </a-entity>
      </template>
    </a-assets>
    <a-entity id="my-net-sphere" networked="template:#net-sphere; attachTempalteToLocal:false;"></a-entity>
</a-scene>

<script>
    // (1) we add a special method to 'networked' component called `setAttribute` that works similarly to the A-Frame `setAttribute` 
    // that would be used this way (though should also accept
    document.querySelector('#my-net-sphere').components['networked'].setAttribute('.outer-sphere','material', 'color', 'red')
    // (1b) should also allow this syntax, to match what is enabled by A-Frame's `el.setAttribute`:
    document.querySelector('#my-net-sphere').components['networked'].setAttribute('.outer-sphere', { material: { color: 'orange' } } })
    
    // (2) declare a custom schema handler for a special property, e.g. 'naf-update' here, to create a way of using the built-in 
    // `setAttribute` for the `networked` component 
    // note: this would require this object-style syntax, because we're using the built-in `el.setAttribute` method
    document.querySelector('#my-net-sphere').setAttribute('networked', 'naf-update', { '.inner-box' { material: { color: 'blue' } } })
    
    // (3a) special global function we add for modifying things that don't exist locally
    NAF.setAttribute('#my-net-sphere', '.inner-box', 'material', 'color', 'green')
    // (3b) again, the global should also allow this syntax
    NAF.setAttribute('#my-net-sphere', { '.outer-sphere': { material: { color: 'orange' } } })
</script>

The idea is that when you use this, if you are using it on something with attachTemplateToLocal=true it does nothing special, but if it's with attachTemplateToLocal=false and the piece of the template is missing locally, then it would either:

• silently add in the necessary element, and then continue in the future by updating invisible, newly created element, OR:
• as per (3), just propoagate the change to peers without actually locally adding the missing template element/component

I talked about localStorage because I have the connectOnLoad:true in mind and also as a user I don't want to reenter my name every time, but yes for demos we can simply generate random nametags and allow to change it when in the room via an overlay.
If you change to connectOnLoad:false, you can prompt for a nametag and color before joining the room.

yeah, exactly. I also generally just don't add networked-scene until I'm ready and make a wrapper component to handle that logic, but same idea.

[kylebakerio]

So, this conversation has derailed a bit--I think this is really important to discuss, and I'll just create a new issue for this if that's alright (edit: done: here)--but for the sake of this pull request, I'd like to come back to what we should do in the short term for updating all of the demos. Trying to organize it, I think my proposal is:

1. The way we update the head color in the templates is a somewhat special edge case that should

• (a) probably have a new api designed, to be discussed in a separate issue here
• (b) should either be shown in JS instead of in HTML, and/or have clarifying comments included in the HTML, and
• (c) not be included in every demo, as it adds more confusion than clarity and distracts from the 'normal' case, and instead just be shown in a single demo illustrating this particular scenario.

2. As an alternative, we can have nametags implemented

• (a) user is prompted before connection for name
• (b) user is shown their own username in an overlay for additional clarity

3. Update use of onConnect:

• (a) possibly/probably have new api designed, as being discussed in other issue here
• (b) for now, update examples to use document.body.addEventListener('connected', callback, false);
• (c) update to readme.md to push users to this api

I think those are the only things remaining for us to discuss here right now--I can implement those, and assuming it looks good, wrap up this pull request, and then I can go update the other examples to match this one in another pull request.

Sound good?

[vincentfretin]

Thanks for the link about the smiley happy or passive-aggressive, this is interesting. This is indeed a common misunderstanding it seems. I'll think twice before adding a smiley now. I'm using https://www.dictionary.com/e/emoji/grinning-face-with-sweat-emoji/ from time to time in chat, wow this can have a lot of meanings. And https://www.dictionary.com/e/emoji/grinning-face-with-smiling-eyes-emoji/ was really bad when the other person received a completely different smiley design.

For the point 2, I agree. For points 1 and 3, follow-up discussion on the associated issues.

[vincentfretin]

How do you detect who the owner is?

networked-audio-source.js does this:

    NAF.utils.getNetworkedEntity(this.el).then((networkedEl) => {
      const ownerId = networkedEl.components.networked.data.owner;

      if (ownerId) {
        // not owned by user
      } else {
        // owned by user
      }
    });

When I wrote the networked-video-source component, I had dug into this code deeply and knew the behavior off the top of my head, but I haven't touched NAF specific code in several months now. But just quickly glancing here, it seems that NAF doesn't add networked.data.owner to local components of networked elements. (Off the top of my head I would assume you match the owner to your own NAF ID, but it seems this is how the library currently does it.)

I found it weird that owner is empty here for your own local components, so I dug a little bit.
Your code above was inspired by

networked-aframe/src/components/networked-audio-source.js

Lines 22 to 33 in 51d08ba

	NAF.utils.getNetworkedEntity(this.el).then((networkedEl) => {
	const ownerId = networkedEl.components.networked.data.owner;
	if (ownerId) {
	NAF.connection.adapter.getMediaStream(ownerId)
	.then(this._setMediaStream)
	.catch((e) => naf.log.error(`Error getting media stream for ${ownerId}`, e));
	} else {
	// Correctly configured local entity, perhaps do something here for enabling debug audio loopback
	}
	});
	},

where it checks if owner is empty. For this particular component, it's used generally on the avatar-template and is created locally only if networked="template:#avatar-template;attachTemplateToLocal:true;"
networked.data.owner will indeed be empty initially before we are connected, but it is not empty anymore once you're connected. Once connected, the owner is set here

networked-aframe/src/components/networked.js

Lines 267 to 270 in ac93c62

	onConnected: function() {
	if (this.data.owner === '') {
	this.lastOwnerTime = NAF.connection.getServerTime();
	this.el.setAttribute(this.name, { owner: NAF.clientId, creator: NAF.clientId });

So this hack works only if you use the condition in init, it won't work in update, so be careful.
If you want a condition in update to check if it's the owner, you may want to check !networked.data.owner || networked.data.owner === NAF.clientId.

[kylebakerio]

So this hack works only if you use the condition in init, it won't work in update, so be careful.
If you want a condition in update to check if it's the owner, you may want to check !networked.data.owner || networked.data.owner === NAF.clientId.

Good catch! That's more like what I expected. That makes sense, and yes, that was me quickly checking how networked-audio-source.js does it because I just needed a quick reference for how it can be done, and that seemed like it should be a good example since it was in the repo. I was really surprised, and networked.data.owner === NAF.clientId is exactly what I expected it to be.

Thanks for the link about the smiley happy or passive-aggressive, this is interesting. This is indeed a common misunderstanding it seems. I'll think twice before adding a smiley now. I'm using https://www.dictionary.com/e/emoji/grinning-face-with-sweat-emoji/ from time to time in chat, wow this can have a lot of meanings. And https://www.dictionary.com/e/emoji/grinning-face-with-smiling-eyes-emoji/ was really bad when the other person received a completely different smiley design.

That's funny. The 'smiling with sweat emoji' has always been associated to me with its appearance in anime, which is mentioned further down on the link you gave when it comments here:

You will also see this emoji accompanying posts about anime because, according to Winnie Liu of the Factorialist, “the giant sweat drop above the brow has traditionally appeared in Japanese animation…to indicate awkwardness, frustration, embarrassment, and sentiments like What the heck?”

I just always interpret it this way, not when it has anything to do with anime. I also haven't watched that much anime in my life--it's just the only place I had seen gigantic sweat drops on a smiling face, so that's just obviously what I interpreted it as. Even as a kid watching pokemon, for example, I was seeing images like this:

https://www.dictionary.com/e/emoji/grinning-face-with-smiling-eyes-emoji/

This one was interesting, though I've never really used that one.

For the point 2, I agree. For points 1 and 3, follow-up discussion on the associated issues.

will follow up and return here afterwards.

[kylebakerio]

update: so, dug into our 'determine if an el is owner by local user or not` question, and it seems it can work on update. With a little more experimentation, I've come up with these functions which seem to be working:

      NAF.utils.ownedByLocalUser = function ownedByLocalUser(el) {
        return new Promise(async (resolve, reject) => {
          const ownerId = await NAF.utils.safelyGetOwner(el)
          resolve(ownerId === NAF.clientId)
        })
      }

      NAF.utils.safelyGetOwner = function safelyGetOwner(el) {
        return new Promise(async (resolve, reject) => {
          const netEl = await NAF.utils.getNetworkedEntity(el)
          const ownerId = netEl.components.networked.data.owner
          resolve(!ownerId || ownerId !== NAF.clientId ? ownerId : NAF.clientId)
        })
      }

But I have to say... If this is really necessary, and there's not a better way I'm missing...

1. maybe we should add these in to the built in utils
2. I feel like it would be ideal to update NAF so that it isn't necessary, i.e., make it so that entities always have a consistent owner available to query in an intuitive way--not sure if there's some good reason it isn't that way already...

[kylebakerio]

Alright, newest version of the draft features:

• updated color syncing, as discussed in relevant issue. uses visible="false" to achieve the desired effect instead of attachTemplateToLocal:false;
• have live-synced, auto-generated, user-settable, local-overlay nametags
• features new draft of onConnect concept, as discussed in that issue

https://glitch.com/edit/#!/8-2021-naf?path=examples%2Ftracked-controllers.html%3A97%3A0

[kylebakerio]

Hm. We should still make one nested component synced somewhere, though. it's good to have a simple example of how that works. That said, I almost never use this feature myself--might make sense to just feature that in a separate demo.

[vincentfretin]

NAF.clientId is only set when successfully connected, The clientId may come from the server, so we don't know it before we're connected. I think your NAF.utils.safelyGetOwner will continue to return empty string in init when not currently connected.
Anyway we don't currently need this for our use case, we just check if some element like .head is there to differentiate between local or remote if attachTemplateToLocal:false. If attachTemplateToLocal:true we don't even need to differentiate between local or remote.

[kylebakerio]

NAF.clientId is only set when successfully connected, The clientId may come from the server, so we don't know it before we're connected. I think your NAF.utils.safelyGetOwner will continue to return empty string in init when not currently connected. Anyway we don't currently need this for our use case, we just check if some element like .head is there to differentiate between local or remote if attachTemplateToLocal:false. If attachTemplateToLocal:true we don't even need to differentiate between local or remote.

That's right. I tried to break it up into two smaller functions, but the only reason I needed safelyGetOwner was to support ownedByLocalUser. That particular flaw in safelyGetOwner (returning a string before connection) isn't a problem when it's used by ownedByLocalUser, but it would undermine it if used directly for another purpose...

Looking at NAF source again, honestly, we should be using this:

networked-aframe/src/components/networked.js

Line 284 in ac93c62

isMine: function() {

But we should update that function to also return true if this.data.owner === ""...

...anyways, that was just something quickly thrown together to support this demo an provide a cleaner version of the method used in networked-audio-source, not really something I mean to focus on right now.

As you say, we can just dodge the issue entirely. I do think we should look into improving this in the future--functionality like this is pretty basic, and is more necessary when you're writing general purpose components, like networked-audio-source--but I agree it isn't really necessary for this demo.

Updated now to:

      AFRAME.registerComponent('player-info', {
        schema: {
          name: { type: 'string', default: "user-" + Math.round(Math.random()*10000) },
          color: { type: 'string', default: '#' + new THREE.Color( Math.random(), Math.random(), Math.random() ).getHexString() },
        },

        init: async function() {
          this.head = this.el.querySelector('.head');
          this.nametag = this.el.querySelector('.nametag');
          this.ownedByLocalUser = this.el.id === "local-head";
          if (this.ownedByLocalUser) {
            this.nametagInput = document.getElementById("username-overlay");
            this.nametagInput.value = this.data.name;
            this.nametagInput.oninput = () => {
              this.el.setAttribute('player-info', 'name', this.nametagInput.value || " ");
            };
          }
        },

        update: async function(newArgs) {
          if (this.head) this.head.setAttribute('material', 'color', this.data.color);
          if (this.nametag) this.nametag.setAttribute('value',this.data.name)
        }
      });

[vincentfretin]

This may be fine for a small demo, but I don't encourage doing this way.
I'm used to have all the UI part in a single place in a DOMContentLoaded listener in a more complex project, separate for aframe components. Here you have very few UI elements, but when you start adding buttons here and there, having the UI scattered inside aframe components will be hard to maintain and understand in my opinion. In my project I put all the UI code at the same place and I'm glad I did this, another developer will soon rewrite the UI part in react, it would be far easier for him to rewrite this part without touching any aframe components. And also it would be easier for me to review, only checking the react code, and not reviewing aframe components changes.

document.getElementById("username-overlay") may be null if the username-overlay is declared after a-scene. Or even before a-scene? I think I saw a random issue in another project even when the div was declared before a-scene.

[kylebakerio]

I'm not sure how that would make this easier... couldn't this could be re-written to react even more effortlessly, because the view is simpler and has fewer interactions with the logic?

For me, views should be simple and only have view. Logic/JS should be kept out of view/HTML.

I agree that in a more robust project, one would want to make sure the control flow is correct (e.g., document.addEventListener('DOMContentLoaded'). I also agree that in larger projects, it could be a difference of opinion about whether to put this logic in the DOM node or not. Again, I'd argue that in a rewrite of a view, the less that's there, the better--the separation of concerns would help.

My perspective:
No matter what, we cannot have all the logic related to that input in the input itself--so, imo, we should have none of it there. Logic should not be spread out if possible. The less 'jumping' you have to do, the easier code is to grok. The larger a project gets, the more separation becomes necessary, but I think it's wrong to try and force a small project to look like a large project arbitrarily.

I also think we're bikeshedding here, fwiw. This is not incorrect code, and I think it's appropriate for a pedagogical model. It's easy to read and understand what it does, which is the point here. A dev can easily pick up what is happening and adapt principles from this code to their own paradigm of choice.

I could argue that everything shouldn't be in one file, either. We don't do that because we're showing how projects should be organized, we do that because this is a simple teaching example. In small projects, it's nice for things to be in one file. Obviously that doesn't scale, though.

document.getElementById("username-overlay") may be null if the username-overlay is declared after a-scene. Or even before a-scene? I think I saw a random issue in another project even when the div was declared before a-scene.

I tested it to be sure. Declaring it after does not produce an issue. A-Frame delays calling init until the page is ready, but I am not curious about exactly what all it waits on, and in a quick search wasn't able to find it in the source code (if you find it, let me know).

    <input 
      id="username-overlay"
      style="z-index: 100; bottom: 24px; left: 24px; position:fixed;"
      oninput="document.getElementById('local-avatar').setAttribute('player-info', 'name', this.value || ' ')"
    ></input>

(I also add a line in the component to add that ID to the user's avatar, of course)

Anyways, I'm fine doing it this way. I don't have a strong opinion, and if you prefer it this way, I think that's ok and could be argued as well. I've updated the code to this.

[vincentfretin]

I would also remove the this.nametagInput remaining code that set the default input value from the player-info component, and do that external to the component. I personally don't like the oninput as as string like this. I prefer your previous code where you handled the default value and the oninput in the same place but we may have an issue getting the input element inside player-info init as discussed. What I had in mind really was this (not tested):

<input 
  id="username-overlay"
  style="z-index: 100; bottom: 24px; left: 24px; position:fixed;"
  />
<script>
document.addEventListener('DOMContentLoaded', () => {
  const player = document.getElementById("local-head");
  const nameInput = document.getElementById("username-overlay");
  nameInput.value = "user-" + Math.round(Math.random()*10000);
  nameInput.oninput = () => {
    player.setAttribute('player-info', 'name', nameInput.value);
  };
});
</script>

and in player-info having only in init:

       init: async function() {
          this.head = this.el.querySelector('.head');
          this.nametag = this.el.querySelector('.nametag');
        },

Is this clearer?

[kylebakerio]

Is there anything else? Are we settled on this at this stage? If so, I think the only thing left would be updates to onConnect, but will need to do that pull request first before changing that, and so I'd recommend we just:

• accept these changes now
• later I'll update onConnect, and with that pull request, will update this demo along with the documentation

[vincentfretin]

Excepted the name input code I suggested to modify, other changes are fine.

[kylebakerio]

Alright, I'll merge this in, then. There are a few more small things to change possibly, but this is a big step forward for the demos for now. Next things that I'll be looking to address are

• doing a pull request to update onConnect API
• maybe looking at updating the schema API, that's pretty rough right now and needs to be fixed (are you working on this, or interested in doing it? If not, it's bothering me and I'd like to see it improve, and could at least do a high level wrapper around this method)
• connecting our glitch demos to this repo to make it easy to keep them in sync, if we don't have a process in place for that, and adding an npm script command to do it.

Once those are done, and updated here, I'll look to update the other demos to follow this example.

also... interested in possibly coming up with a better format for our demos so that they're easier to maintain. Technically there's a lot of code duplication between demos, which makes for quite a maintenance headache when we want to update them. Would be nice to come up with a way to have a single demo template that gets used across all the demos and makes updating the core of them straightforward. I'll think on this.

Other things I'd like as well:

• doing a full networked-hands.js similar to our networked-audio, that picks up A-Frame hand events, to allow actual hand gestures
• another thing I really want to do, but should be a different demo: networked hand-tracking, that keeps full skeletons in sync for using actual full hand-tracking (instead of this demo, which is controller-tracking).

[vincentfretin]

* maybe looking at updating the schema API, that's pretty rough right now and needs to be fixed (are you working on this, or interested in doing it? If not, it's bothering me and I'd like to see it improve, and could at least do a high level wrapper around this method)

I'm not working on it, and fixing it is not currently on my agenda. If you find a way to fix the issue in a PR from the comments I wrote in #267, we'll discuss it then.