Lua API reorganization

Discussion of Lua and LuaWML support, development, and ideas.

Moderator: Forum Moderators

DeFender1031
Posts: 11
Joined: March 19th, 2016, 8:29 pm

Lua API reorganization

Post by DeFender1031 »

Hi all. There has been some discussion in the development IRC channel as of late of reorganizing the Lua libraries into a more coherent, consistent, and organized API. To that end, I have created a proposal on the wiki for how such a reorganization might look. Also note that to ensure backwards compatibility, the plan would be to keep all existing access points for the Lua libraries as deprecated aliases to their reorganized locations, pending final removal at some unspecified later date once there has been sufficient time for code to be updated. Before any decisions are made though, I and others from the development channel thought that it would be a good idea to open the proposal to a wider audience to get feedback and make sure we're doing this right, as the primary purpose of reorganizing is to make it easier for the content creators using the API. To that end, any comments, questions, alternative ideas, changes, dissenting opinions, etc. should be discussed here, and we will try to keep the proposal up-to-date with the latest points of discussion. So, my fellow Wesnothians, give it a read, and let me know what you think, and more importantly, what you would change.
User avatar
doofus-01
Art Director
Posts: 4122
Joined: January 6th, 2008, 9:27 pm
Location: USA

Re: Lua API reorganization

Post by doofus-01 »

Speaking as someone who goes months at a time without thinking about Lua, and then struggles to remember what the hell is going on, I'll say your proposal doesn't seem bad, but it really depends upon the documentation. So, if you are thorough in documentation, you've got my approval, for what little that is worth.
BfW 1.12 supported, but active development only for BfW 1.13/1.14: Bad Moon Rising | Trinity | Archaic Era |
| Abandoned: Tales of the Setting Sun
GitHub link for these projects
gfgtdf
Developer
Posts: 1432
Joined: February 10th, 2013, 2:25 pm

Re: Lua API reorganization

Post by gfgtdf »

First i not convinced that its worth the breakage (that such changes imples, unless 'pending final removal at some unspecified later date once there has been sufficient time for code to be updated' means removal never happens, since there are always addons that are not activaley maintained and will never get udpates of this scale).

Some issues on the poposal in detail:

helper.all_teams was a helper function form when wesnoth.sides was not avaiable yet, so no reason to have it in a 'new api'.

Similar goes for the unit functions like wesnoth.unit.add_modification, they are already avaiable as unit:add_modification(), so i don't think we need to need to keep the in the new wesnoth.unit. api it when you want to deprecate the old name function anyways

wesnoth.dofile replaces luas standard dofile function, so renaming it woudl only be more confusing.

Lastly there are change that make the name sureley worse than before in my opinion, in particular wesnoth.sides -> wesnoth.side.proxy (this sounds like proxy is some proxy object but actually it is a normal array), wesnoth.get_sides -> wesnoth.side.get (this soudns like it would just return one single side) and wesnoth.synchronize_choice -> wesnoth.game.synchronize (point of this function is to sync user choices done by one single user, also note that there is another function wesnoth.synchronize_choices that syncs multiple choices)
Scenario with Robots SP scenario (1.11/1.12), allows you to build your units with components, PYR No preperation turn 1.12 mp-mod that allows you to select your units immideately after the game begins.
User avatar
Pentarctagon
Project Manager
Posts: 5530
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Lua API reorganization

Post by Pentarctagon »

To go the other direction as gfgtdf, I completely support reorganizing/renaming things to make things clearer, as long as "pending final removal at some unspecified later date once there has been sufficient time for code to be updated" becomes a specific release. Because the only thing worse than a poorly organized api, is keeping around two apis for the same thing - one of which is still poorly organized - especially when it's literally just renaming stuff.

Perhaps another python tool could be written to automate the renaming, assuming this wouldn't belong in the existing wmllint?
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
User avatar
Celtic_Minstrel
Developer
Posts: 2166
Joined: August 3rd, 2012, 11:26 pm
Location: Canada
Contact:

Re: Lua API reorganization

Post by Celtic_Minstrel »

gfgtdf wrote:First i not convinced that its worth the breakage (that such changes imples, unless 'pending final removal at some unspecified later date once there has been sufficient time for code to be updated' means removal never happens, since there are always addons that are not activaley maintained and will never get udpates of this scale).
We really can't be worrying about addons that are not actively maintained. If someone really wants to play such an addon, and they are unable to update it themselves, they can probably download an older version of Wesnoth and play it there.
gfgtdf wrote:Similar goes for the unit functions like wesnoth.unit.add_modification, they are already avaiable as unit:add_modification(), so i don't think we need to need to keep the in the new wesnoth.unit. api it when you want to deprecate the old name function anyways
Oh yeah, I was thinking about this too but had forgotten by the time he presented this list. Some of the unit functions definitely need to stay though - at least create_unit, possibly others.
gfgtdf wrote:wesnoth.dofile replaces luas standard dofile function, so renaming it woudl only be more confusing.
I was thinking about that too, but I think it's not really that important. It replaces Lua's standard dofile, sure, but it's not a drop-in replacement for it.
gfgtdf wrote:wesnoth.sides -> wesnoth.side.proxy (this sounds like proxy is some proxy object but actually it is a normal array),
In fact, it's literally populated by wesnoth.sides = wesnoth.get_sides{} if I recall correctly.
gfgtdf wrote:wesnoth.synchronize_choice -> wesnoth.game.synchronize (point of this function is to sync user choices done by one single user, also note that there is another function wesnoth.synchronize_choices that syncs multiple choices)
I'm not quite sure I understand what's worse about this.
Pentarctagon wrote:Perhaps another python tool could be written to automate the renaming, assuming this wouldn't belong in the existing wmllint?
To me it doesn't seem like something for wmllint (since this is Lua, not WML), but I dunno. It'd mostly be simple find-and-replace, though if someone does something like local W = wesnoth then there could be problems.
Author of The Black Cross of Aleron campaign and Default++ era.
Former maintainer of Steelhive.
DeFender1031
Posts: 11
Joined: March 19th, 2016, 8:29 pm

Re: Lua API reorganization

Post by DeFender1031 »

