Party full bell #85

Merged
merged 5 commits into from Dec 12, 2013

Conversation

Projects
None yet
4 participants
@cbeck88
Member

cbeck88 commented Dec 4, 2013

I decided to do a little coding to fulfill a feature request made originally on forums, later posted to bugs.w.o here:

https://gna.org/bugs/?21211

The change is very simple, in multiplayer_connect.cpp I add 3 lines to function connect::update_player_list. This function already may call ui::set_user_list which can produce sounds when players are added or leave. After that event I add a check, if the engine says there are no slots available and the silent flag is off, then ask ui to play the "turn bell" sound, which alerts host that game is ready to start. I also must include sound.hpp in multiplayer_connect.cpp.

I think this is the appropriate place to add this code, as ui::set_user_list does not have the engine in its scope. It might be a bit annoying if in some circumstance, update_player_list is called again and again and the bell plays many times -- as far as I know there is no circumstance where this happens, limited testing suggests good performance.

iceiceice

@cbeck88

This comment has been minimized.

Show comment Hide comment
@cbeck88

cbeck88 Dec 4, 2013

Member

Hmm, this error doesn't make sense to me, it says Travis failed to connect to github.com/wesnoth/wesnoth-old... is there someway to make it try again?

Member

cbeck88 commented Dec 4, 2013

Hmm, this error doesn't make sense to me, it says Travis failed to connect to github.com/wesnoth/wesnoth-old... is there someway to make it try again?

@bumbadadabum

This comment has been minimized.

Show comment Hide comment
@bumbadadabum

bumbadadabum Dec 4, 2013

Member

Ignore the error.

Member

bumbadadabum commented Dec 4, 2013

Ignore the error.

@cbeck88 cbeck88 closed this Dec 4, 2013

@bumbadadabum

This comment has been minimized.

Show comment Hide comment
@bumbadadabum

bumbadadabum Dec 4, 2013

Member

Why did you close it?

Member

bumbadadabum commented Dec 4, 2013

Why did you close it?

@cbeck88

This comment has been minimized.

Show comment Hide comment
@cbeck88

cbeck88 Dec 4, 2013

Member

i just reposted it to try to get travis to build it again...

hmm looks like that didn't help... i guess i will reopen and delete the other one

Member

cbeck88 commented Dec 4, 2013

i just reposted it to try to get travis to build it again...

hmm looks like that didn't help... i guess i will reopen and delete the other one

@cbeck88 cbeck88 reopened this Dec 4, 2013

@cbeck88

This comment has been minimized.

Show comment Hide comment
@cbeck88

cbeck88 Dec 4, 2013

Member

Well the closed duplicate compiled just fine... https://travis-ci.org/wesnoth/wesnoth-old/builds/14947364

Anyways I'm going to stop worrying about travis.

Member

cbeck88 commented Dec 4, 2013

Well the closed duplicate compiled just fine... https://travis-ci.org/wesnoth/wesnoth-old/builds/14947364

Anyways I'm going to stop worrying about travis.

@shikadiqueen

This comment has been minimized.

Show comment Hide comment
@shikadiqueen

shikadiqueen Dec 5, 2013

Member

Wouldn't it be better to add a new global sound option instead of game_config::sounds::turn_bell in case we get/decide to use a new dedicated bell sound for the "game full" notification?

At least in my opinion, the turn bell might not be the best choice for this feature since it is also normally heard at the beginning of the first player's turn once the game starts (i.e. when the host decides to start the game). A new sound option might as well continue to use the turn bell as a placeholder until a better sound effect becomes available.

Member

shikadiqueen commented Dec 5, 2013

Wouldn't it be better to add a new global sound option instead of game_config::sounds::turn_bell in case we get/decide to use a new dedicated bell sound for the "game full" notification?

At least in my opinion, the turn bell might not be the best choice for this feature since it is also normally heard at the beginning of the first player's turn once the game starts (i.e. when the host decides to start the game). A new sound option might as well continue to use the turn bell as a placeholder until a better sound effect becomes available.

@cbeck88

This comment has been minimized.

Show comment Hide comment
@cbeck88

cbeck88 Dec 5, 2013

Member

Okay, that's a good point. I can try to figure out how to register a new global sound option.

My thinking is that the turn bell sound is already the "time to come back from the kitchen and make your turns" signal, and I want a sound that says "hey its time for you to give attention to the game". I only had a few options, this seemed to work better than the "server message received" sound.

Member

cbeck88 commented Dec 5, 2013

Okay, that's a good point. I can try to figure out how to register a new global sound option.

My thinking is that the turn bell sound is already the "time to come back from the kitchen and make your turns" signal, and I want a sound that says "hey its time for you to give attention to the game". I only had a few options, this seemed to work better than the "server message received" sound.

@AI0867

This comment has been minimized.

Show comment Hide comment
@AI0867

AI0867 Dec 5, 2013

Member

Please don't open new pull requests to get new travis builds. We (the admins) can request retries on the builds if needed, and an errored travis build certainly isn't a reason to reject a PR.

Member

AI0867 commented Dec 5, 2013

Please don't open new pull requests to get new travis builds. We (the admins) can request retries on the builds if needed, and an errored travis build certainly isn't a reason to reject a PR.

@cbeck88

This comment has been minimized.

Show comment Hide comment
@cbeck88

cbeck88 Dec 5, 2013

Member

Sorry about that, I am pretty new to github and travis.

Member

cbeck88 commented Dec 5, 2013

Sorry about that, I am pretty new to github and travis.

@ghost ghost assigned shikadiqueen Dec 8, 2013

cbeck88 added some commits Dec 4, 2013

added game_config::sounds::party_full_sound, defaults to turn bell
and switched multiplayer_connect.cpp to use this sound
changed debugging output message
changed the logic for the bell:
connect engine must report no sides are available and game can start.
@shikadiqueen

This comment has been minimized.

Show comment Hide comment
@shikadiqueen

shikadiqueen Dec 9, 2013

Member

After some quick testing, it seems the game isn't truly ready to start the moment the I'm Ready button is enabled and the party full bell sound is played -- the other players might be busy choosing a leader unit type and this is not taken into account.

Still, I'd consider that a separate bug/area in need of enhancement.

Member

shikadiqueen commented Dec 9, 2013

After some quick testing, it seems the game isn't truly ready to start the moment the I'm Ready button is enabled and the party full bell sound is played -- the other players might be busy choosing a leader unit type and this is not taken into account.

Still, I'd consider that a separate bug/area in need of enhancement.

@shikadiqueen

This comment has been minimized.

Show comment Hide comment
@shikadiqueen

shikadiqueen Dec 9, 2013

Member

If you add yourself to Miscellaneous Contributors in data/core/about.cfg, this should be good to go.

Member

shikadiqueen commented Dec 9, 2013

If you add yourself to Miscellaneous Contributors in data/core/about.cfg, this should be good to go.

@cbeck88

This comment has been minimized.

Show comment Hide comment
@cbeck88

cbeck88 Dec 11, 2013

Member

"-- the other players might be busy choosing a leader unit type and this is not taken into account.

Still, I'd consider that a separate bug/area in need of enhancement." --shikadilord

My two cents: If we want to fix that, we should also at the same time make it so that the other players in the game can see that there is a player still picking faction. I remember being in mp games where the last player joins but takes a very long time to pick faction, and the players already in the game will get frustrated with the host for not starting. So maybe the best change would be to have a text notice "picking faction" for that player when it is the case, and also delay the bell until they are done. Anyways, something to think about for the future.

Member

cbeck88 commented Dec 11, 2013

"-- the other players might be busy choosing a leader unit type and this is not taken into account.

Still, I'd consider that a separate bug/area in need of enhancement." --shikadilord

My two cents: If we want to fix that, we should also at the same time make it so that the other players in the game can see that there is a player still picking faction. I remember being in mp games where the last player joins but takes a very long time to pick faction, and the players already in the game will get frustrated with the host for not starting. So maybe the best change would be to have a text notice "picking faction" for that player when it is the case, and also delay the bell until they are done. Anyways, something to think about for the future.

shikadiqueen added a commit that referenced this pull request Dec 12, 2013

@shikadiqueen shikadiqueen merged commit 11c0382 into wesnoth:master Dec 12, 2013

1 check passed

default The Travis CI build passed
Details

@cbeck88 cbeck88 deleted the cbeck88:party-full-bell branch Dec 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment