move the carryover gold handling and dialog to lua #7610

gfgtdf · 2023-05-08T13:24:11Z

This is still a wip, in particular im not yet sure what exactly the api for replacing it should be

CelticMinstrel · 2023-05-08T13:31:15Z

data/test/scenarios/wml_tests/ScenarioWML/SideWML/test_no_sides.cfg

@@ -0,0 +1,17 @@
+#textdomain wesnoth-test
+# This series of tests checks that scenarios are ending as appropriate.


The comment seems inaccurate

CelticMinstrel · 2023-05-08T17:56:10Z

src/scripting/game_lua_kernel.cpp

@@ -1704,6 +1704,7 @@ int game_lua_kernel::impl_current_get(lua_State *L)
 	return_int_attrib("turn", play_controller_.turn());
 	return_string_attrib("synced_state", synced_state());
 	return_bool_attrib("user_can_invoke_commands", !events::commands_disabled && gamedata().phase() == game_data::TURN_PLAYING);
+	return_bool_attrib("is_replaying", play_controller_.is_replay());


The convention for attributes of wesnoth.current is that reading it out makes sense as a phrase – "current map", "current schedule", "current event context", "current user can invoke commands", etc. This one doesn't fit that convention – "current is replaying" doesn't make sense.

Maybe user_is_replaying?

i can also move it somehere else, i wouldn't know where though. I'm not convinced that user_is_replayingis a better name

Hmm… wesnoth.scenario.is_replaying doesn't really make sense… maybe you could put it inwesnoth.interface as a function…

This still needs to be addressed.

Slayer95 · 2023-05-09T07:38:47Z

The default do_carryover_gold() function needs to be split in 4 functions:

Calculation of gold bonus. It should return (*) a table that, for each side, contains:

A boolean signaling whether they get a bonus.
Total gold bonus.
Gold bonus per turn.

Gold increase. It should accept the table from (1) and make it effective.
Calculation of bonus message. It should accept the table from (1) (or a side subtable?), and return the customizable parts as a table.
Display of bonus message. It should accept the table from (3) and show it.

Using events for carryover looks good. However, putting the entire do_carryover_gold() main routine as an event handler may not be that good (**). Instead, I wonder if it would be a palatable approach to fire events as the first instruction in (1), (2), (3), (4).

These events can then be handled by user-made code that mimicks the respective APIs of the default functions, and that calls wesnoth.cancel_action(). Then, if I am reading this correctly, the return value of fire() should be a tuple (***), and its second element should tell the main routine whether the event handlers called cancel_action.

(*) By return I mean setting these values as attributes in a public table or push them to the Lua stack. These side channels need to be used so that custom event handlers can actually set them.
(**) If the cancel_action approach is followed and the main routine were an event handler, then the main routine would seemingly share the same pump instance with its subevents, rendering the modularization useless.
(***) At least from the C++ side. That information doesn't seem to be pushed to the Lua stack at game_lua_kernel::intf_fire_event -but it should.

Slayer95 · 2023-05-09T07:38:53Z

Another event-based approach would rely on the other API mentioned in #7582 : aborting subsequent event handlers to the current event (maybe wesnoth.game_events.extinguish()). Then, it could be used for all of 1, 2, 3, 4, and even the main do_carryover_gold() routine itself were it also an event handler (as is in the current state of the PR.)

Either way, it seems the event system needs further improvements to be usable.

gfgtdf · 2023-05-09T15:09:29Z

The default do_carryover_gold() function needs to be split in 4 functions ... wonder if it would be a palatable approach to fire events ...

While i agree that parts of the code could be designed to be a bit more reusable, i also don't think its worth to write a event based framework here, in particular if it makes it harder for the main usecase i had in mind when writing this, which are campaigns that want to completely replace the carryover gold mechanic, (no gold carried over at all, and probably reward quick finishes in another way (maybe items or free exp), or campaigns that have a score system)

Slayer95 · 2023-05-09T15:39:52Z

