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

Fixed a skirmish arena queue bug #336

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@Senader
Contributor

Senader commented Dec 24, 2012

Fixed a skirmish arena queue bug where the arena launched when you were only 2 in the queue (so 1v1).
Now works properly : it wait for four players to tag 2v2 (six to 3v3) before launching one(for rated too).
It didn't affect rated because queue was only in a full group, so there were never only 2 people in the queue.

Fixed a skirmish arena queue bug
Fixed a skirmish arena queue bug where the arena launched when you were only 2 in the queue (so 1v1).
Now works properly (for rated too).
@dfighter1985

This comment has been minimized.

Show comment
Hide comment
@dfighter1985

dfighter1985 Jan 30, 2013

Member

I'd rather have this done in the world config file, not by doubling the wrong value in the code :P

Member

dfighter1985 commented Jan 30, 2013

I'd rather have this done in the world config file, not by doubling the wrong value in the code :P

@Senader

This comment has been minimized.

Show comment
Hide comment
@Senader

Senader Feb 1, 2013

Contributor

You haven't done anything. This is the simplest and less ugly possible way to fix it.
You do really imagine creating ('cause those lines doesn't exist) a line "MEMBERS REQUIRED FOR 2V2 ARENA : 2" ?
Fact is this code double the amount of people required to launch a skirmish game because, for now, the core only seek for X players in a XvX game. X + X = X * 2.

I made tests, unlike you. This works fine. And this is the right value ;) What about having reading classes ?

PS : Prove me I'm wrong and the code committed is wrong. I would learn something.

Contributor

Senader commented Feb 1, 2013

You haven't done anything. This is the simplest and less ugly possible way to fix it.
You do really imagine creating ('cause those lines doesn't exist) a line "MEMBERS REQUIRED FOR 2V2 ARENA : 2" ?
Fact is this code double the amount of people required to launch a skirmish game because, for now, the core only seek for X players in a XvX game. X + X = X * 2.

I made tests, unlike you. This works fine. And this is the right value ;) What about having reading classes ?

PS : Prove me I'm wrong and the code committed is wrong. I would learn something.

@dfighter1985

This comment has been minimized.

Show comment
Hide comment
@dfighter1985

dfighter1985 Feb 1, 2013

Member

First of all please drop the attitude!

I don't need to test something to see it's wrong.
Just read where

    uint32 minPlayers = BattlegroundManager.GetMinimumPlayers(i);

gets the value from.
It's this method:
https://github.com/arcemu/arcemu/blob/master/src/arcemu-world/BattlegroundMgr.cpp#L844

This already uses the config file for the BGs, there's no reason why Arenas cannot do the exact same thing.
Doing it that way is much cleaner than hacking a multiplier into the check...

Member

dfighter1985 commented Feb 1, 2013

First of all please drop the attitude!

I don't need to test something to see it's wrong.
Just read where

    uint32 minPlayers = BattlegroundManager.GetMinimumPlayers(i);

gets the value from.
It's this method:
https://github.com/arcemu/arcemu/blob/master/src/arcemu-world/BattlegroundMgr.cpp#L844

This already uses the config file for the BGs, there's no reason why Arenas cannot do the exact same thing.
Doing it that way is much cleaner than hacking a multiplier into the check...

@Senader

This comment has been minimized.

Show comment
Hide comment
@Senader

Senader Feb 1, 2013

Contributor

So your point is to make the battleground part of the config file looks like that :
<Battleground AV_MIN="10"
AV_MAX="40"
AB_MIN="5"
AB_MAX="15"
WSG_MIN="5"
WSG_MAX="10"
EOTS_MIN="5"
EOTS_MAX="15"
SOTA_MIN="5"
SOTA_MAX="15"
IOC_MIN="10"
IOC_MAX="40"
2V2ARENA_MIN="4"
3V3ARENA_MIN="6"
5V5ARENA_MIN="10">

Don't you find it unreadable ? Doubling the value for the ArenaCheck, or modifying returns form GetMinimumPlayers (which is still ugly) are far better. It is far more understandable to double the amount in order to let both teams get full.
Also, this, at least, is done and works (since a month).
But please, tell me how wrong I am to change this value. As far as I know, the last revamp of Battleground and arenas code is from you, so lead my blinded eyes to my fault.

Contributor

Senader commented Feb 1, 2013

So your point is to make the battleground part of the config file looks like that :
<Battleground AV_MIN="10"
AV_MAX="40"
AB_MIN="5"
AB_MAX="15"
WSG_MIN="5"
WSG_MAX="10"
EOTS_MIN="5"
EOTS_MAX="15"
SOTA_MIN="5"
SOTA_MAX="15"
IOC_MIN="10"
IOC_MAX="40"
2V2ARENA_MIN="4"
3V3ARENA_MIN="6"
5V5ARENA_MIN="10">

Don't you find it unreadable ? Doubling the value for the ArenaCheck, or modifying returns form GetMinimumPlayers (which is still ugly) are far better. It is far more understandable to double the amount in order to let both teams get full.
Also, this, at least, is done and works (since a month).
But please, tell me how wrong I am to change this value. As far as I know, the last revamp of Battleground and arenas code is from you, so lead my blinded eyes to my fault.

@dfighter1985

This comment has been minimized.

Show comment
Hide comment
@dfighter1985

dfighter1985 Feb 1, 2013

Member

No, it's much more readable, not to mention it's also a lot more flexible, since you can change the values without having to rebuild Arcemu.
Also I've specifically asked you to drop the attitude.

Member

dfighter1985 commented Feb 1, 2013

No, it's much more readable, not to mention it's also a lot more flexible, since you can change the values without having to rebuild Arcemu.
Also I've specifically asked you to drop the attitude.

@Senader

This comment has been minimized.

Show comment
Hide comment
@Senader

Senader Feb 1, 2013

Contributor

You don't need flexibility. A 2v2 skirmish who need 2 players to start a match means you can fight in 2v1 if unlucky. In fact, it's not even luck as it happened a lot on my test server (we were four, but if the last one tagged a bit too late, we were in 2v1). That's why I fixed it. That's why, now, it works as intended.
Also, saying the minplayers for a 2v2 is 4 isn't "readable" at all. Let's say a random people change it to 5 : impossible to launch any Arena. To 1. People can enter rated arena without any enemy and grind wins. To any other value than the one intended : It will screw up. Why do you think arena values are simple returns on the GetMinimumPlayers ? Because it has to be only one value. In BG, it depends a lot, as you can imagine a 3v3 Warsong on lowpop servers. That's why flexibility is needed here.
Can you imagine a 2v2-3v3 arena ? It makes no sense.

Contributor

Senader commented Feb 1, 2013

You don't need flexibility. A 2v2 skirmish who need 2 players to start a match means you can fight in 2v1 if unlucky. In fact, it's not even luck as it happened a lot on my test server (we were four, but if the last one tagged a bit too late, we were in 2v1). That's why I fixed it. That's why, now, it works as intended.
Also, saying the minplayers for a 2v2 is 4 isn't "readable" at all. Let's say a random people change it to 5 : impossible to launch any Arena. To 1. People can enter rated arena without any enemy and grind wins. To any other value than the one intended : It will screw up. Why do you think arena values are simple returns on the GetMinimumPlayers ? Because it has to be only one value. In BG, it depends a lot, as you can imagine a 3v3 Warsong on lowpop servers. That's why flexibility is needed here.
Can you imagine a 2v2-3v3 arena ? It makes no sense.

@dfighter1985

This comment has been minimized.

Show comment
Hide comment
@dfighter1985

dfighter1985 Feb 2, 2013

Member

It makes no sense to you.

Member

dfighter1985 commented Feb 2, 2013

It makes no sense to you.

@Senader

This comment has been minimized.

Show comment
Hide comment
@Senader

Senader Feb 2, 2013

Contributor

Your only point is to allow unbalanced games by permitting 1v2 and more ? This ain't Blizzlike nor fun. What's the point of this emulator so ?

Contributor

Senader commented Feb 2, 2013

Your only point is to allow unbalanced games by permitting 1v2 and more ? This ain't Blizzlike nor fun. What's the point of this emulator so ?

if(!forceStart && tempPlayerVec[0].size() < minPlayers)
continue;
if(CanCreateInstance(i, j))
// if(CanCreateInstance(i, j))
if(forceStart || tempPlayerVec[0].size() >= minPlayers)

This comment has been minimized.

@Senader

This comment has been minimized.

Show comment
Hide comment
@Senader

Senader Feb 3, 2013

Contributor

Yes it is. You can change it by an "else".

Contributor

Senader commented Feb 3, 2013

Yes it is. You can change it by an "else".

@dfighter1985

This comment has been minimized.

Show comment
Hide comment
@dfighter1985

dfighter1985 Feb 3, 2013

Member

@Senader Fun and balance are matters of perspective. Besides Arcemu is not an emulator, it's a reimplementation of the World Of Warcraft 3.3.5a protocol.

Member

dfighter1985 commented Feb 3, 2013

@Senader Fun and balance are matters of perspective. Besides Arcemu is not an emulator, it's a reimplementation of the World Of Warcraft 3.3.5a protocol.

@Senader

This comment has been minimized.

Show comment
Hide comment
@Senader

Senader Feb 3, 2013

Contributor

Do whatever you want. I fixed it as a lot of garbage in battlegrounds and arenas code. I was willing to help, but you prefer doing it another way. As you wish.

Contributor

Senader commented Feb 3, 2013

Do whatever you want. I fixed it as a lot of garbage in battlegrounds and arenas code. I was willing to help, but you prefer doing it another way. As you wish.

@dfighter1985

This comment has been minimized.

Show comment
Hide comment
@dfighter1985

dfighter1985 Feb 3, 2013

Member

Help is always welcome, and yes we do prefer Arcemu to be maximally flexible.

Member

dfighter1985 commented Feb 3, 2013

Help is always welcome, and yes we do prefer Arcemu to be maximally flexible.

dfighter1985 added a commit that referenced this pull request Feb 5, 2013

MODIFIED: Arena player limits can now be set in the world config file…
…. Arena instances will now only be created if the proper amount of players are queued. #336

Artox pushed a commit to Artox/arcemu that referenced this pull request Mar 11, 2013

MODIFIED: Arena player limits can now be set in the world config file…
…. Arena instances will now only be created if the proper amount of players are queued. #336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment