Lua API reorganization
Moderator: Forum Moderators
-
- Posts: 11
- Joined: March 19th, 2016, 8:29 pm
Lua API reorganization
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.
Re: Lua API reorganization
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
| Abandoned: Tales of the Setting Sun
GitHub link for these projects
Re: Lua API reorganization
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:
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
Lastly there are change that make the name sureley worse than before in my opinion, in particular
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.
- Pentarctagon
- Project Manager
- Posts: 5592
- Joined: March 22nd, 2009, 10:50 pm
- Location: Earth (occasionally)
Re: Lua API reorganization
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?
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
take one down, patch it around
-2,147,483,648 little bugs in the code
- Celtic_Minstrel
- Developer
- Posts: 2274
- Joined: August 3rd, 2012, 11:26 pm
- Location: Canada
- Contact:
Re: Lua API reorganization
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: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).
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 leastgfgtdf 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
create_unit
, possibly others.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.dofile
replaces luas standard dofile function, so renaming it woudl only be more confusing.
In fact, it's literally populated bygfgtdf wrote:wesnoth.sides
->wesnoth.side.proxy
(this sounds like proxy is some proxy object but actually it is a normal array),
wesnoth.sides = wesnoth.get_sides{}
if I recall correctly.I'm not quite sure I understand what's worse about this.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 functionwesnoth.synchronize_choices
that syncs multiple choices)
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 likePentarctagon wrote:Perhaps another python tool could be written to automate the renaming, assuming this wouldn't belong in the existing wmllint?
local W = wesnoth
then there could be problems.-
- Posts: 11
- Joined: March 19th, 2016, 8:29 pm
Re: Lua API reorganization
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: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).
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 withgfgtdf 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'.
ipairs()
on the sides array.Same as above, with an additional talking point that some people may prefer thegfgtdf 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
.
syntax to the :
syntax. Do we care about such people? I dunno, we should discuss that.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.dofile
replaces luas standard dofile function, so renaming it woudl only be more confusing.
But modifying the properties of a particular side actually changes those values in-game. That makes it a proxy, no?gfgtdf wrote:wesnoth.sides
->wesnoth.side.proxy
(this sounds like proxy is some proxy object but actually it is a normal array)
You are correct, that's a typo which I will correct shortly. That should begfgtdf wrote:wesnoth.get_sides
->wesnoth.side.get
(this soudns like it would just return one single side)
get_all
.Then do we even NEEDgfgtdf wrote:In fact, it's literally populated bywesnoth.sides = wesnoth.get_sides{}
if I recall correctly.
get_sides
at all? (Assuming we're expanding the scope of the reorg to dropping redundant functions.)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:wesnoth.synchronize_choice
->wesnoth.game.synchronize
(point of this function is to sync user choices done by one single user
That's not an issue, based on the naming schemes in the proposed reorg, that would naturally end up namedgfgtdf wrote:also note that there is another functionwesnoth.synchronize_choices
that syncs multiple choices)
wesnoth.game.synchronize_all
Agreed.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
- Celtic_Minstrel
- Developer
- Posts: 2274
- Joined: August 3rd, 2012, 11:26 pm
- Location: Canada
- Contact:
Re: Lua API reorganization
I was going to say something similar, until I realized thatDeFender1031 wrote:I also think that having a side iterator could still be useful, though I suppose one could get the same effect withipairs()
on the sides array.
ipairs()
could easily get the same effect (though it's not quite a drop-in replacement).Well, yes, butDeFender1031 wrote:But modifying the properties of a particular side actually changes those values in-game. That makes it a proxy, no?gfgtdf wrote:wesnoth.sides
->wesnoth.side.proxy
(this sounds like proxy is some proxy object but actually it is a normal array)
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...
Yes, becauseDeFender1031 wrote:Then do we even NEEDgfgtdf wrote:In fact, it's literally populated bywesnoth.sides = wesnoth.get_sides{}
if I recall correctly.get_sides
at all? (Assuming we're expanding the scope of the reorg to dropping redundant functions.)
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.)-
- Posts: 11
- Joined: March 19th, 2016, 8:29 pm
Re: Lua API reorganization
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 likeCeltic_Minstrel wrote:Well, yes, butDeFender1031 wrote:But modifying the properties of a particular side actually changes those values in-game. That makes it a proxy, no?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...
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.Right. Forgot. My bad.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.)
Last edited by DeFender1031 on November 7th, 2016, 4:13 am, edited 1 time in total.
Re: Lua API reorganization
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: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: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).
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:
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,
This is just your presonal opinion.DeFender1031 wrote: and further, "dofile" is a dumb name
The name sync_choice puts stess of the fact that the engine asked one clients to calculate a specific value (choice).DeFender1031 wrote: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:wesnoth.synchronize_choice
->wesnoth.game.synchronize
(point of this function is to sync user choices done by one single user
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.
-
- Posts: 11
- Joined: March 19th, 2016, 8:29 pm
Re: Lua API reorganization
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: 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.
Agreed. I think it makes sense to stick with the standard name even if...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.
Yes it is just my personal opinion, and I'm sticking to it.gfgtdf wrote:This is just your presonal opinion.DeFender1031 wrote: "dofile" is a dumb name
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?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.
- Celtic_Minstrel
- Developer
- Posts: 2274
- Joined: August 3rd, 2012, 11:26 pm
- Location: Canada
- Contact:
Re: Lua API reorganization
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: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.
Actually,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.
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.)- Celtic_Minstrel
- Developer
- Posts: 2274
- Joined: August 3rd, 2012, 11:26 pm
- Location: Canada
- Contact:
Re: Lua API reorganization
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.
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.
- Pentarctagon
- Project Manager
- Posts: 5592
- Joined: March 22nd, 2009, 10:50 pm
- Location: Earth (occasionally)
Re: Lua API reorganization
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:
The deprecation warnings also always display, whether in debug mode or not.
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>
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
take one down, patch it around
-2,147,483,648 little bugs in the code
Re: Lua API reorganization
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.]
[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."
- Pentarctagon
- Project Manager
- Posts: 5592
- Joined: March 22nd, 2009, 10:50 pm
- Location: Earth (occasionally)
Re: Lua API reorganization
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:
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
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
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
take one down, patch it around
-2,147,483,648 little bugs in the code