Page MenuHomeWildfire Games

Extinct Volcano (Rising Water map)
ClosedPublic

Authored by elexis on Mar 17 2017, 3:52 AM.

Details

Summary

https://github.com/elexis1/0ad/tree/extinctvolcano

Third version with resources and gaia buildings becoming identical visual actors:



Second version with bumps and rising water

First version of the random map script provided by Hannibal Barca.

Test Plan

Things to check for:

  • Resource collisions
  • extreme mapsizes
  • units stuck at trees
  • the script throwing random errors
  • description making sense to newcomers
  • throwing errors in relic gamemode
  • players getting defeated when it makes sense (only docks remaining?)
  • Water rising not too soon, not too late, not too slowly, not too fast

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
bb added inline comments.Apr 20 2017, 9:21 PM
binaries/data/mods/public/maps/random/extinct_volcano.js
4 ↗(On Diff #1274)

Why we have these duplicate consts? more of them below

15 ↗(On Diff #1274)

empty todo?

48 ↗(On Diff #1274)

]; on next line, below too

72 ↗(On Diff #1274)

ever written too?

75 ↗(On Diff #1274)

unused?

78 ↗(On Diff #1274)

isn't this empty the first few usages?

97 ↗(On Diff #1274)

2 * PI and spaces

110 ↗(On Diff #1274)

unneeded comment

153 ↗(On Diff #1274)

Math.

153–154 ↗(On Diff #1274)

Was thinking if we could skip the while, but meh

154 ↗(On Diff #1274)

2 * Pi

208 ↗(On Diff #1274)

hmmmpfff 15 => 45 => 20 lolz

244 ↗(On Diff #1274)

nuke .0

294 ↗(On Diff #1274)

whitespaces

312 ↗(On Diff #1274)

whitespaces

332 ↗(On Diff #1274)

whitespaces

333 ↗(On Diff #1274)

whitespaces

347 ↗(On Diff #1274)

whitespaces

372 ↗(On Diff #1274)

whitespaces and trailing comma

383 ↗(On Diff #1274)

whitespaces

408 ↗(On Diff #1274)

ok, we create an array of 1 element ask for its length and loop over it : nucular

413 ↗(On Diff #1274)

whitespaces

binaries/data/mods/public/maps/random/extinct_volcano.json
5 ↗(On Diff #1274)

...of its former era.?

binaries/data/mods/public/maps/random/extinct_volcano_triggers.js
23 ↗(On Diff #1274)

Time in minutes between the increases of the water level.?

38 ↗(On Diff #1274)

Number of meters the waterheight increases every step.?

54 ↗(On Diff #1274)

Would be nice if we have a couple of these sentences, some suggestions:

"The rivers are standing high, we need to find a save place!"
"We have got to find dry ground, our lands will drawn soon!"
"The lakes start swallowing the land, we have to find shelter!"

70 ↗(On Diff #1274)

don't we have a timer component for that?

74 ↗(On Diff #1274)

cmpTimer

85 ↗(On Diff #1274)

It is meant to get all entities here I guess, perhaps make a function for that?

87 ↗(On Diff #1274)

cmpPosition

88 ↗(On Diff #1274)

units can stay in shallow water, so do trees, mines etc. perhaps nice to take this into account. no strong opinion

91–93 ↗(On Diff #1274)

Move below health check

99 ↗(On Diff #1274)

Move out of loop

104 ↗(On Diff #1274)

siege == Unit right? but siege won't sink in the ground when under water... perhaps use "Organic" class

118 ↗(On Diff #1274)

We already called GetPosition so if we pull that out of the check we don't need this.

122 ↗(On Diff #1274)

only used once, inline

125 ↗(On Diff #1274)

In most entity change function, the new ent in created first, afterwards the old one is destroyed. With that some of the variables above can be inlined.

133–135 ↗(On Diff #1274)

bummer there is no setXYZRotation

140 ↗(On Diff #1274)

cmpTimer

149–150 ↗(On Diff #1274)

I did prefer to have the constant 60 * 1000 behind the ms time

binaries/data/mods/public/maps/random/rmgen/misc.js
809–812 ↗(On Diff #1274)

periods and caps
also fx and fz

852 ↗(On Diff #1274)

for ... of indeed bad since i-1 used

856 ↗(On Diff #1274)

3 == steepness? (dunno)

879 ↗(On Diff #1274)

fx != fractionToTiles(fx) so not the same. (we don't want duplicate fractionToTiles execution so it requires new variable guess)

882 ↗(On Diff #1274)

shouldn't that be clLast?

This revision now requires changes to proceed.Apr 20 2017, 9:21 PM
Sandarac requested changes to this revision.EditedApr 23 2017, 12:00 AM
Sandarac added a subscriber: Sandarac.

When playing this map with the capture the relic victory condition, relics can be submerged by the rising water and deleted. The actual relic count is properly updated as of D283, but warnings are still shown when this happens (the warning is intended as relics should not be deleted/destroyed).

From my point of view, this is unacceptable, as warnings should not be thrown during the course of a typical game. I believe that either first a way to prevent certain victory conditions on certain maps should be implemented, or (as suggested by sanderd17) the map description should state that capture the relic does not work on this map.

elexis marked 63 inline comments as done.May 11 2017, 10:17 PM

Thanks for the review!
Will never serve you something with that whitespace or non-inlined group calls anymore!
Besides you found a good number of issues and optimizations, that make this code much more appealing :-)

binaries/data/mods/public/maps/random/extinct_volcano.js
4 ↗(On Diff #1274)

(Leftover. Did change the textures back an forth)

11 ↗(On Diff #1274)

These actually seem useful to me, since you can insert a texture here and have that texture appear right below the forests. If we'd replace all forest floor occurances with the hill textures, we'd have to start looking. And it's not unlikely that someone will want to add a forest floor texture. It's just that I was completely unable to find one that fits.

15 ↗(On Diff #1274)

(Leftover; we don't have Lava on this map, so TODO was nuke after fixing the volcano)

72 ↗(On Diff #1274)

Same story as on survival and many others - its the createFood function which grossly takes that as a default. But I'll pass it explicitly to make it more obvious, good point.

75 ↗(On Diff #1274)

createLayeredPatches. Passing explicitly now. Actually wondering whether they look artistically good enough, or whether that map would be better off being more monotonous.

78 ↗(On Diff #1274)

Is. Removing the two misleading entries.

153–154 ↗(On Diff #1274)

let mAngle = bbAngle; pointless, entire loop pointless (since the intention is to keep a distance < x but > y to the berries, but those arent placed first here.

208 ↗(On Diff #1274)

This just reflects the coding process

245–247 ↗(On Diff #1274)

Yes sir (this was a leftover from rewriting 4 different forest methods, since all of them had some weird issues which I luckily don't recall)

408 ↗(On Diff #1274)

Looked for other bush models, but that only other one seems to be a dupe.
Also reusing numStragglers here seems to make totalTrees lie, but don't really care.

423 ↗(On Diff #1274)

Thanks! Also misses water waviness, since those puddles shouldn't have ocean waves

binaries/data/mods/public/maps/random/extinct_volcano_triggers.js
38 ↗(On Diff #1274)

per step

54 ↗(On Diff #1274)

Sure, good idea

70 ↗(On Diff #1274)

Omg did I write that piece of code? How on earth did that not go OOS, nor OOS on rejoin. Thx!

85 ↗(On Diff #1274)

Probably useful, making a separate proposal and if that makes it in, will change this occurance

88 ↗(On Diff #1274)

Was worried about units being able to do things underwater, but they will just become wiped out with the next increment and this feature was requested by hannibal too. Since template_unit_infantry has <Height>2.5</Height>, going for +1

104 ↗(On Diff #1274)

The idea is that owned entities either become visual actors or die. I guess we can keep those siege engine remains underwater, because players will not confuse them more with alive units as they might be confused with existing buildings becoming actors. So yay.
Also relics and potentially others, but "Organic" seems to be a wise decision.

125 ↗(On Diff #1274)

Tbh I find it more clean to recall the set of properties to be transfered explicitly than mixing calls to both entities

133–135 ↗(On Diff #1274)

Could implement, could also not

149–150 ↗(On Diff #1274)

me too, it can be considered the unit

binaries/data/mods/public/maps/random/rmgen/misc.js
856 ↗(On Diff #1274)

That's the distance between those 2 textures. If this is used for painting dirt patches, we can increase the size of the dirt patches this way f.e.

879 ↗(On Diff #1274)

ix, iz, done.

882 ↗(On Diff #1274)

The idea is that the smoke is only placed onto the lava-area, so creating clLava to make it more obvious.

binaries/data/mods/public/maps/random/volcanic_lands.js
144 ↗(On Diff #1274)

Making fx, ..., iz, local variables of the new helper function was what broke the volcano smoke. Took me 4 hours one night and 5 minutes today.

elexis marked 19 inline comments as done.May 11 2017, 10:22 PM

When playing this map with the capture the relic victory condition, relics can be submerged by the rising water and deleted. The actual relic count is properly updated as of D283, but warnings are still shown when this happens (the warning is intended as relics should not be deleted/destroyed).

From my point of view, this is unacceptable, as warnings should not be thrown during the course of a typical game. I believe that either first a way to prevent certain victory conditions on certain maps should be implemented, or (as suggested by sanderd17) the map description should state that capture the relic does not work on this map.

Well, as mentioned in some other differential proposal and on irc, we can very well play this map with relic mode. We have 25 minutes to capture all relics and potentially win by then already, and another 25min until the last dry place is flodded. Since all other entities are destroyed too, it shouldn't be unexpected at all of the relic entities die off or become actors. If super serious about the warning, it can be simply removed, or if super duper serious about keeping it for all maps except maps that expect relics to vanish (for example if they come with destructable catafalques), then the map should specify a boolean that relics can be destroyed.
Adding this boolean to the JSON file is easy, it will be absorbed by the game attributes and we can pass it down to the endgame manager. However since the persist-match-settings issue in the gamesetup isn't resolved yet, every random map will have this property if after selecting this map once, so it should be done afterwards.

elexis updated this revision to Diff 1858.May 11 2017, 11:22 PM
elexis edited edge metadata.

Address bb's review remarks and garrison roman soldiers in those towers.

Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.May 11 2017, 11:22 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1154/
See console output for more information: http://jw:8080/job/phabricator/1154/console

Build is green

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

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

Just reverted to using those 2 rangemanager funcitons as I'm not convinced of the existing name nor the name proposed in D477.

Thanks for the extensive review! I hope we have found all issues. In our playtest, it has often been the end of the game has often been drawn out a bit, because one couldn't move with ships until the water passed most mountains.

binaries/data/mods/public/maps/random/extinct_volcano.js
423 ↗(On Diff #1274)

Actually the color was equivalent to black, which was only noticeable when using the worst ugliest water settings possible. So keeping the hueness and saturation, just increasing the value of the water color a bit.

binaries/data/mods/public/maps/random/extinct_volcano_triggers.js
54 ↗(On Diff #1274)

have got to -> have to

This revision was automatically updated to reflect the committed changes.