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

[FR] Setting flags with Lua #4396

Closed
sevu opened this issue Sep 28, 2019 · 15 comments

Comments

@sevu
Copy link
Member

commented Sep 28, 2019

Flags can be read, but not changed in Lua, e.g. this isn't accepted: wesnoth.sides[1].flag="hjbd"
That's as documented in the wiki, but I guess it would be good to change flags without WML.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

I think there is a function for this although it has a rather confusing Name. But I agree that it should be added.

@jostephd jostephd closed this in 9bd717c Sep 29, 2019
@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

@jostephd

i think you have to call game_display_->reinit_flags_for_side(team_num); afterwards.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

reopening for now.

@gfgtdf gfgtdf reopened this Sep 29, 2019
@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

Thanks for catching that. That's what I get for testing the flag_icon setter but not the flag setter :(

I tried this patch:

patch
diff --git a/src/scripting/lua_team.cpp b/src/scripting/lua_team.cpp
index ac4693a29ff..f653ddf9fae 100644
--- a/src/scripting/lua_team.cpp
+++ b/src/scripting/lua_team.cpp
@@ -130,6 +130,12 @@ static int impl_side_set(lua_State *L)
 	team &t = luaW_checkteam(L, 1);
 	char const *m = luaL_checkstring(L, 2);
 
+	const auto& reinit_flag_for_team = [&L] (const team& t) -> void {
+		auto* disp = lua_kernel_base::get_lua_kernel<game_lua_kernel>(L).get_display();
+		if(disp) {
+			disp->reinit_flags_for_side(t.side());
+		}
+	};
 	// Find the corresponding attribute.
 	modify_int_attrib("gold", t.set_gold(value));
 	modify_tstring_attrib("objectives", t.set_objectives(value, true));
@@ -141,17 +147,17 @@ static int impl_side_set(lua_State *L)
 	modify_bool_attrib("objectives_changed", t.set_objectives_changed(value));
 	modify_bool_attrib("hidden", t.set_hidden(value));
 	modify_bool_attrib("scroll_to_leader", t.set_scroll_to_leader(value));
-	modify_string_attrib("flag", t.set_flag(value));
+	modify_string_attrib("flag", {
+		t.set_flag(value);
+		reinit_flag_for_team(t);
+	});
 	modify_string_attrib("flag_icon", t.set_flag_icon(value));
 	modify_tstring_attrib("user_team_name", t.change_team(t.team_name(), value));
 	modify_string_attrib("team_name", t.change_team(value, t.user_team_name()));
 	modify_string_attrib("controller", t.change_controller_by_wml(value));
 	modify_string_attrib("color", {
-		auto* disp = lua_kernel_base::get_lua_kernel<game_lua_kernel>(L).get_display();
 		t.set_color(value);
-		if(disp) {
-			disp->reinit_flags_for_side(t.side());
-		}
+		reinit_flag_for_team(t);
 	});
 	modify_string_attrib("defeat_condition", t.set_defeat_condition_string(value));
 	modify_int_attrib("carryover_percentage", t.set_carryover_percentage(value));
diff --git a/src/team.hpp b/src/team.hpp
index 909b44cd298..21e700bc6dc 100644
--- a/src/team.hpp
+++ b/src/team.hpp
@@ -254,6 +254,7 @@ public:
 
 	CONTROLLER controller() const { return info_.controller; }
 	const std::string& color() const { return info_.color; }
+	/// @note Call display::reinit_flag_for_team() after calling this
 	void set_color(const std::string& color) { info_.color = color; }
 	bool is_empty() const { return info_.controller == CONTROLLER::EMPTY; }
 
@@ -300,6 +301,7 @@ public:
 	const std::string& flag() const { return info_.flag; }
 	const std::string& flag_icon() const { return info_.flag_icon; }
 
+	/// @note Call display::reinit_flag_for_team() after calling this
 	void set_flag(const std::string& flag) { info_.flag = flag; }
 	void set_flag_icon(const std::string& flag_icon) { info_.flag_icon = flag_icon; }
 

With this patch, setting flag_icon and color works, but setting flag does not. The lambda is fine, since setting color works. What's missing?

I'll revert the setter in the meantime until it's fixed.

jostephd added a commit that referenced this issue Sep 29, 2019
It wasn't working, see #4396 (comment)

This partly reverts commit 9bd717c.
@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

With this patch, setting flag_icon and color works, but setting flag does not. The lambda is fine, since setting color works. What's missing?

i don't see anything wrong, maybe you did a mistake when testing?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

See my comment on 9bd717c for a probable explanation. Though I think @gfgtdf pretty much covered it as well.

More to the point, the premise of this issue report is wrong - it is already possible to change the flag in Lua. (Not sure about flag_icon but definitely flag can be changed.)

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

@CelticMinstrel Yes, I did test that setting flag_icon works. I said so in a previous comment. Could it work in some cases and not others, or something?

OK, so the setter for flag needs to do what the function you mentioned does, whatever that is.

Unrelated, I noticed that after doing wesnoth.set_terrain(10, 10, 'Ss'), the hex does get swamp stats but isn't redrawn. How to fix that? I've tried :refresh.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

Unrelated, I noticed that after doing wesnoth.set_terrain(10, 10, 'Ss'), the hex does get swamp stats but isn't redrawn. How to fix that? I've tried :refresh.

you call [redraw] iirc. wesnoth.set_terrain does nto do that after perofrmance reasons, but its called at then of every [event] iirc.

what the function you mentioned does, whatever that is.

is basically just calls reinit_flags_for_side, that's what you ahve already added

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

