Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Review: Needs Fixing This needs trunk merging really bad. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Review: Resubmit I fixed the line and cleaned out the build directory and the segfault is gone. Thanks! -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
OK, next try, but I need somebody to help me hunt down a segfault. I have described the problem in my commit message http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1341081/revision/7121 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
GunChleoc, it failed to compile here: [ 82%] Building CXX object src/map_io/CMakeFiles/map_io.dir/widelands_map_object_packet.cc.o In file included from /var/widelands/BZR/bug-1341081/bug-1341081/src/map_io/widelands_map_object_packet.cc:22:0: /var/widelands/BZR/bug-1341081/bug-1341081/src/map_io/widelands_map_object_packet.cc: In member function ‘void Widelands::Map_Object_Packet::LoadFinish()’: /var/widelands/BZR/bug-1341081/bug-1341081/src/map_io/widelands_map_object_packet.cc:116:69: error: ‘const struct Widelands::Map_Object_Descr’ has no member named ‘c_str’ load_pointers for %s: %s, (*i.current)-get_object()-descr().c_str(), e.what()); ^ /var/widelands/BZR/bug-1341081/bug-1341081/src/base/wexception.h:60:57: note: in definition of macro ‘wexception’ #define wexception(...) _wexception(__FILE__, __LINE__, __VA_ARGS__) ^ src/map_io/CMakeFiles/map_io.dir/build.make:399: recipe for target 'src/map_io/CMakeFiles/map_io.dir/widelands_map_object_packet.cc.o' failed make[2]: *** [src/map_io/CMakeFiles/map_io.dir/widelands_map_object_packet.cc.o] Error 1 CMakeFiles/Makefile2:21975: recipe for target 'src/map_io/CMakeFiles/map_io.dir/all' failed make[1]: *** [src/map_io/CMakeFiles/map_io.dir/all] Error 2 Makefile:127: recipe for target 'all' failed make: *** [all] Error 2 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Review: Resubmit This is ready for review again. 2 things I'd like to point out: 1. I had to change the visibility of a class member for the code to compile - check the NOCOM here: http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1341081/revision/7113#src/logic/carrier.h Since it'S static, I don't think it's a problem, but I want to make sure. 2. I replaced some is_a stuff in lua_map.cc with type_name, because I assume this would be more efficient. Tell me if I'm wrong and I'll revert. It also has the advantage that the order of checking doesn't matter anymore - before, I had to make sure to start with the leaves in the type hierarchy. The diff is here: http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1341081/revision/7118#src/scripting/lua_map.cc -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Review: Needs Fixing
1) Is strange, but makes sense. I do not fully understand why that is needed
now and wasn't before, but meh.
2) Dangerous assumption. The speed of a dynamic_cast() depends hugely on many
factors. String comparison is always slow. See
http://stackoverflow.com/questions/9778145/how-fast-is-dynamic-cast for a more
involved discussion.
What I do not like about the new design is that we got rid of the enum {} we
had and now use strings to identify our classes everywhere. This is slow, in
many places where a switch() got replaced with an if() else if() at least by an
order of magnitude or two. I think we have to reconsider again. Also the
comments in the old enum reads like we did if (type = BUILDING) in places
which I do not like design wise (fragile!) but which broke if we ever did.
The best design imho would be to bring back the old enum but as an enum class
(http://www.cprogramming.com/c++11/c++11-nullptr-strongly-typed-enum-class.html)
that does not allow = comparison and replace type_name() in the c++ world
through type() everywhere. We then need a function that takes a type and
returns a string (for Lua and probably for logging output, not sure).
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1341081.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Also, your editor formats source code quite differently to the codebase, so there are a lot of style checker warnings. Don't you get style warnings during compile? It's a matter of setting up the editor to confirm with our code formatting standards - which I can't find a reference for. I don't even know if we're supposed to use tabs or blank space for indentation, but it did look like we're using tabs. The codecheck warnings are hidden behind the check to see if codecheck needs to run messages, so everything rushes by so fast I can't notice if there are any. I guess I will have to run the python workaround somebody gave me. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Yes, we are using tabs. Personally, I'd much rather switch to spaces and reformat all code with clang-format, which would then be the definition of 'correct'. I got push back the last two times I tried to establish this though. The codecheck warnings are hidden behind the check to see if codecheck needs to run messages, so everything rushes by so fast I can't notice if there are any. I guess I will have to run the python workaround somebody gave me. Switch to using ninja. Or wait till hjd's compile.sh branch lands which will switch for you (given you have installed it). Seriously - it is way faster and has no disadvantages. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Thanks for the info. I am looking forward to Ninja :D -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Review: Needs Fixing Diff comments: === modified file 'campaigns/atl01.wmf/scripting/init.lua' --- campaigns/atl01.wmf/scripting/init.lua2014-07-17 15:26:32 + +++ campaigns/atl01.wmf/scripting/init.lua2014-07-19 09:00:26 + @@ -324,7 +324,7 @@ if not f.immovable then return end -- Flags are not so interesting - if f.immovable.type == flag and + if f.immovable.name == flag and (f.tln.immovable and is_building(f.tln.immovable)) then f = f.tln end === modified file 'campaigns/tutorial01.wmf/scripting/init.lua' --- campaigns/tutorial01.wmf/scripting/init.lua 2014-07-17 15:26:32 + +++ campaigns/tutorial01.wmf/scripting/init.lua 2014-07-19 09:00:26 + @@ -311,7 +311,7 @@ end -- buildings and constructionsite have a flag - if is_building(i) or i.type == constructionsite then + if is_building(i) or i.descr.type_name == constructionsite then registered_player_immovables[_fmt(i.fields[1].brn)] = true end end @@ -350,15 +350,15 @@ -- Allows constructionsites for the given buildings, all others are invalid -- as is any other immovable build by the player function allow_constructionsite(i, buildings) - if i.type == constructionsite then + if i.descr.type_name == constructionsite then if not buildings then return i end for idx,n in ipairs(buildings) do if i.building == n then return i end end return false - elseif i.type == flag then + elseif i.name == flag then local tr = i.fields[1].tln.immovable - if tr and tr.type == constructionsite then + if tr and tr.descr.type_name == constructionsite then return allow_constructionsite(tr, buildings) end end @@ -442,7 +442,7 @@ blocker:lift_blocks() -- Wait for flag - while not (f.immovable and f.immovable.type == flag) do sleep(300) end + while not (f.immovable and f.immovable.name == flag) do sleep(300) end o.done = true sleep(300) @@ -506,7 +506,7 @@ local function _rip_road() for idx,f in ipairs(cs.fields[1].brn:region(2)) do - if f.immovable and f.immovable.type == road then + if f.immovable and f.immovable.name == road then click_on_field(f) click_on_panel(wl.ui.MapView().windows. field_action.buttons.destroy_road, 300) @@ -671,7 +671,7 @@ local function _find_nearby_flag() for i=2,8 do for idx, f in ipairs(conquer_field:region(i)) do -if f.immovable and f.immovable.type == flag then +if f.immovable and f.immovable.name == flag then return f end end === modified file 'scripting/format_help.lua' --- scripting/format_help.lua 2014-07-17 09:42:13 + +++ scripting/format_help.lua 2014-07-19 09:00:26 + @@ -330,23 +330,23 @@ image_line(tribes/ .. tribename .. / .. resourcename .. /menu.png, 1, p(purpose)) if (note) then result = result .. rt(h3(_Note:)) .. rt(p(note)) end - if(building_description.type == productionsite) then + if(building_description.type_name == productionsite) then if(building_description.workarea_radius and building_description.workarea_radius 0) then result = result .. text_line(_Working radius:, building_description.workarea_radius) end - elseif(building_description.type == warehouse) then + elseif(building_description.type_name == warehouse) then result = result .. rt(h3(_Healing:) .. p(_Garrisoned soldiers heal %s per second:bformat(building_description.heal_per_second))) result = result .. text_line(_Conquer range:, building_description.conquers) - elseif(building_description.type == militarysite) then + elseif(building_description.type_name == militarysite) then result = result .. rt(h3(_Healing:) .. p(_Garrisoned soldiers heal %s per second:bformat(building_description.heal_per_second))) result = result .. text_line(_Capacity:, building_description.max_number_of_soldiers) result = result .. text_line(_Conquer range:, building_description.conquers) - elseif(building_description.type == trainingsite) then + elseif(building_description.type_name == trainingsite) then result = result .. rt(h3(_Training:)) if(building_description.max_attack and building_description.min_attack) then -- TRANSLATORS: %1$s = Health, Evade, Attack or Defense. %2$s and %3$s are numbers. @@ -693,7 +693,7 @@ local building_description = wl.Game():get_building_description(tribename, building_description.name) local result = - if(building_description.type == productionsite
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
I added a bunch of NOCOM(#codereview). You do not implement some methods (see the codereview comments). The code I pushed still doesn't compile, but it should give you an idea of what is missing. Also, your editor formats source code quite differently to the codebase, so there are a lot of style checker warnings. Don't you get style warnings during compile? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
Review: Needs Fixing This avoids having to get the tribes' Immovable_Descr through the Lua interface. If you think this is a problem, I can have a go at it though. Otherwise, ready for merging. I do not think this is a problem. Wrapping those too might make sense in the future anyways though. Diff comments: === modified file 'campaigns/atl01.wmf/scripting/init.lua' --- campaigns/atl01.wmf/scripting/init.lua2014-07-15 10:02:22 + +++ campaigns/atl01.wmf/scripting/init.lua2014-07-16 21:09:18 + @@ -324,7 +324,7 @@ if not f.immovable then return end -- Flags are not so interesting - if f.immovable.type == flag and + if f.immovable.name == flag and (f.tln.immovable and is_building(f.tln.immovable)) then f = f.tln end === modified file 'campaigns/tutorial01.wmf/scripting/init.lua' --- campaigns/tutorial01.wmf/scripting/init.lua 2014-07-15 10:02:22 + +++ campaigns/tutorial01.wmf/scripting/init.lua 2014-07-16 21:09:18 + @@ -311,7 +311,7 @@ end -- buildings and constructionsite have a flag - if is_building(i) or i.type == constructionsite then + if is_building(i) or i.descr.type_name == constructionsite then registered_player_immovables[_fmt(i.fields[1].brn)] = true end end @@ -350,15 +350,15 @@ -- Allows constructionsites for the given buildings, all others are invalid -- as is any other immovable build by the player function allow_constructionsite(i, buildings) - if i.type == constructionsite then + if i.descr.type_name == constructionsite then if not buildings then return i end for idx,n in ipairs(buildings) do if i.building == n then return i end end return false - elseif i.type == flag then + elseif i.name == flag then local tr = i.fields[1].tln.immovable - if tr and tr.type == constructionsite then + if tr and tr.descr.type_name == constructionsite then return allow_constructionsite(tr, buildings) end end @@ -442,7 +442,7 @@ blocker:lift_blocks() -- Wait for flag - while not (f.immovable and f.immovable.type == flag) do sleep(300) end + while not (f.immovable and f.immovable.name == flag) do sleep(300) end o.done = true sleep(300) @@ -506,7 +506,7 @@ local function _rip_road() for idx,f in ipairs(cs.fields[1].brn:region(2)) do - if f.immovable and f.immovable.type == road then + if f.immovable and f.immovable.name == road then click_on_field(f) click_on_panel(wl.ui.MapView().windows. field_action.buttons.destroy_road, 300) @@ -671,7 +671,7 @@ local function _find_nearby_flag() for i=2,8 do for idx, f in ipairs(conquer_field:region(i)) do -if f.immovable and f.immovable.type == flag then +if f.immovable and f.immovable.name == flag then return f end end === modified file 'scripting/format_help.lua' --- scripting/format_help.lua 2014-07-15 10:02:22 + +++ scripting/format_help.lua 2014-07-16 21:09:18 + @@ -331,23 +331,23 @@ image_line(tribes/ .. tribename .. / .. resourcename .. /menu.png, 1, p(purpose)) if (note) then result = result .. rt(h3(_Note:)) .. rt(p(note)) end - if(building_description.type == productionsite) then + if(building_description.type_name == productionsite) then if(building_description.workarea_radius and building_description.workarea_radius 0) then result = result .. text_line(_Working radius:, building_description.workarea_radius) end - elseif(building_description.type == warehouse) then + elseif(building_description.type_name == warehouse) then result = result .. rt(h3(_Healing:) .. p(_Garrisoned soldiers heal %s per second:bformat(building_description.heal_per_second))) result = result .. text_line(_Conquer range:, building_description.conquers) - elseif(building_description.type == militarysite) then + elseif(building_description.type_name == militarysite) then result = result .. rt(h3(_Healing:) .. p(_Garrisoned soldiers heal %s per second:bformat(building_description.heal_per_second))) result = result .. text_line(_Capacity:, building_description.max_number_of_soldiers) result = result .. text_line(_Conquer range:, building_description.conquers) - elseif(building_description.type == trainingsite) then + elseif(building_description.type_name == trainingsite) then result = result .. rt(h3(_Training:)) if(building_description.max_attack and building_description.min_attack) then -- TRANSLATORS:
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
There is also a diff comment buried in my last comment. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
