Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tracked Controllers Example Overhaul #290

Merged
merged 10 commits into from
Oct 14, 2021
Merged

Tracked Controllers Example Overhaul #290

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
112c512
updated environment to be more appealing, and suggest that we update …
kylebakerio Aug 15, 2021
59a72f6
further code style improvements, avatar appearance improvements, comm…
kylebakerio Aug 15, 2021
6b16e18
switched to new style sync schema--seems to be working now, think I h…
kylebakerio Aug 15, 2021
04f3c3c
more minor comment and style changes
kylebakerio Aug 15, 2021
9052c4f
Update tracked-controllers.html
kylebakerio Aug 29, 2021
a584869
Update tracked-controllers.html
kylebakerio Aug 29, 2021
9f55f92
latest updates: draft of new NAF.connection.onConnect usage, draft mo…
kylebakerio Oct 10, 2021
c7fa3e2
Merge branch 'update-examples' of github.com:networked-aframe/network…
kylebakerio Oct 10, 2021
af3fa26
style tweaks to player-info component
kylebakerio Oct 10, 2021
ee96468
request changes, and other minor improvements; minor bugfix for code …
kylebakerio Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 16 additions & 33 deletions examples/tracked-controllers.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,52 +106,35 @@
// could add as 'networked-hands' component within repo
}
})

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)
})
}

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() },
nametag: { type: 'string', default: "user-" + Math.round(Math.random()*10000) },
},

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

// for handling usernames
NAF.utils.ownedByLocalUser(this.el).then(local => {
if (local) {
document.querySelector("#username-overlay").value = this.data.nametag;
document.querySelector("#username-overlay").oninput = () => {
this.data.nametag = document.querySelector("#username-overlay").value || " "
};
}
console.log("userlist", [...document.querySelectorAll('[player-info]')].map(el => el.components['player-info'].data.nametag))
})
this.ownedByLocalUser = this.el.id === "local-head"; // await NAF.utils.ownedByLocalUser(this.el);
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 || " ");
};
}
},
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?


listUsers: function() {
console.log("userlist", [...document.querySelectorAll('[player-info]')].map(el => el.components['player-info'].data.nametag))
},

update: async function(newArgs) {
console.log('update',this.data, newArgs)
try {
this.nametag.setAttribute('value',this.data.nametag)
this.head.setAttribute('material', 'color', this.data.color);
} catch (e) {
console.warn("will benignly fail when `this.head` doesn't exist, which is the case for local user, who has visible='false' set.",e)
}
if (this.head) this.head.setAttribute('material', 'color', this.data.color);
if (this.nametag) this.nametag.setAttribute('value',this.data.name)
}
});
</script>
Expand Down