don't think its worth to write a event based framework here

Understandable.

Not sure what sort of API you have in mind. However, a baseline approach could be to make do_carryover_gold() call scenario.do_carryover_gold if defined, else
do_carryover_gold_default().

gfgtdf · 2023-05-09T16:14:35Z

That similar to what i had in mind, i'm just unsure whether to put it, also i'm wondering whether i should make the helper functions available too, on the one hand i think they could be helpful, on he other hand making it an api would restrict us when changing their bahaviour in the future.

CelticMinstrel · 2023-05-09T17:40:12Z

data/lua/carryover_gold.lua

+	id = "carryover_gold",
+	first_time_only = false,
+	--priority = -1000,
+	action = function()


Just action = do_carryover_gold should suffice, right?

i was considering making the variable do_carryover_gold non local (not sure where to put it yet), so that it could be replaced by umc. That's why i added this indirection

Hmm… someone wanting to replace it entirely could just unregister this event. I guess what you say makes sense if they want to mildly alter the behaviour, maybe…

hmm i also wasn't sure how cleanly unregistering an event handler that was set in preload (and by preload i also mean toplevel lua) works, but i guess you just have to unregister them at preload aswell.

gfgtdf · 2023-05-10T01:32:03Z

So i split the function into smaller parts, since the parts are then kinda public api, we now have to worry about making them pretty.

CelticMinstrel · 2023-05-10T02:59:31Z

data/lua/scenario_end_events.lua

@@ -0,0 +1,11 @@
+carryover = wesnoth.require("carryover_gold.lua")


This should be local.

CelticMinstrel · 2023-05-10T03:01:06Z

data/lua/carryover_gold.lua

@@ -47,83 +52,108 @@ local function get_popup_text_basic()
 	return title, report
 end

-local function do_carryover_gold()
+--- Calculates the amount of carryover gold.


Please make these documentation comments conform to the format of the documentation comments in other modules, for example location_set or functional. That means using @param and @return annotations in addition to 3 dashes for the brief description as you've done here.

CelticMinstrel · 2023-05-10T03:02:30Z

data/lua/carryover_gold.lua

 	return #(wesnoth.map.find{ gives_income = true })
 end

-local function half_signed_value(num)
+--- Like tostring(num) but uses a prettier minus sign, to be used in the UI.


This is clearly not true, it's just returning tostring(num) directly.

Also, even if it were changed to be true, this seems like something that probably belongs in either the mathx or stringx module, not in here.

Tbh i don't even know why it's called that i just copied the name from the c++ function, but yeah the implementation is not there yet, idk whether it'd be really useful another places or whether other places usually want to use the normal minus sign.

If it's a C++ function, we could just make this a thin wrapper around the C++ function. We should at least consider changing the name though, as it doesn't make sense to me.

I also think it should return a t_string, rather than a regular Lua string.

This still applies now.

stevecotton · 2023-05-16T10:16:00Z

data/lua/carryover_gold.lua

+
+		else
+			goldmsg = _("You will start the next scenario with the defined minimum starting gold.",
+				"You will start the next scenario with the defined minimum starting gold.", side.carryover_gold)


This should be a normal string, not a plural.

I know it's copying the C++, it's a bug there too.

Why do you think so?

Oh right now i see it

The idea is to make it easier for umc devs to implement their own carryover / early finish bonus mechanics. Maybe we can also make use of it in Worls Conquest.

And use it in carryover_gold.lua

this way the functions can easily be reused for alternative carryover implementations.

We now use a modified default carryover calculation, the main advantage is it now uses the normal carryover message on endlevel.

gfgtdf · 2024-02-23T00:56:58Z

Now that 1.18 is branched of i'll probably merge this the next days

CelticMinstrel · 2024-02-23T01:24:42Z

data/lua/carryover_gold.lua

+
+--- Returns true iff there is not a single side that is controlled by the local human player.
+---@return boolean
+function carryover.get_is_observer()


This seems like something that should be moved into wesnoth.interface.

