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

Faster muting/deafening at stage change #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lscobe16
Copy link
Contributor

@lscobe16 lscobe16 commented Nov 5, 2020

There is an issue regarding lags during the transition between game stages: #48
I too, can see, that sometimes it takes a while for the bot to mute/unmute everyone one by one.

Looking at the code, this is the reason for such "lag":

for (const player of sortedPlayers) {
      await this.updatePlayerMute(player)
}

The bot always waits for one user to be muted until he initiates the muting of the next user.

I can totally see, why you did this, because of the sorted array. You don't want dead players to shortly be able to talk in discussion. To achieve that, you always first muted users one by one, and then unmuted other users one by one.

First of all: the await here did not have any effect, because this.updatePlayerMute(player) and Utils.editMember() didn't even return a Promise.
That's fixed now.
So to be honest, I am not sure, whether this really speeds up the transition, but see for yourself:

Also there is a faster way to do so:
Using the Promise.all() feature in ES6 (no problem with Node.js 12) it is possible to first mute(and deafen) all players, that have to be muted, at once, wait for all of those to finish, and then unmute everyone else.
This way, the bot doesn't have to wait 10 times (for a full round), but only 2 times.
Because of the waiting (not for a timeout, but for everyone to be muted) in between, it is still guaranteed, that no voice is leaked to people not supposed to hear it.

@pedrofracassi
Copy link
Owner

Thanks for your contribution! I'll get to reviewing it when I have some time later this week.

As for that last part about Promise.all:

Using the Promise.all() feature in ES6 (no problem with Node.js 12) it is possible to first mute(and deafen) all players, that have to be muted, at once, wait for all of those to finish, and then unmute everyone else.
This way, the bot doesn't have to wait 10 times (for a full round), but only 2 times.
Because of the waiting (not for a timeout, but for everyone to be muted) in between, it is still guaranteed, that no voice is leaked to people not supposed to hear it.

This probably won't be possible. Another reason why I use await is to intentionally slow down requests to the Discord API, as the "lag" is way worse if we hit the ratelimit (which is super low at 5req/5s) and have to wait for it to reset.

@lscobe16
Copy link
Contributor Author

lscobe16 commented Nov 6, 2020

Hi, of course, do it when you have time.

Also I was not aware of those rate limits.
But as I already wrote the await didn't do anything until now. So all the requests to mute were fired at once.
And there weren't much problems, that point to a rate limit being hit, were there? (except that is already what the lag came from? that would require further investigation...)
So I am certain, that adding all my changes won't make the rate limit issue worse than it is now.

However, if you just put in those two returns that pass the Promises from member.edit() over Game.updatePlayerMute() to the setStage() function, then I guess the code would be what you initially intended with the await. So only then the bot would mute everyone one after another. (Best for rate limits, but slower than now)

After all it's up to you to find the right compromise between performance/speed and holding back in order to stay within the rate limits. I don't have enough experience with that to make a qualified decision/recommendation.

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

Successfully merging this pull request may close these issues.

2 participants