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
Branch
extinctvolcano
Lint
Lint OK
SeverityLocationCodeMessage
Auto-Fixsource/tools/dist/0ad.nsi:49TXT6Trailing Whitespace
Auto-Fixsource/tools/dist/0ad.nsi:50TXT6Trailing Whitespace
Unit
No Unit Test Coverage
Build Status
Buildable 1653
Build 2615: Vulcan BuildJenkins
Build 2614: arc lint + arc unit

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
5

Why we have these duplicate consts? more of them below

16

empty todo?

49

]; on next line, below too

73

ever written too?

76

unused?

79

isn't this empty the first few usages?

98

2 * PI and spaces

111

unneeded comment

154

Math.

154–155

Was thinking if we could skip the while, but meh

155

2 * Pi

209

hmmmpfff 15 => 45 => 20 lolz

245

nuke .0

295

whitespaces

313

whitespaces

333

whitespaces

334

whitespaces

348

whitespaces

373

whitespaces and trailing comma

384

whitespaces

409

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

414

whitespaces

binaries/data/mods/public/maps/random/extinct_volcano.json
6

...of its former era.?

binaries/data/mods/public/maps/random/extinct_volcano_triggers.js
24

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

39

Number of meters the waterheight increases every step.?

55

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!"

71

don't we have a timer component for that?

75

cmpTimer

86

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

88

cmpPosition

89

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

92–94

Move below health check

100

Move out of loop

105

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

119

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

123

only used once, inline

126

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.

134–136

bummer there is no setXYZRotation

141

cmpTimer

150–151

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

binaries/data/mods/public/maps/random/rmgen/misc.js
807–810

periods and caps
also fx and fz

850

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

854

3 == steepness? (dunno)

877

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

880

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
5

(Leftover. Did change the textures back an forth)

12

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.

16

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

73

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.

76

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

79

Is. Removing the two misleading entries.

154–155

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.

209

This just reflects the coding process

246–248

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)

409

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.

424

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

binaries/data/mods/public/maps/random/extinct_volcano_triggers.js
39

per step

55

Sure, good idea

71

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

86

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

89

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

105

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.

126

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

134–136

Could implement, could also not

150–151

me too, it can be considered the unit

binaries/data/mods/public/maps/random/rmgen/misc.js
854

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.

877

ix, iz, done.

880

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

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
424

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
55

have got to -> have to

This revision was automatically updated to reflect the committed changes.