gfgtdf wrote:First i not convinced that its worth the breakage (that such changes imples, unless 'pending final removal at some unspecified later date once there has been sufficient time for code to be updated' means removal never happens, since there are always addons that are not activaley maintained and will never get udpates of this scale).
I agree with celmin, there are plenty of other changes that are going to be breaking unmaintained add-ons. If we're going to worry about that, we're never going to make any progress on anything. A significant deprecation period should be more than enough for this.
gfgtdf wrote:helper.all_teams was a helper function form when wesnoth.sides was not avaiable yet, so no reason to have it in a 'new api'.
The scope of the reorg did not include adding/removing/modifying existing stuff. We can certainly expand the scope if we'd like, and if so, we'll update accordingly. I also think that having a side iterator could still be useful, though I suppose one could get the same effect with ipairs() on the sides array.
gfgtdf wrote:Similar goes for the unit functions like wesnoth.unit.add_modification, they are already avaiable as unit:add_modification(), so i don't think we need to need to keep the in the new wesnoth.unit. api it when you want to deprecate the old name function anyways
Same as above, with an additional talking point that some people may prefer the . syntax to the : syntax. Do we care about such people? I dunno, we should discuss that.
gfgtdf wrote:wesnoth.dofile replaces luas standard dofile function, so renaming it woudl only be more confusing.
Did not know that. I'm on the fence on this one, as on the one hand, celmin's right, it's not a drop-in replacement, and further, "dofile" is a dumb name (IMO), but on the other, if it's the standard name in lua, might be best not to mess with it. This requires further discussion.
gfgtdf wrote:wesnoth.sides -> wesnoth.side.proxy (this sounds like proxy is some proxy object but actually it is a normal array)
But modifying the properties of a particular side actually changes those values in-game. That makes it a proxy, no?
gfgtdf wrote:wesnoth.get_sides -> wesnoth.side.get (this soudns like it would just return one single side)
You are correct, that's a typo which I will correct shortly. That should be get_all.
gfgtdf wrote:In fact, it's literally populated by wesnoth.sides = wesnoth.get_sides{} if I recall correctly.
Then do we even NEED get_sides at all? (Assuming we're expanding the scope of the reorg to dropping redundant functions.)
gfgtdf wrote:wesnoth.synchronize_choice -> wesnoth.game.synchronize (point of this function is to sync user choices done by one single user
Functions should be named for what they do, not for their primary use-case. I've seen documentation all over the wiki talking about how all sorts of stuff that has nothing to do with choices is only safe to use inside a synchronize_choice. To me, that screams of a misnamed function. It synchronizes nonsynched game data, not just choices.
gfgtdf wrote:also note that there is another function wesnoth.synchronize_choices that syncs multiple choices)
That's not an issue, based on the naming schemes in the proposed reorg, that would naturally end up named wesnoth.game.synchronize_all
Pentarctagon wrote:the only thing worse than a poorly organized api, is keeping around two apis for the same thing - one of which is still poorly organized
Agreed.
User avatar
Celtic_Minstrel
Developer
Posts: 2166
Joined: August 3rd, 2012, 11:26 pm
Location: Canada
Contact:

Re: Lua API reorganization

Post by Celtic_Minstrel »

DeFender1031 wrote:I also think that having a side iterator could still be useful, though I suppose one could get the same effect with ipairs() on the sides array.
I was going to say something similar, until I realized that ipairs() could easily get the same effect (though it's not quite a drop-in replacement).
DeFender1031 wrote:
gfgtdf wrote:wesnoth.sides -> wesnoth.side.proxy (this sounds like proxy is some proxy object but actually it is a normal array)
But modifying the properties of a particular side actually changes those values in-game. That makes it a proxy, no?
Well, yes, but wesnoth.sides an array of those side proxies - just a normal table.

That said, ideally everything representing game data should be a proxy. So I wonder if we should stop using the word proxy to describe them...
DeFender1031 wrote:
gfgtdf wrote:In fact, it's literally populated by wesnoth.sides = wesnoth.get_sides{} if I recall correctly.
Then do we even NEED get_sides at all? (Assuming we're expanding the scope of the reorg to dropping redundant functions.)
Yes, because wesnoth.get_sides executes a Standard Side Filter and returns a list of the matching sides. That's why my illustrative code passed an empty table for the SSF. (It's illustrative because it's actually done with the C API rather than by executing Lua code.)
Author of The Black Cross of Aleron campaign and Default++ era.
Former maintainer of Steelhive.
DeFender1031
Posts: 11
Joined: March 19th, 2016, 8:29 pm

Re: Lua API reorganization

Post by DeFender1031 »

Celtic_Minstrel wrote:
DeFender1031 wrote:But modifying the properties of a particular side actually changes those values in-game. That makes it a proxy, no?
Well, yes, but wesnoth.sides an array of those side proxies - just a normal table.

That said, ideally everything representing game data should be a proxy. So I wonder if we should stop using the word proxy to describe them...
I'm open to a better name, side.all? I agree that everything should be proxies, but we still need some standard word in general for "wesnoth.category.main_proxy_to_whatever_this_category_is". "all" works for sides, but what happens when we have a proxy to some kind of game singleton... for example, imagine we added more to work with scenario data (something which is sorely lacking), we'd have functions like wesnoth.scenario.set_next and wesnoth.scenario.get_next, but we'd also have a proxy to the current scenario wesnoth.scenario....something? Though I suppose .current would make sense as the proxy for that. Still, the point remains that any name there would be highly subject to the particular case, and there can be cases where there's no good word. "state" maybe for a generic word? I dunno. We may also just want to only burn that bridge after we cross it and hope we keep finding appropriate words to use. For now though, does "all" work for the array of side proxies, or is there something else that would be better.
Celtic_Minstrel wrote:wesnoth.get_sides executes a Standard Side Filter and returns a list of the matching sides. That's why my illustrative code passed an empty table for the SSF. (It's illustrative because it's actually done with the C API rather than by executing Lua code.)
Right. Forgot. My bad.
Last edited by DeFender1031 on November 7th, 2016, 4:13 am, edited 1 time in total.
gfgtdf
Developer
Posts: 1432
Joined: February 10th, 2013, 2:25 pm

Re: Lua API reorganization

Post by gfgtdf »

DeFender1031 wrote:
gfgtdf wrote:First i not convinced that its worth the breakage (that such changes imples, unless 'pending final removal at some unspecified later date once there has been sufficient time for code to be updated' means removal never happens, since there are always addons that are not activaley maintained and will never get udpates of this scale).
I agree with celmin, there are plenty of other changes that are going to be breaking unmaintained add-ons. If we're going to worry about that, we're never going to make any progress on anything. A significant deprecation period should be more than enough for this.
But this is a very huge change, that breaks all addons that use lua and also requires a lot of work to update addons. An update like this is quite likeley to discourage addondevs from maintaining older addons.
DeFender1031 wrote:
Did not know that. I'm on the fence on this one, as on the one hand, celmin's right, it's not a drop-in replacement,
the onyl diference is that wesnoth.dofile paths are relative to wesnot/userdata dir to prevent poeple from reading wesnoth-unrelated files. wenoth.dofile, and wesnoth.require are in the same relation as standard lua dofile require: require caches the result while dofile does not.
DeFender1031 wrote: and further, "dofile" is a dumb name
This is just your presonal opinion.
DeFender1031 wrote:
gfgtdf wrote:wesnoth.synchronize_choice -> wesnoth.game.synchronize (point of this function is to sync user choices done by one single user
Functions should be named for what they do, not for their primary use-case. I've seen documentation all over the wiki talking about how all sorts of stuff that has nothing to do with choices is only safe to use inside a synchronize_choice. To me, that screams of a misnamed function. It synchronizes nonsynched game data, not just choices.
The name sync_choice puts stess of the fact that the engine asked one clients to calculate a specific value (choice). syncronize could mean anything, like issuesing synced commands form an unsynced context which is kinda the opposite of what this function does.
Scenario with Robots SP scenario (1.11/1.12), allows you to build your units with components, PYR No preperation turn 1.12 mp-mod that allows you to select your units immideately after the game begins.
DeFender1031
Posts: 11
Joined: March 19th, 2016, 8:29 pm

Re: Lua API reorganization

Post by DeFender1031 »

gfgtdf wrote: this is a very huge change, that breaks all addons that use lua and also requires a lot of work to update addons. An update like this is quite likeley to discourage addondevs from maintaining older addons.
I hear your point, however, a simple polyfill add-on could be created to set up the old aliases even after they're removed, and people can add it as a dependency. That doesn't have to be mainline. Such a lua file would necessarily be created for this switchover anyway, so even after final removal, we could simply add that lua file to the add-ons server and let anyone continue to use it. Not an issue.
gfgtdf wrote: the onyl diference is that wesnoth.dofile paths are relative to wesnot/userdata dir to prevent poeple from reading wesnoth-unrelated files. wenoth.dofile, and wesnoth.require are in the same relation as standard lua dofile require: require caches the result while dofile does not.
Agreed. I think it makes sense to stick with the standard name even if...
gfgtdf wrote:
DeFender1031 wrote: "dofile" is a dumb name
This is just your presonal opinion.
Yes it is just my personal opinion, and I'm sticking to it.
gfgtdf wrote:The name sync_choice puts stess of the fact that the engine asked one clients to calculate a specific value (choice). syncronize could mean anything, like issuesing synced commands form an unsynced context which is kinda the opposite of what this function does.
I hear your point. Then might I suggest "synchronize_value" and "synchronize_values" since it could be a value from anything, not just a choice?
User avatar
Celtic_Minstrel
Developer
Posts: 2166
Joined: August 3rd, 2012, 11:26 pm
Location: Canada
Contact:

Re: Lua API reorganization

Post by Celtic_Minstrel »

gfgtdf wrote:But this is a very huge change, that breaks all addons that use lua and also requires a lot of work to update addons. An update like this is quite likeley to discourage addondevs from maintaining older addons.
It doesn't break any addons, since the old versions will be kept around for quite awhile. I don't think it would be terribly hard to update older addons, either – it might even be possible to automate it.
gfgtdf wrote: the onyl diference is that wesnoth.dofile paths are relative to wesnot/userdata dir to prevent poeple from reading wesnoth-unrelated files. wenoth.dofile, and wesnoth.require are in the same relation as standard lua dofile require: require caches the result while dofile does not.
Actually, wesnoth.require is even more different from the built-in require than wesnoth.dofile is from the built-in dofile. Lua's require function doesn't take a file path as its argument. Wesnoth's does. So the relationship between Wesnoth's pair isn't quite the same as that between Lua's pair. (You could argue that there isn't really much of a relation between Lua's pair, but in Wesnoth there's a clear relation since they take the same type of argument and resolve the path in the same way.)
Author of The Black Cross of Aleron campaign and Default++ era.
Former maintainer of Steelhive.
User avatar
Celtic_Minstrel
Developer
Posts: 2166
Joined: August 3rd, 2012, 11:26 pm
Location: Canada
Contact:

Re: Lua API reorganization

Post by Celtic_Minstrel »

Considering starting a new global "wml" table which contains the things proposed for "wesnoth.wml" as well as a "variable" subtable with the things proposed for "wesnoth.variable"; any objections?

During transition I'm not sure whether I'd 1) duplicate these things in lua/core.lua (simply copy-paste the code from helper.lua), 2) have core.lua import and use helper.lua, or 3) move it to core.lua and have helper.lua use it. The third option seems ideal but has a few problems with respect to the special metatables - rather than offering "set_X_metatable" functions I'd rather just expose a table that already has that metatable.
Author of The Black Cross of Aleron campaign and Default++ era.
Former maintainer of Steelhive.
User avatar
Pentarctagon
Project Manager
Posts: 5530
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Lua API reorganization

Post by Pentarctagon »

No objections, but the current implementation doesn't quite work. wml.variable.get/wml.variable.set are non-functional - using either of them prints an error, such as:

Code: Select all

error scripting/lua: [string "..."]:2: attempt to call a nil value (field 'get')
stack traceback:
	[string "..."]:2: in local 'bytecode'
	lua/wml-tags.lua:265: in local 'cmd'
	lua/wml-utils.lua:145: in field 'handle_event_commands'
	lua/wml-flow.lua:6: in function <lua/wml-flow.lua:5>
The deprecation warnings also always display, whether in debug mode or not.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
User avatar
Sapient
Inactive Developer
Posts: 4453
Joined: November 26th, 2005, 7:41 am
Contact:

Re: Lua API reorganization

Post by Sapient »

Honest question: What's the big payoff from all these changes? In most cases, you'll be typing more characters. What's stopping anyone from organizing the existing content and documentation, to achieve similar gains with fewer pains? As far as items that need to be named more clearly, those could be considered on an individual basis, so I don't really see that as an argument in favor of total re-org. Maybe someone can explain the benefits more clearly.


[Edit: nevermind, I didn't realize this was already implemented. Feel free to disregard my post.]
http://www.wesnoth.org/wiki/User:Sapient... "Looks like your skills saved us again. Uh, well at least, they saved Soarin's apple pie."
User avatar
Pentarctagon
Project Manager
Posts: 5530
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Lua API reorganization

Post by Pentarctagon »

Also, since the change has already been made, is the rest of mainline going to use the non-deprecated API calls? Currently loading a map gives me the following in the log:

Code: Select all

20170526 01:20:33 warning wml: helper.set_wml_tag_metatable is deprecated; use wml.tag instead which has the metatable already set
20170526 01:20:33 warning wml: helper.shallow_literal is deprecated; use wml.shallow_literal instead
20170526 01:20:33 warning wml: helper.get_child is deprecated; use wml.get_child instead
20170526 01:20:33 warning wml: wesnoth.set_variable is deprecated; use wml.variable.set instead
20170526 01:20:33 warning wml: helper.child_range is deprecated; use wml.child_range instead
20170526 01:20:33 warning wml: helper.child_count is deprecated; use wml.child_count instead
20170526 01:20:33 warning wml: wesnoth.tovconfig is deprecated; use wml.tovconfig instead
20170526 01:20:33 warning wml: helper.get_variable_array is deprecated; use wml.variable.get_array instead
20170526 01:20:33 warning wml: wesnoth.get_variable is deprecated; use wml.variable.get instead
20170526 01:20:33 warning wml: helper.parsed is deprecated; use wml.parsed instead
20170526 01:20:33 warning wml: helper.literal is deprecated; use wml.literal instead
This is not caused by any add-on, as confirmed by me grepping for all of them in my add-on directory. Also also, shouldn't it be warning lua or something like that?
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
Post Reply