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.

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

Ownership change, gained, lost events no longer fire #178

Closed
PlumCantaloupe opened this issue Feb 5, 2020 · 3 comments
Closed

Ownership change, gained, lost events no longer fire #178

PlumCantaloupe opened this issue Feb 5, 2020 · 3 comments

Comments

@PlumCantaloupe
Copy link

Not sure when this started happening but on the most recent NAF update the ownership events do not fire (including on toggle-ownership example).

The issue seems stems from the comparison that happens in the component networked.js here:

if (owner && !this.isMine() && lastOwnerTime < now) {

The lastOwnerTime variable is never initialized (and so the comparison between date and -1 is always false).

The following line, that also checks for the -1, appears to fix the issue but I am not certain it is the correct approach. Perhaps lastOwnerTime needs to be initialized on network creation :)

if (owner && !this.isMine() && (lastOwnerTime < now || lastOwnerTime === -1)) {
@vincentfretin
Copy link
Member

I just looked into this with current master. I don't see anything wrong with the code.
this.lastOwnerTime is initialized to -1 in init at

this.lastOwnerTime = -1;

and is set on connect here
this.lastOwnerTime = NAF.connection.getServerTime();

and the condition you're referring here
const lastOwnerTime = this.lastOwnerTime;
const now = NAF.connection.getServerTime();
if (owner && !this.isMine() && lastOwnerTime < now) {
this.lastOwnerTime = now;

seems to works as attended.

Date.now()
1616314285921
-1 < Date.now()
true

so the comparison between -1 and a date is true here. There is no need for and additional || lastOwnerTime === -1
At one point there was an issue with the implementation of NAF.connection.getServerTime() in the new socketio/webrtc adapters, you may have encountered an issue with this?
All three events are working properly, I can see the cube change the opacity between three users. See

el.addEventListener('ownership-gained', e => {
that.updateOpacity(1);
});
el.addEventListener('ownership-lost', e => {
that.updateOpacity(0.5);
});
el.addEventListener('ownership-changed', e => {
clearTimeout(timeout);
console.log(e.detail)
if (e.detail.newOwner == NAF.clientId) {
//same as listening to 'ownership-gained'
} else if (e.detail.oldOwner == NAF.clientId) {
//same as listening to 'ownership-lost'
} else {
that.updateOpacity(0.8);
timeout = setTimeout(() => {
that.updateOpacity(0.5);
}, 200)
}
});

@vincentfretin
Copy link
Member

I tested socketio, webrtc and easyrtc adapters, all three are working properly.

@PlumCantaloupe
Copy link
Author

Ach, thank you. Sorry, I should have closed this a while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants