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 address hidden>: > > 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 address hidden> > Unsubscribe : https://launchpad.net/~widelands-dev > More help : https://help.launchpad.net/ListHelp
« Back to merge proposal
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 address hidden>: bazaar. launchpad. net/~widelands- dev/widelands/ one_tribe/ revision/ 7452#tribes/ init.lua bazaar. launchpad. net/~widelands- dev/widelands/ one_tribe/ revision/ 7453#world/ init.lua /code.launchpad .net/~widelands -dev/widelands/ one_tribe/ +merge/ 274832 _______ _______ _______ _______ _______ _____ /launchpad. net/~widelands- dev /launchpad. net/~widelands- dev /help.launchpad .net/ListHelp
>
> 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://
>
> 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://
>
> 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:/
> Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/one_tribe into lp:widelands.
>
> _______
> Mailing list: https:/
> Post to : <email address hidden>
> Unsubscribe : https:/
> More help : https:/