@bunnybot merge
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352943
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe.
___
@bunnybot merge
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352943
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe.
___
@ypopezios: Good idea, but not as easy as it sounds - when set to "random", the
tribe is already sneakily assigned in the background so that the user can
choose an initialization ("Headquarters") etc. So, removing that extra variable
has a lot of other implications in the code as well and is
Review: Approve compile, review, test,
Works for me,
I cannot judge mutch abouthe the code.
ypopezios: you are correct, but lets get rid of the bug first
and do the improvement in some second step.
Gun: will we need a resubmit before wer can merge this?
@bonnybot merge
--
Added inline comment.
Diff comments:
>
> === modified file 'src/ui_fsmenu/launch_spg.cc'
> --- src/ui_fsmenu/launch_spg.cc 2018-07-07 20:22:12 +
> +++ src/ui_fsmenu/launch_spg.cc 2018-08-26 11:35:09 +
> @@ -283,7 +283,14 @@
> Widelands::PlayerNumber const nrplayers =
Changes are done. Could you please retest to make sure that I haven't missed
anything?
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352943
Your team Widelands Developers is subscribed to branch
Review: Needs Fixing
I like the proposed change. However, when starting the editor and clicking the
"Players" button I get an exception:
widelands: ../src/logic/map.cc:411: const string&
Widelands::Map::get_scenario_player_tribe(Widelands::PlayerNumber) const:
Assertion `p <= get_nrplayers()'
Review: Resubmit
Actually, the players loaded into the editor were never used, so I kicked them
out. I changed the semantics so that an empty tribe means "pick at random."
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352943
Your team
Review: Approve
Code is looking okay and the editor no longer crashes. Two observations:
- When loading the old/broken maps, we still get the "Tribe not known" message.
Maybe only display a "note:" message and select a default tribe in that case?
Having the wrong tribe is in most cases
Thanks GunChleoc.
I'll delete this branch then.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe.
I think it makes most sense if I'm the one who takes this one on - the game
setup screens are in transition code-wise, so it's hard for somebody who
doesn't know that code to deal with it.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
My suggestion has been based solely on general programming principles, not on
reading the code. I'm not familiar with the relevant files. I had a look and
they are a mess (as most of the codebase is). If someone points me to specific
places in the code, maybe I could do it.
--
ypopecios, can you please make the needed changes?
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe.
Random tribe is a separate variable in the game setup screens. I think
ypopezios has the right idea to keep the implementation simple.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
Your team Widelands Developers is subscribed to branch
I would not create a special extra tribe. I would instead use a nullptr as a
default value and have it represent the random tribe by convention and only
where it makes sense.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
Your team
As ever: The idea might be good, but implementing is quite difficult.
I think a special "Random Tribe" needs only the name, descname and an icon,
nothing more. So i have tried to add a "Random Tribe" through lua, but it fails
because the code for tribe initializing depends on so much other
Sounds good - we could have that both for the random map generator and the
player menu.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
Your team Widelands Developers is subscribed to branch
Since one could also choose "Random tribe" in the Game setup menu, regardless
of the maps setting, i am thinking about this:
Maybe we can have a special tribe called "Random Tribe". So a map designer can
choose "Random tribe" beside the others in the editor player menu for a normal
map.
Review: Approve
Looks good to me - the maps will be more interesting anyway than if we were
assigning Barbarians to everybody.
Not tested.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
Your team Widelands Developers is subscribed to
After sleeping a night i am not sure if this is the correct approach to fix the
bug ...
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352038
Your team Widelands Developers is requested to review the proposed merge of
20 matches
Mail list logo