Well, what I mentioned in the comment applies to flag and color, not flag and flag_icon. So flag_icon being added as a modifiable attribute is fine.

I don't think we need setters for flag and color since the function exists, and having setters might mislead people into thinking it's simple write when in fact it's also invalidating a cache.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

i don't see anything wrong, maybe you did a mistake when testing?

No, I see the problem. set_side_id uses side-1, the C++ function I patched doesn't do the -1. For some reason setting the color does work despite that, but setting the flag doesn't. With the following patch, both set_side_id and assignments work to change both flag and color.

revised patch
diff --git a/src/scripting/lua_team.cpp b/src/scripting/lua_team.cpp
index 1d9ee1685ef..f6a4fa5d191 100644
--- a/src/scripting/lua_team.cpp
+++ b/src/scripting/lua_team.cpp
@@ -130,6 +130,12 @@ static int impl_side_set(lua_State *L)
 	team &t = luaW_checkteam(L, 1);
 	char const *m = luaL_checkstring(L, 2);
 
+	const auto& reinit_flag_for_team = [&L] (const team& t) -> void {
+		auto* disp = lua_kernel_base::get_lua_kernel<game_lua_kernel>(L).get_display();
+		if(disp) {
+			disp->reinit_flags_for_side(t.side() - 1);
+		}
+	};
 	// Find the corresponding attribute.
 	modify_int_attrib("gold", t.set_gold(value));
 	modify_tstring_attrib("objectives", t.set_objectives(value, true));
@@ -141,16 +147,17 @@ static int impl_side_set(lua_State *L)
 	modify_bool_attrib("objectives_changed", t.set_objectives_changed(value));
 	modify_bool_attrib("hidden", t.set_hidden(value));
 	modify_bool_attrib("scroll_to_leader", t.set_scroll_to_leader(value));
+	modify_string_attrib("flag", {
+		t.set_flag(value);
+		reinit_flag_for_team(t);
+	});
 	modify_string_attrib("flag_icon", t.set_flag_icon(value));
 	modify_tstring_attrib("user_team_name", t.change_team(t.team_name(), value));
 	modify_string_attrib("team_name", t.change_team(value, t.user_team_name()));
 	modify_string_attrib("controller", t.change_controller_by_wml(value));
 	modify_string_attrib("color", {
-		auto* disp = lua_kernel_base::get_lua_kernel<game_lua_kernel>(L).get_display();
 		t.set_color(value);
-		if(disp) {
-			disp->reinit_flags_for_side(t.side());
-		}
+		reinit_flag_for_team(t);
 	});
 	modify_string_attrib("defeat_condition", t.set_defeat_condition_string(value));
 	modify_int_attrib("carryover_percentage", t.set_carryover_percentage(value));
diff --git a/src/team.hpp b/src/team.hpp
index 909b44cd298..21e700bc6dc 100644
--- a/src/team.hpp
+++ b/src/team.hpp
@@ -254,6 +254,7 @@ public:
 
 	CONTROLLER controller() const { return info_.controller; }
 	const std::string& color() const { return info_.color; }
+	/// @note Call display::reinit_flag_for_team() after calling this
 	void set_color(const std::string& color) { info_.color = color; }
 	bool is_empty() const { return info_.controller == CONTROLLER::EMPTY; }
 
@@ -300,6 +301,7 @@ public:
 	const std::string& flag() const { return info_.flag; }
 	const std::string& flag_icon() const { return info_.flag_icon; }
 
+	/// @note Call display::reinit_flag_for_team() after calling this
 	void set_flag(const std::string& flag) { info_.flag = flag; }
 	void set_flag_icon(const std::string& flag_icon) { info_.flag_icon = flag_icon; }
 

I don't think we need setters for flag and color since the function exists, and having setters might mislead people into thinking it's simple write when in fact it's also invalidating a cache.

We already have a setter for color, so we can either add a setter for flag too for consistency, or deprecate the color setter at level 1. I don't have an opinion as to which course of action to take. I assume we should at least add the missing -1 to the color setter even if we don't add a flag setter.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

set_side_id uses side-1, the C++ function I patched doesn't do the -1

By the way, can we come up with a way to make this sort of bug impossible?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

I recall making a pass for that purpose, though it likely never got merged... the basic approach would be to not directly expose the sides array anywhere, so that the exposed API is exclusively 1-based. Then you only have to worry about the -1 in functions internal to... I think it was the model? The game map or something?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

revised patch

I think i'd also prefer if instead reinit_flags_for_side was changed to use side number of instead of indicies to the array.

We already have a setter for color, so we can either add a setter for flag too for consistency, or deprecate the color setter at level 1

I still say the setts are nice to have, celmins main argument, ('one does not expect a real computation form a setter' if i understood correctly) is for this case not really relevant here since one usually does not set flag/color in a loop such that performance here would matter. Also if we even come to the situation where it would matter we can probably change the implementation to a laze calculation of that image where these setts would become simple.

@jostephd jostephd self-assigned this Sep 30, 2019
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

I think i'd also prefer if instead reinit_flags_for_side was changed to use side number of instead of indicies to the array.

It absolutely should be, but that's not really in scope for the PR.

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

It absolutely should be, but that's not really in scope for the PR.

Agreed.

What about the other questions, though? As I wrote:

We already have a setter for color, so we can either add a setter for flag too for consistency, or deprecate the color setter at level 1. I don't have an opinion as to which course of action to take. I assume we should at least add the missing -1 to the color setter even if we don't add a flag setter.

In further consideration I think I prefer to add a setter for flag, @gfgtdf's argument convinced me.

@jostephd jostephd closed this in 51c6559 Oct 14, 2019
jostephd added a commit that referenced this issue Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.