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

1.15.1 possibly broken `droid` command #4308

Closed
vgaming opened this issue Sep 3, 2019 · 10 comments · Fixed by #4313
Labels

Comments

@vgaming
Copy link
Member

@vgaming vgaming commented Sep 3, 2019

Steps to reproduce:

  • start any game
  • type in user command droid
  • observe the response "Invalid action provided for side '1'. Valid actions are: on, off, full" (this is good)
  • type in droid on

Expected at this point:

  • current side (1) gets droided

Actually:

  • "Error: can't droid invalid side: '0'.
@vgaming vgaming changed the title possibly broken `droid` side detection 1.15.1 possibly broken `droid` side detection Sep 3, 2019
@soliton-

This comment has been minimized.

Copy link
Member

@soliton- soliton- commented Sep 3, 2019

The syntax is: droid [<side> [on/off/full]]

Just droid should work. droid on is not valid.

@soliton- soliton- added Bug Command Line UI and removed Command Line labels Sep 3, 2019
@soliton-

This comment has been minimized.

Copy link
Member

@soliton- soliton- commented Sep 3, 2019

Looks like there is no default anymore for the last argument. Default should be to toggle. I.e. if the side is AI controlled switch away from it and if not switch it to AI.

@vgaming

This comment has been minimized.

Copy link
Member Author

@vgaming vgaming commented Sep 4, 2019

@soliton- you're right indeed. I've edited the description a bit if you don't mind as well.

@vgaming vgaming changed the title 1.15.1 possibly broken `droid` side detection 1.15.1 possibly broken `droid` command Sep 4, 2019
@Pentarctagon

This comment has been minimized.

Copy link
Member

@Pentarctagon Pentarctagon commented Sep 4, 2019

Do you get the same invalid side error on 1.14? on/off/full not being optional was probably caused by me when I added the "full" option, but the logic determining the side when not provided hasn't changed between 1.14 and master.

@vgaming

This comment has been minimized.

Copy link
Member Author

@vgaming vgaming commented Sep 4, 2019

@Pentarctagon when I enter droid when playing side 1 on wesnoth-1.14, I get it toggled (as expected). The "synopsis" that you see by entering command help droid also suggests that droid should do something without arguments. I checked that yesterday on ArchLinux-s system wesnoth-1.14.7-1.

@vgaming

This comment has been minimized.

Copy link
Member Author

@vgaming vgaming commented Sep 4, 2019

@Pentarctagon code-wise, I think the problem is with parsing "action" (on/off/full) instead of parsing side, as already noted by @soliton- .

@soliton-

This comment has been minimized.

Copy link
Member

@soliton- soliton- commented Sep 4, 2019

There's no default handling for action here:

wesnoth/src/menu_events.cpp

Lines 1442 to 1471 in efd808e

if(action == "on") {
if(!menu_handler_.board().get_team(side).is_human() || !menu_handler_.board().get_team(side).is_droid()) {
menu_handler_.board().get_team(side).make_human();
menu_handler_.board().get_team(side).make_droid();
changed = true;
print(get_cmd(), VGETTEXT("Side '$side' controller is now controlled by: AI.", symbols));
} else {
print(get_cmd(), VGETTEXT("Side '$side' is already droided.", symbols));
}
} else if(action == "off") {
if(!menu_handler_.board().get_team(side).is_human() || !menu_handler_.board().get_team(side).is_proxy_human()) {
menu_handler_.board().get_team(side).make_human();
menu_handler_.board().get_team(side).make_proxy_human();
changed = true;
print(get_cmd(), VGETTEXT("Side '$side' controller is now controlled by: human.", symbols));
} else {
print(get_cmd(), VGETTEXT("Side '$side' is already not droided.", symbols));
}
} else if(action == "full") {
if(!menu_handler_.board().get_team(side).is_ai() || !menu_handler_.board().get_team(side).is_droid()) {
menu_handler_.board().get_team(side).make_ai();
menu_handler_.board().get_team(side).make_droid();
changed = true;
print(get_cmd(), VGETTEXT("Side '$side' controller is now fully controlled by: AI.", symbols));
} else {
print(get_cmd(), VGETTEXT("Side '$side' is already fully AI controlled.", symbols));
}
} else {
print(get_cmd(), VGETTEXT("Invalid action provided for side '$side'. Valid actions are: on, off, full.", symbols));
}

If action is empty it should call toggle_droid().

@Pentarctagon

This comment has been minimized.

Copy link
Member

@Pentarctagon Pentarctagon commented Sep 4, 2019

@vgaming The check for whether "side" is valid comes first though, so if the error "Error: can't droid invalid side: '0'." happens after just typing :droid, then that seems to indicate that the line I linked is also giving different results in 1.14 vs master.

@soliton- If side 1 called :droid 1 full and then :droid, that would leave them with a controller of AI but a proxy controller of PROXY_HUMAN for example, which doesn't sound right.

@soliton-

This comment has been minimized.

Copy link
Member

@soliton- soliton- commented Sep 4, 2019

The side is no issue except that if the last optional parameter doesn't work properly it doesn't matter how earlier parameters are handled.

I just mentioned toggle_droid() as what was done before. I have not looked closely how the new full action changes that. That needs to be considered certainly.

soliton- added a commit that referenced this issue Sep 5, 2019
Now, if no action is provided and the current side's controller or proxy controller is the AI, the side is changed to fully human control (same as `:droid <side> off`). If no action is provided and neither the controller or proxy controller of the current side are controlled by the AI, then the controller is set to human and the proxy controller set to AI(same as `:droid <side> on`).

Fixes #4308
@vgaming

This comment has been minimized.

Copy link
Member Author

@vgaming vgaming commented Sep 5, 2019

Thanks for fixing! Code-wise, seems to be good. I'll check in-game if another RC release will be made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.