Page MenuHomeWildfire Games

Fortress map should provide same population space for all civs
ClosedPublic

Authored by lyv on Feb 7 2018, 10:48 AM.

Details

Summary

Two civilizations that have houses with 5 population was omitted.

Test Plan

Generate the map and see if all civilizations have 50 population slots.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lyv created this revision.Feb 7 2018, 10:48 AM
Owners added a subscriber: Restricted Owners Package.Feb 7 2018, 10:48 AM
lyv updated this revision to Diff 5693.Feb 7 2018, 10:53 AM
lyv edited the summary of this revision. (Show Details)
lyv edited the test plan for this revision. (Show Details)
elexis added a comment.Feb 7 2018, 2:37 PM

Can you add a programming.json entry or tell me your realname if you want to have that mentioned in the credits?

Are you sure it is an oversight? I might suspect that the line was written before these civilizations were added.One could find out using the blame tool.
Good find though.

It would be better if the stuff inside the fortress were split from the actual fortress, but those things are not easy to achieve.
(I was wondering if we could use some kind of CityPainter to fill an area with buildings)

lyv added a comment.Feb 7 2018, 2:48 PM

The line was last changed in 2015. So very well likely that the 2 civs were not yet added back then.

lyv updated this revision to Diff 5695.Feb 7 2018, 2:53 PM

Added programming.json as requested.

Owners added a subscriber: Restricted Owners Package.Feb 7 2018, 2:53 PM
lyv updated this revision to Diff 5696.Feb 7 2018, 2:57 PM
Imarok added a subscriber: Imarok.Feb 7 2018, 2:57 PM
In D1286#52504, @smiley wrote:

Added programming.json as requested.

Good, but you forgot to upload the other file. ;)

Also a comment in the code explaining that these are the civs with small houses (I guess) won't hurt. Could also be done when committing.
(And thx for contributing ofc)

lyv added a comment.Feb 7 2018, 2:59 PM

Yeah, I forgot that I had reverted it.

elexis retitled this revision from Oversight in Fortress.js to Fortress map should provide same population space for all civs.Feb 7 2018, 3:43 PM
elexis added a comment.Feb 7 2018, 4:06 PM

Mauryan cc:
rP12005 | Pureon | 2012-06-24 11:54:52 +0200 (So, 24 Jun 2012) | 1 line

Ptol CC:
rP13905 | michael | 2013-09-29 05:08:59 +0200 (So, 29 Sep 2013) | 1 line

This civ condition was changed in:
rP11361 Spahbod if (civ == "celt" || civ == "iber")
rP12034 vts if (civ == "brit" || civ == "celt" || civ == "gaul" || civ == "iber")

So basically I guess noone ever considered the house pop differences.
We should add a comment for that. Also should parse the templates ideally to determine the number of houses to be placed.
There is the Engine.GetTemplate function returning that data in case we can lure you :-)

lyv updated this revision to Diff 5713.Feb 8 2018, 1:25 PM
elexis added a comment.Feb 8 2018, 3:36 PM

Great! This way we don't have to maintain that line anymore and it works for mods too. Or at least for mods that have houses with only 5 or 10 pop space. (One could compute if one wanted to be exact, but who cares, not relevant. Can do <= 5 however.)

If that wall variable would be split into two (one for the actual walls, one for the city buildings), then that part might look cleaner. But that sounds like work. One would have to replace the long element with gap elements.
You don't happen to want to look into it, do you? :-) (It's something that should be done for the danubius fortification too I assume)

If you don't want to try it's ok, then I'll commit this patch.

Also, are you (-_-) from the lobby?

lyv added a comment.Feb 11 2018, 4:32 PM

I plan to get around it once more. And will submit a patch then.

Also, are you (-_-) from the lobby?

Yes.

elexis accepted this revision.Feb 16 2018, 10:37 PM

The work on your random map should have priority now.
Thanks for this patch, it's much more sustainable than the first iteration!

This revision is now accepted and ready to land.Feb 16 2018, 10:37 PM
This revision was automatically updated to reflect the committed changes.