-
Notifications
You must be signed in to change notification settings - Fork 296
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.
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
Feature/ownership transfer events #99
Feature/ownership transfer events #99
Conversation
Looks great! Could you add these events to the readme in the events section too? https://github.com/networked-aframe/networked-aframe#events |
README.md
Outdated
| | | `evt.detail.previousOwner` - the clientId of the previous owner | | ||
| ownership-lost | Fired when a networked entity's ownership is lost | `evt.detail.el` - the entity whose ownership was lost | | ||
| | | `evt.detail.newOwner` - the clientId of the new owner | | ||
| ownership-changed | Fired when a networked entity's ownership is changed, but you are neither the new or previous owner | `evt.detail.el` - the entity whose ownership was lost | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I would have expected -ownership-changed
to have fired any time the owner changes (including if it was me losing or becoming the owner). You could then actually get rid of the other events. Most of the times you use this you are going to have to handle all the cases, seems like it would be easier to just have a single event that forces you to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, but it seems like most aframe packages, (including aframe itself) tends to favor explicit events, as opposed to more generic events that have a type
field in the evt.details.
e.g. child-attached
, child-detached
; stateadded
, state-removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see what you're suggesting down below. Though I disagree with "Most of the times you use this you are going to have to handle all the cases". I think by far the most used case will be the ownership-lost event, since that happens asynchronously, as opposed to taking ownership which is handled synchronously with whatever code is running that calls takeOwnership()
. And in your example you always have to write if(e.oldOwner === NAF.clientId)
to handle that event. I can see the case for both though.
this.networkedEl.addEventListener("ownership-changed", e => { | ||
this.el.setAttribute('material', 'opacity', 0.8); | ||
setTimeout(() => { | ||
this.el.setAttribute('material', 'opacity', 0.5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition here since this could end up firing after one of the other events. You would need to store the timeout and clear it in the other events like:
let blinkTimeout;
this.networkedEl.addEventListener("ownership-gained", e => {
clearTimeout(blinkTimeout);
this.el.setAttribute("material", "opacity", 1);
});
this.networkedEl.addEventListener("ownership-lost", e => {
clearTimeout(blinkTimeout);
this.el.setAttribute("material", "opacity", 0.5);
});
this.networkedEl.addEventListener("ownership-changed", e => {
this.el.setAttribute("material", "opacity", 0.8);
blinkTimeout = setTimeout(() => {
this.el.setAttribute("material", "opacity", 0.5);
}, 200);
});
Or if we change the events like I suggested above:
let blinkTimeout;
this.networkedEl.addEventListener("ownership-changed", e => {
clearTimeout(blinkTimeout);
if(e.newOwner === NAF.clientId) {
this.el.setAttribute("material", "opacity", 1);
} else if(e.oldOwner === NAF.clientId) {
this.el.setAttribute("material", "opacity", 0.5);
} else {
this.el.setAttribute("material", "opacity", 0.8);
blinkTimeout = setTimeout(() => {
this.el.setAttribute("material", "opacity", 0.5);
}, 200);
}
})
…or lost for anyone.
This reverts commit 5b5ba5c.
add events to make it easier for components to handle changes of ownership transfer state