Remove use of barbarian stronghold

Bug #787508 reported by Hans Joachim Desserud
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
widelands
Fix Released
Low
Unassigned

Bug Description

A while back the barbarian building stronghold was removed (or rather disabled) from the game (bug 695018). However I noticed the building is still used in some places where it probably should be removed or replaced with something else. I have done some changes, and basically replaced them with barriers instead (not added additional soldiers though) or removed it where it is mentioned. I will attach a branch with my work so far. From what I can see, the entries that remain are those below, which I haven't looked closely at, but seems to be slightly more important for the scenarios than the ones I have changed:
widelands/campaigns$ grep -ri "stronghold" .
./emp02.wmf/scripting/starting_conditions.lua: "stronghold",
./t02.wmf/scripting/mission_thread_texts.lua:[[Build military buildings (like sentries and strongholds) to expand your
./t02.wmf/scripting/mission_thread.lua: local rv = plr:get_buildings{"sentry", "stronghold"}
./t02.wmf/scripting/mission_thread.lua: if #rv.sentry + #rv.stronghold > 0 then
./t02.wmf/scripting/starting_conditions.lua: "stronghold"
./t03.wmf/scripting/mission_thread.lua: p1:allow_buildings{"sentry", "stronghold", "barrier"}
./t03.wmf/scripting/mission_thread.lua: {"stronghold", 118, 100, soldiers =
./t03.wmf/scripting/starting_conditions.lua: {"stronghold", 104, 110, soldiers = {[{0,0,0,0}]=2}},
./t03.wmf/scripting/starting_conditions.lua: {"stronghold", 104, 30, soldiers = {
./t03.wmf/scripting/starting_conditions.lua: {"stronghold", 102, 90, soldiers = {[{0,0,0,0}]=1}},
./t03.wmf/scripting/starting_conditions.lua: {"stronghold", 132, 78, soldiers = {
./t03.wmf/scripting/starting_conditions.lua: {"stronghold", 132, 3, soldiers = {

I could probably have changed these as well, but they seem to be used by the scripting so I'm not sure if it could break something.

Related branches

Revision history for this message
Leif Sandstede (lcsand) wrote :

Most of that can be safely removed without breaking the scenario.
Just a minor change in the displayed scenario text is needed at one point.
I will make a branch from your branch with the changes.

Changed in widelands:
assignee: nobody → Leif Sandstede (lcsand)
Leif Sandstede (lcsand)
Changed in widelands:
status: New → In Progress
Leif Sandstede (lcsand)
Changed in widelands:
assignee: Leif Sandstede (lcsand) → nobody
Revision history for this message
Leif Sandstede (lcsand) wrote :

This is pretty much done.
Anyone care to comment?

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

Thanks for your suggested fix, Leif. Looks like you have cleared up the rest. That said, I have not looked at your changes in detail and haven't had time to test them yet. Also I'm wondering why you have removed a reference to barriers in one of the texts.

The reason I didn't touch the remaining strongholds was because I was afraid to break anything, so I'm probably not the best person to review the changes. What do others think? Would it be better to create two merge proposals towards trunk for each branch, to get someone who knows a bit better than me what's going on to review it?

Revision history for this message
Leif Sandstede (lcsand) wrote :

That was a comment in the code wich was outdated, the code does not check for barriers to be built so I updated the comment.
I did not test the changes in an actual game, but I verified the mission logic and it should work.
Just merge my changes into your branch and let someone else review the whole thing. Your changes seem to be OK too.
Might as well request a merge into trunk.

Revision history for this message
Hans Joachim Desserud (hjd) wrote :

I have unlinked my branch from bug 695018, since bugs marked fix released are generally considered done and not touched unless it reappears. I guess the difference is that bug dealt with the building in general, while this one covers where it is used. A minor distinction, but I think it makes sense. I added a comment with a link to this bug, though...

Revision history for this message
SirVer (sirver) wrote :

thanks for your work! I will review this when I find some time (rare at the moment).

Revision history for this message
SirVer (sirver) wrote :

Reviewed and merged Leifs branch in r5926. Credit and Thanks go to you both for the work!

Changed in widelands:
status: In Progress → Fix Committed
Revision history for this message
SirVer (sirver) wrote :

Released in build17-rc1.

Changed in widelands:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.