CelticMinstrel · 2024-02-23T01:25:21Z

data/lua/carryover_gold.lua

+end
+
+--- Show names when thre are multiple persistent teams.
+---@return integer


Please add a comment explaining the return value, like this:

---@return integer #the number of things

Or change the main comment to include this information. (Speaking of which, the main comment seems to be incorrect.)

CelticMinstrel · 2024-02-23T01:27:24Z

data/lua/carryover_gold.lua

+--- Calculates the amount of carryover gold.
+---@param side side
+---@param turns_left integer
+---@return number (finishing_bonus), number (finishing_bonus_per_turn)


If you want to explain multiple return values, each one needs to be on its own line.

Suggested change

---@return number (finishing_bonus), number (finishing_bonus_per_turn)

---@return number #total finishing bonus

---@return number #finishing bonus per turn

CelticMinstrel · 2024-02-23T01:28:33Z

data/lua/carryover_gold.lua

+	return finishing_bonus, finishing_bonus_per_turn
+end
+
+---@return boolean


Please add a comment, either with # at the end of this line or with --- on a separate line above, that explains what get_report_visible does.

CelticMinstrel · 2024-02-23T01:28:43Z

data/lua/carryover_gold.lua

+function carryover.get_report_visible()
+	local is_observer = carryover.get_is_observer()
+	local is_replay = wesnoth.current.is_replaying
+	--return true


This probably needs to be deleted.

CelticMinstrel · 2024-02-23T01:29:12Z

data/lua/carryover_gold.lua

+	return wesnoth.scenario.end_level_data.carryover_report and (not is_observer) and (not is_replay)
+end
+
+--- Whether to show the sides names in the scenario end report.


Suggested change

--- Whether to show the sides names in the scenario end report.

--- Whether to show the sides' names in the scenario end report.

CelticMinstrel · 2024-02-23T01:29:38Z

data/lua/carryover_gold.lua

+
+
+--- returns a stub to show in the scenario end dialog.
+---@return string (title), string (report)


Suggested change

---@return string (title), string (report)

---@return string #title

---@return string #report

CelticMinstrel · 2024-02-23T01:31:51Z

data/lua/carryover_gold.lua

+
+-- returns the first part of the carryover mesage, explain remaining gold
+---@param side side
+---@param info table


Please specify the format of this table.

There are two ways to do this. You can do it inline like this:

{field1: type, field2: type}

Or on the lines above you can add a block of comments like this:

---@class some_descriptive_name_for_the_table ---@field field1 type explanation of this field ---@field field2 type explanation of this field

If there's no obvious name for the type, remaining_gold_options or remaining_gold_info should be fine.

Or, if this is no longer used, then it could also be removed. (Though maybe the point is that the function could be overridden with one that does use it?)

CelticMinstrel · 2024-02-23T01:32:49Z

data/lua/carryover_gold.lua

+end
+
+---@param side side
+---@param info table


Please specify the format of this table too. It looks like it has 3 optional fields? See my previous comment for how. Also, add a comment explaining what the function does.

CelticMinstrel · 2024-02-23T01:34:06Z

data/lua/carryover_gold.lua

+end
+
+---@param side side
+---@param info table


Please specify the format of this table. Also, add a comment explaining what the function does.

CelticMinstrel · 2024-02-23T01:35:04Z

data/lua/carryover_gold.lua

+	return title, report
+end
+
+-- returns the first part of the carryover mesage, explain remaining gold


This comment isn't descriptive enough. "explain remaining gold" isn't an explanation, it's just restating the function name.

CelticMinstrel · 2024-02-23T01:35:19Z

data/lua/carryover_gold.lua

+end
+
+---@param side side
+---@param info table


Please specify the format of this table. Also, add a comment explaining what the function does.

CelticMinstrel · 2024-02-23T01:35:38Z

data/lua/carryover_gold.lua

+end
+--- returns the last part of the gold report for a single side, describing how much gold is added in the next scenario.
+---@param side side
+---@param info table


