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 <[email protected]>: > > 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 : [email protected] > 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. _______________________________________________ Mailing list: https://launchpad.net/~widelands-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
