Annihilation of duplicate Unknown map player placement and water code
ClosedPublic
Actions

Authored by elexis on Mar 23 2017, 5:34 PM.

Details

Reviewers: None
Commits: rP20420: Obliterate Unknown (rP12545), Unknown Land (rP12654) and Unknown Nomad…
Trac Tickets: #4317

Summary

As reported in #4317, the Unknown, Unknown Land and Unknown Nomad maps have about 10 variations and each copied code to achieve the intended mapgen.
So it contains about 30 duplications of mapgen code with broken indentation, whitespace, sometimes further duplication depending on horizontal/vertical layout.
Instead of fixing small code style mistakes there, it has to be unified in the first place.

Warning: When reading the code, you will go through the following stages of emotions:

Test Plan

Modify the md variable to trigger each variation of the map generations possible.
Use Atlas to quickly regenerate new variations without restarting the game.
Use 8 players, small mapsize and Iberian civs to test the player placement function.

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

elexis created this revision.Mar 23 2017, 5:34 PM

Obviously far from complete

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/579/ for more details.

Harbormaster completed remote builds in B854: Diff 906.Mar 23 2017, 6:19 PM

AFAICT these RMS scripts just include a bunch of others verbatim. Why not mark those included (and more, since quite a few have been added) with some keyword, and then pick one of those randomly before generating?

In D252#9673, @leper wrote:

AFAICT these RMS scripts just include a bunch of others verbatim. Why not mark those included (and more, since quite a few have been added) with some keyword, and then pick one of those randomly before generating?

Some variations are unique to the Unknown, other variations are very similar to other maps (continent map), others are remotely similar (enlgish channel for the horizontal river one, but that doesn't include the random biome part).
Would be nice to nuke them entirely, don't really want to lose the classic maps though.
Also once they are cleaned up, one can do actual improvements with them.
Didn't / don't really want to work on this, but each time someone wants to change something (D235), the right solution would be not changing slight coding style mistakes but nuking the duplicate code (same reason for rP19282) (Insert Yo dawg I heard you like bugs meme).

Some variations are unique to the Unknown,

So maybe add them as new maps?

other variations are very similar to other maps (continent map)

Those should just use the other map (which most likely have seen more improvements than the versions in the unkown scripts).

others are remotely similar (enlgish channel for the horizontal river one, but that doesn't include the random biome part).

Either nuke them, or change the present map to allow random biomes, or don't do that and remove some of the pointless variants (sometimes quality trumps quantity).

Would be nice to nuke them entirely, don't really want to lose the classic maps though.
Also once they are cleaned up, one can do actual improvements with them.

But we still have duplicated code in a way that seems very strange.

elexis mentioned this in D235: randBool changes from randomPatch.Mar 28 2017, 6:24 AM

elexis mentioned this in rP11137: Added new Random maps. Updated the existing ones with the new starting unit….Sep 30 2017, 4:45 AM

refs rP19236

The players I maintain this for enjoy every map we have, I won't remove them unless I have to.
The developer pain expressed in the memes in the summary is alleviated by using the mechanism to load common code that rmgen supports.
Alternative file loading mechanisms (files instead of directories) may only be implemented in a separate diff.
Once gamesetup options for individual maps are supported (#4838), the player can also select which of the variants should be loaded.
"Unknown" and "Unknown Land" can become one map by adding a new gamesetup option that only works for that map (allow naval checkbox).
"Unknown" and "Unknown Nomad" can become one map after supporting Nomad on all maps, #3591.