Please specify the format of this table. Also, add a line break before this comment block.

CelticMinstrel · 2024-02-23T01:36:05Z

data/lua/carryover_gold.lua

+			side.carryover_gold)
+	end
+
+	-- xgettext:no-c-format


Is this actually needed? Does it even do anything?

pretty sure i coped it from the c++ code so not sure

If that's the case, remove it. xgetext is the tool we use to gather translatable strings from C++ files, but it's not used on Lua files.

CelticMinstrel · 2024-02-23T01:36:44Z

data/lua/carryover_gold.lua

+
+--- returns the default gold report for a single side.
+---@param side side
+---@param info table


Please specify the format of this table. Also, the comment could be improved.

CelticMinstrel · 2024-02-23T01:37:15Z

data/lua/carryover_gold.lua

+	return report
+end
+
+---@return boolean


Please add a comment indicating what this function does. I realize it may seem obvious from the name, but I'd like to make it explicit too.

CelticMinstrel · 2024-02-23T01:38:04Z

data/lua/carryover_gold.lua

+end
+
+---@return table
+function carryover.turns_left()


This says it returns a table but it actually returns an integer.

CelticMinstrel · 2024-02-23T01:38:14Z

data/lua/carryover_gold.lua

+	return math.max(0, wesnoth.scenario.turns - wesnoth.current.turn)
+end
+
+--- sets the sides carryover gold and returns information that is used in the report message


Suggested change

--- sets the sides carryover gold and returns information that is used in the report message

--- sets the side's carryover gold and returns information that is used in the report message

CelticMinstrel · 2024-02-23T01:38:33Z

data/lua/carryover_gold.lua

+
+--- sets the sides carryover gold and returns information that is used in the report message
+---@param side side
+---@return table


Please specify the format of this table.

CelticMinstrel · 2024-02-23T01:38:50Z

data/lua/carryover_gold.lua

+	}
+end
+
+function carryover.do_carryover_gold()


Why is this function undocumented?

CelticMinstrel · 2024-02-23T01:40:04Z

src/playsingle_controller.cpp

@@ -394,7 +394,7 @@ void playsingle_controller::do_end_level()
 	}

 	persist_.end_transaction();
-	carryover_show_gold(gamestate(), is_observer() || is_replay(), is_observer(), saved_game_.classification().is_test());
+	// carryover_show_gold(gamestate(), is_observer() || is_replay(), is_observer(), saved_game_.classification().is_test());


Please delete.

CelticMinstrel · 2024-02-23T01:42:14Z

carryover_show_gold.?pp have nothing left in them now, so please delete them. I realize that means editing the Xcode project file, but it's honestly not that hard to remove a file from it – just delete all lines mentioning the file.

Please fix at least everything I mentioned that's not related to documentation before merging. I'd prefer you to fix all the documentation issues too, though.

gfgtdf marked this pull request as draft May 8, 2023 13:24

github-actions bot added Lua API Issues with the Lua engine and API. Unit Tests Issues involving Wesnoth's unit test suite. labels May 8, 2023

CelticMinstrel reviewed May 8, 2023

View reviewed changes

gfgtdf force-pushed the lua_carryover branch 4 times, most recently from 66f98fa to 2adfb8d Compare May 8, 2023 17:23

CelticMinstrel reviewed May 8, 2023

View reviewed changes

gfgtdf force-pushed the lua_carryover branch 2 times, most recently from 01171b9 to 0d6d6ce Compare May 9, 2023 02:52

gfgtdf force-pushed the lua_carryover branch from 0d6d6ce to bc915b6 Compare May 9, 2023 15:22

CelticMinstrel reviewed May 9, 2023

View reviewed changes

CelticMinstrel reviewed May 10, 2023

View reviewed changes

stevecotton reviewed May 16, 2023

View reviewed changes

gfgtdf force-pushed the lua_carryover branch from 08c7516 to 8cea1b7 Compare September 4, 2023 18:30

