New lua proxy table "wesnoth.game_config.mp_settings". #87

Merged
merged 4 commits into from Dec 21, 2013

Conversation

Projects
None yet
4 participants
Member

cbeck88 commented Dec 5, 2013

Old title: New lua proxy table "wesnoth.mp_settings".

Hoping to fill a feature request which I made here:

https://gna.org/bugs/?21208

I introduce a lua proxy table to access all the fields that occur in "multiplayer" tag in save game files.

The lua expression "wesnoth.mp_settings.[field]" accesses any field of resources::state_of_game->mp_settings(), if the current game type is multiplayer. The name "field" is chosen according to mp_game_settings::to_config(), so that the field name matches exactly what occurs in save games files. The access should be read only, the same as wesnoth.current.

I don't currently handle vector objects like "active_mods", or the std::map "side_users"... active_mods could be a very useful thing to have access to, if someone could provide a suggestion how best to pass this to lua that would be great.

Side Remark: I noticed when doing this that in mp_game_settings.hpp, .cpp, the fields "share_map" and "share_view" are defined and I assume match those features in game, but they are not written by to_config() and hence not saved in the save file... is this a bug or is it actually intentional? If these are indeed used and should be going to the save file then I will indeed want to add lua hooks to them as well for consistency.

Member

cbeck88 commented Dec 5, 2013

By the way, I have tested this and the hooks seem to work as intended.

Edit: Realized it might be a good idea to allow access to state_of_game->classification().campaign_type...

Member

cbeck88 commented Dec 5, 2013

The most recent commit also adds "campaign_type" as a field to the proxy table "wesnoth.game_config". It refers to the string resources::state_of_game->classification().campaign_type. So users who wish to use mp_settings in a mod can check that campaign_type == "multiplayer" before reading 0's for all mp_settings fields.

Member

anonymissimus commented Dec 5, 2013

It seems to me these are too many seldomly used proxy fields. It should be possible to use mp_game_settings::to_config() and return_cfg_attrib in lua.cpp, similar to how it's done with the __cfg fields of lua proxy units. This should be a lot less (C++) code.
As for the side remark, that should go into a separate pull request then, in any case.

Member

cbeck88 commented Dec 5, 2013

Thanks for the tip, that's a lot better than what I did. I will test the new version now.

Member

cbeck88 commented Dec 6, 2013

New commits change it so that wesnoth.game_config.mp_settings is the new proxy table, and exists as a child of wesnoth.game_config only if wesnoth.game_config.campaign_type == "multiplayer". Works great in tests.

Member

cbeck88 commented Dec 6, 2013

I added a bunch of test scenarios for sp, mp, mp campaign, as a zipped add-on folder, to the bug report: https://gna.org/bugs/index.php?21208

It seems to behave as expected in each case.

Member

anonymissimus commented Dec 7, 2013

Can you please merge such commits like "fixed whitespace" into that commit which introduced the wrong whitespace in the first place. (I'm doing such things with git rebase -i for as long as the commit introducing the problem is still completely local, not yet pushed to a remote reporitory.)
Not sure about how it works out with an existing pull request (as opposed to creating a new one) though. One should not modify any commits which are already no longer completely local.

Owner

AI0867 commented Dec 7, 2013

If you force push to the branch you created this pull request from (cbeck88:new_lua_hooks), it will automatically be updated.

cbeck88 added some commits Dec 5, 2013

New lua metatable "wesnoth.mp_settings".
Follows pattern of previous lua hooks in src/scripting/lua.cpp.

I would like to be able to access all the fields that occur in
"multiplayer" tag in save game files.

Referring to gamestatus.cpp line 1208, which is what writes
the game state to a config for output, I check if

(game_state.classification().campaign_type=="multiplayer" ),

and if so I ask the mp_settings object for fields.

Referring to line 195 of mp_game_settings.cpp, I use the same names
as appear in save files for the lua hooks.

I don't currently handle vector objects like "active_mods".
enable wesnoth.mp_settings.active_mods and side_users
(using string join functions as to_config() does)
make campaign_type visible from wesnoth.game_config
fix int mp_countdown -> bool, oops
Member

cbeck88 commented Dec 8, 2013

I see, so this is a separate part of PR etiquette here? I had not used git rebase before this, github basically says "don't use this it's dangerous" https://help.github.com/articles/why-are-my-commits-in-the-wrong-order

But when I read some guides to git rebase, some of them point to this: http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html

I gather that we are much closer to the latter than the former? I cleaned up this PR and wesnoth#85.

moved mp_settings to be a (conditional) child of game_config,
using return_cfgref_attrib fcn.
works fine in testing, using path wesnoth.game_config.mp_settings
Owner

AI0867 commented Dec 8, 2013

Linus's explanation sounds pretty close.

If you develop some feature over 4 commits, then, with discussion, finetune it over 6 more commits, all of those 10 commits are useful history and valid intermediate points. We want those in our repository.
If you make some changes, back out of them, try a different way, back out of those and so on (obvious exaggeration of what happened here), the intermediate points are not really of any use to anyone, and will confuse people reading the repository logs. (and using Linus's terms, I consider a pull request to still be mostly your history)

@ghost ghost assigned Coffee-- Dec 21, 2013

Contributor

Coffee-- commented Dec 21, 2013

Considering this is something that I've also seen as missing for a while and it is really a small bit of code, I'm going to push changes.

Coffee-- added a commit that referenced this pull request Dec 21, 2013

Merge pull request #87 from cbeck88/new_lua_hooks
New lua proxy table "wesnoth.game_config.mp_settings".

@Coffee-- Coffee-- merged commit 31e9c4e into wesnoth:master Dec 21, 2013

1 check passed

default The Travis CI build passed
Details

@cbeck88 cbeck88 deleted the cbeck88:new_lua_hooks branch Dec 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment