Page MenuHomeWildfire Games

Fortress map should provide same population space for all civs
ClosedPublic

Authored by lyv on Feb 7 2018, 10:48 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 27, 12:28 AM
Unknown Object (File)
Sun, Sep 22, 6:27 AM
Unknown Object (File)
Fri, Sep 13, 12:05 PM
Unknown Object (File)
Tue, Sep 3, 12:37 PM
Unknown Object (File)
Aug 23 2024, 7:31 AM
Subscribers
Restricted Owners Package
Restricted Owners Package

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
Branch
/ps/trunk
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4852
Build 8406: arc lint + arc unit

Event Timeline

Owners added a subscriber: Restricted Owners Package.Feb 7 2018, 10:48 AM
lyv edited the summary of this revision. (Show Details)
lyv edited the test plan for this revision. (Show Details)

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)

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

Added programming.json as requested.

Owners added a subscriber: Restricted Owners Package.Feb 7 2018, 2:53 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)

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

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 :-)

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?

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

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

Yes.

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.