github-actions bot removed the Unit Tests Issues involving Wesnoth's unit test suite. label Sep 4, 2023

gfgtdf force-pushed the lua_carryover branch from 8cea1b7 to 0e28580 Compare September 4, 2023 18:32

gfgtdf added 2 commits February 21, 2024 18:59

move the carryover gold handling and dialog to lua

ea1f5ac

The idea is to make it easier for umc devs to implement their own carryover / early finish bonus mechanics. Maybe we can also make use of it in Worls Conquest.

Add wesnoth.current.is_replaying

6230d89

And use it in carryover_gold.lua

gfgtdf force-pushed the lua_carryover branch from 0e28580 to 2401395 Compare February 21, 2024 17:59

github-actions bot added the Campaign WC The mainline World Conquest campaign label Feb 21, 2024

gfgtdf mentioned this pull request Feb 22, 2024

Wrong carryover gold when the scenario is ended in debug mode #8446

Open

gfgtdf added 2 commits February 23, 2024 01:53

split gold_carryover into smaller functions

3aa00fb

this way the functions can easily be reused for alternative carryover implementations.

Refactor WC carryover implementation.

0e592b9

We now use a modified default carryover calculation, the main advantage is it now uses the normal carryover message on endlevel.

gfgtdf force-pushed the lua_carryover branch from 2401395 to 0e592b9 Compare February 23, 2024 00:54

gfgtdf marked this pull request as ready for review February 23, 2024 00:55

CelticMinstrel reviewed Feb 23, 2024

View reviewed changes

data/lua/carryover_gold.lua

}

end

function carryover.do_carryover_gold()

Copy link

Member

CelticMinstrel Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this function undocumented?

CelticMinstrel reviewed Feb 23, 2024

View reviewed changes

		@@ -0,0 +1,17 @@
		#textdomain wesnoth-test
		# This series of tests checks that scenarios are ending as appropriate.

		@@ -0,0 +1,11 @@
		carryover = wesnoth.require("carryover_gold.lua")

	---@return number (finishing_bonus), number (finishing_bonus_per_turn)
	---@return number #total finishing bonus
	---@return number #finishing bonus per turn

	--- Whether to show the sides names in the scenario end report.
	--- Whether to show the sides' names in the scenario end report.



		--- returns a stub to show in the scenario end dialog.
		---@return string (title), string (report)

	---@return string (title), string (report)
	---@return string #title
	---@return string #report

	--- sets the sides carryover gold and returns information that is used in the report message
	--- sets the side's carryover gold and returns information that is used in the report message

move the carryover gold handling and dialog to lua #7610

Are you sure you want to change the base?

move the carryover gold handling and dialog to lua #7610

Conversation

gfgtdf commented May 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Slayer95 commented May 9, 2023 • edited

Slayer95 commented May 9, 2023 • edited

gfgtdf commented May 9, 2023

Slayer95 commented May 9, 2023

gfgtdf commented May 9, 2023

Choose a reason for hiding this comment

gfgtdf May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfgtdf commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CelticMinstrel May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevecotton May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfgtdf commented Feb 23, 2024

Choose a reason for hiding this comment

CelticMinstrel Feb 23, 2024 • edited

Choose a reason for hiding this comment

CelticMinstrel Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CelticMinstrel Feb 23, 2024 • edited

Choose a reason for hiding this comment

CelticMinstrel Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CelticMinstrel commented Feb 23, 2024

gfgtdf commented May 8, 2023 •

edited

Slayer95 commented May 9, 2023 •

edited

Slayer95 commented May 9, 2023 •

edited

gfgtdf May 9, 2023 •

edited

CelticMinstrel May 10, 2023 •

edited

stevecotton May 16, 2023 •

edited

CelticMinstrel Feb 23, 2024 •

edited

CelticMinstrel Feb 23, 2024 •

edited

CelticMinstrel Feb 23, 2024 •

edited

CelticMinstrel Feb 23, 2024 •

edited