widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Still working on the review. Have another long train ride today and hope to get it done  then. I'll get back to this thread then. 

In general I find very little to complain about in the code - really great job :) 

> Am 01.11.2015 um 11:01 schrieb GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>:
> 
> Some comments to the code review - posting them here rather than in the code in order not change the branch, in case you're still working on it:
> 
> Regarding http://bazaar.launchpad.net/~widelands-dev/widelands/one_tribe/revision/7452#tribes/init.lua
> 
> We do need the number glob for parsing the animations for efficiency reasons. Using regular expressions for collecting the image files is sloooooooow - increases loading time for the tribes by about factor 10. I tried both Lua list directory and C++ boost:regex. We could think about writing an auxiliary Lua script to do what NumberGlob does though. Since this won't affect savegame compatibility, I'd classify that as a TODO for a future branch.
> 
> Regarding the renaming in
> http://bazaar.launchpad.net/~widelands-dev/widelands/one_tribe/revision/7453#world/init.lua
> 
> This has consequences for the resource indicators, the legacy tables and the C++ code as well - I don't want to spend a day digging around and undoing this. Since we're renaming a lot of entities, to me it made sense to rename them all at once.
> 
> Love the new Lua cleanup script :)
> 
> 
> -- 
> https://code.launchpad.net/~widelands-dev/widelands/one_tribe/+merge/274832
> Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/one_tribe into lp:widelands.
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~widelands-dev
> Post to     : widelands-dev@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~widelands-dev
> More help   : https://help.launchpad.net/ListHelp

-- 
https://code.launchpad.net/~widelands-dev/widelands/one_tribe/+merge/274832
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/one_tribe into lp:widelands.