Page MenuHomeWildfire Games

[WIP] Texture priority support for rmgen
Changes PlannedPublic

Authored by smiley on Jul 29 2018, 6:14 PM.

Details

Reviewers
elexis
FeXoR
Summary

Implement the currently unused texture priority support for rmgen. This would allow for certain textures to be submissive to surrounding textures. For example, a small patch of dirt. And so, better blending. If still not convinced, play around with texture painting use both mouse buttons in Atlas.
TODO: Add painter support so this would actually be usable in an easy way.

Test Plan

Use g_Map.setTexture() in all kinds of ways.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6324
Build 10493: Vulcan BuildJenkins
Build 10492: arc lint + arc unit

Event Timeline

smiley created this revision.Jul 29 2018, 6:14 PM
Owners added a subscriber: Restricted Owners Package.Jul 29 2018, 6:14 PM
Vulcan added a subscriber: Vulcan.Jul 29 2018, 6:19 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|  24|  24| 		for (let z = 0; z < this.size; ++z)
|  25|  25| 		{
|  26|  26| 			let textureID = this.getTextureID(typeof baseTerrain == "string" ? baseTerrain : pickRandom(baseTerrain));
|  27|    |-			this.texture[x][z] = {"index": textureID, "priority": 10}; // 10 to allow for multiple "LOW" ones close by. Also keep in mind priority is u32 in c++
|    |  27|+			this.texture[x][z] = { "index": textureID, "priority": 10}; // 10 to allow for multiple "LOW" ones close by. Also keep in mind priority is u32 in c++
|  28|  28| 		}
|  29|  29| 	}
|  30|  30| 
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|  24|  24| 		for (let z = 0; z < this.size; ++z)
|  25|  25| 		{
|  26|  26| 			let textureID = this.getTextureID(typeof baseTerrain == "string" ? baseTerrain : pickRandom(baseTerrain));
|  27|    |-			this.texture[x][z] = {"index": textureID, "priority": 10}; // 10 to allow for multiple "LOW" ones close by. Also keep in mind priority is u32 in c++
|    |  27|+			this.texture[x][z] = {"index": textureID, "priority": 10 }; // 10 to allow for multiple "LOW" ones close by. Also keep in mind priority is u32 in c++
|  28|  28| 		}
|  29|  29| 	}
|  30|  30| 
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
| 237| 237| 	if (!this.inMapBounds(position))
| 238| 238| 		throw new Error("getTexture: invalid tile position " + uneval(position));
| 239| 239| 
| 240|    |-	return {"texture": this.IDToName[this.texture[position.x][position.y].index], "priority" : this.texture[position.x][position.y].priority};
|    | 240|+	return { "texture": this.IDToName[this.texture[position.x][position.y].index], "priority" : this.texture[position.x][position.y].priority};
| 241| 241| };
| 242| 242| 
| 243| 243| /**
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'priority'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
| 237| 237| 	if (!this.inMapBounds(position))
| 238| 238| 		throw new Error("getTexture: invalid tile position " + uneval(position));
| 239| 239| 
| 240|    |-	return {"texture": this.IDToName[this.texture[position.x][position.y].index], "priority" : this.texture[position.x][position.y].priority};
|    | 240|+	return {"texture": this.IDToName[this.texture[position.x][position.y].index], "priority": this.texture[position.x][position.y].priority};
| 241| 241| };
| 242| 242| 
| 243| 243| /**
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
| 237| 237| 	if (!this.inMapBounds(position))
| 238| 238| 		throw new Error("getTexture: invalid tile position " + uneval(position));
| 239| 239| 
| 240|    |-	return {"texture": this.IDToName[this.texture[position.x][position.y].index], "priority" : this.texture[position.x][position.y].priority};
|    | 240|+	return {"texture": this.IDToName[this.texture[position.x][position.y].index], "priority" : this.texture[position.x][position.y].priority };
| 241| 241| };
| 242| 242| 
| 243| 243| /**
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
| 267| 267| 
| 268| 268| 	let priorityIndex = priority == "LOW" ? Math.min(...surroundingTiles.priority) - 1 : Math.max(...surroundingTiles.priority) + 1;
| 269| 269| 
| 270|    |-	this.texture[position.x][position.y] = {"index" : this.getTextureID(texture), "priority" : priorityIndex};
|    | 270|+	this.texture[position.x][position.y] = { "index" : this.getTextureID(texture), "priority" : priorityIndex};
| 271| 271| };
| 272| 272| 
| 273| 273| RandomMap.prototype.getHeight = function(position)
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'index'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
| 267| 267| 
| 268| 268| 	let priorityIndex = priority == "LOW" ? Math.min(...surroundingTiles.priority) - 1 : Math.max(...surroundingTiles.priority) + 1;
| 269| 269| 
| 270|    |-	this.texture[position.x][position.y] = {"index" : this.getTextureID(texture), "priority" : priorityIndex};
|    | 270|+	this.texture[position.x][position.y] = {"index": this.getTextureID(texture), "priority" : priorityIndex};
| 271| 271| };
| 272| 272| 
| 273| 273| RandomMap.prototype.getHeight = function(position)
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'priority'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
| 267| 267| 
| 268| 268| 	let priorityIndex = priority == "LOW" ? Math.min(...surroundingTiles.priority) - 1 : Math.max(...surroundingTiles.priority) + 1;
| 269| 269| 
| 270|    |-	this.texture[position.x][position.y] = {"index" : this.getTextureID(texture), "priority" : priorityIndex};
|    | 270|+	this.texture[position.x][position.y] = {"index" : this.getTextureID(texture), "priority": priorityIndex};
| 271| 271| };
| 272| 272| 
| 273| 273| RandomMap.prototype.getHeight = function(position)
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/RandomMap.js
| 267| 267| 
| 268| 268| 	let priorityIndex = priority == "LOW" ? Math.min(...surroundingTiles.priority) - 1 : Math.max(...surroundingTiles.priority) + 1;
| 269| 269| 
| 270|    |-	this.texture[position.x][position.y] = {"index" : this.getTextureID(texture), "priority" : priorityIndex};
|    | 270|+	this.texture[position.x][position.y] = {"index" : this.getTextureID(texture), "priority" : priorityIndex };
| 271| 271| };
| 272| 272| 
| 273| 273| RandomMap.prototype.getHeight = function(position)

Link to build: https://jenkins.wildfiregames.com/job/differential/694/display/redirect

  • It sounds more performant to create two arrays of JS native integer arrays than a 2D array with one object per item.
  • Why not numeric priorities?
binaries/data/mods/public/maps/random/rmgen/RandomMap.js
241

Some users of this expect a string returned

smiley planned changes to this revision.Jul 29 2018, 6:41 PM
FeXoR added a comment.Aug 3 2018, 5:23 PM

Yea, texture priority support is an addition that I'd like to see.
Thanks @smiley to pick this up!

However, a solution shouldn't mess to much with existing map scripts but AFAICS this does (pushing later added textures of the same type above the surrounding even if some are of the same type).
On the other hand this should be as close to what Atlas does IMO (or not?) so if it's done the same there, not sure.
This at least will need quite some testing.

binaries/data/mods/public/maps/random/rmgen/RandomMap.js
30

I'd rather make an array and put textures (or IDs, not sure yet) in as required. The actual priority can be got at export.
"10" is an unneeded preset value.

smiley added a comment.Aug 3 2018, 5:29 PM

On the other hand this should be as close to what Atlas does IMO (or not?) so if it's done the same there, not sure.

Atlas does the same thing. AFAIK, Atlas checks the priorities of the tiles adjacent to the brush, then does the min - 1.

Copy pasted from source/tools/atlas/GameInterface/Handlers/TerrainHandler.cpp

// Priority system: If the new tile should have a high priority,
// set it to one plus the maximum priority of all surrounding tiles
// that aren't included in the brush (so that it's definitely the highest).
// Similar for low priority.
elexis added a comment.Aug 5 2018, 7:48 PM

Your JS test you uploaded to pastebin recently to confirm that 2 native-int arrays are faster than one array with an object with two properties each:


It wasn't by that much, but probably sufficiently.

smiley updated this revision to Diff 6844.Aug 5 2018, 8:22 PM

Use two typed arrays which are more performant and return texture string in getTexture.

smiley planned changes to this revision.Aug 5 2018, 8:26 PM

Still WIP. But this was an important change.

Vulcan added a comment.Aug 5 2018, 8:31 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/712/display/redirect

smiley added inline comments.Aug 5 2018, 8:38 PM
binaries/data/mods/public/maps/random/rmgen/RandomMap.js
19

Just noticed the singluar and plural variable names, will fix with next update

smiley added inline comments.Aug 26 2018, 4:07 PM
binaries/data/mods/public/maps/random/rmgen/RandomMap.js
30

That would lead to an even longer delay to actually finish mapgen. (Considering the things which happen after ExportMap();), IMO, it is better to spread negligible latency over the whole duration than to concentrate them and make it noticeable to the user. I have no better argument though, and would do whats best interms of design.

FeXoR added a comment.EditedAug 26 2018, 7:21 PM

Is the arbitrary "10" is gone? Can't find it any more.

What to store, texture or ID? I'm really not sure.
The ID's where only there for the map export so the map script has at least some control about what's on top AFAIK.
If we now offer full control the ID's are actually unneeded. However, since the number of textures and the number of texture priorities might not at all be the same.

What I'm absolutely not sure about is if the priority system of Atlas is actually that suitable for random maps. If it is OK I'd stick to that as closely as possible.
If not - for a reason I can't think of ATM - we should pick one that is and that might impact if ID's or textures should be saved during generation and handled during export.

Making a bad choice when introducing this feature will likely enrage map authors for decades ;p

smiley added a comment.EditedAug 27 2018, 7:37 AM
In D1599#64687, @FeXoR wrote:

Is the arbitrary "10" is gone? Can't find it any more.

What to store, texture or ID? I'm really not sure.
The ID's where only there for the map export so the map script has at least some control about what's on top AFAIK.
If we now offer full control the ID's are actually unneeded. However, since the number of textures and the number of texture priorities might not at all be the same.

What I'm absolutely not sure about is if the priority system of Atlas is actually that suitable for random maps. If it is OK I'd stick to that as closely as possible.
If not - for a reason I can't think of ATM - we should pick one that is and that might impact if ID's or textures should be saved during generation and handled during export.

The 10 is gone when switched it was switched to a typed array. Intialized with all 0s.
Regarding the ID, it looks to be pretty similair to do a kind of texture priority thing. But the presence of both index and priority plus that todo makes me wonder if it really is the case. Need to read the c++ code to really be sure though.
One limitation of the Atlas method is that you cant choose which surrounding tiles to be submissive to. (For example, only make the tile submissive to the top tile). Maybe its not much of an issue and could be ignored. Might very well be unneeded complexity.
Need to see how these are handled at the engine level for further comments.

Making a bad choice when introducing this feature will likely enrage map authors for decades ;p

Better get this right then ;)

smiley added a comment.EditedNov 11 2018, 7:21 AM

I forgot about writing here before, but the IDs actually do somethig else. Its an index for the textures array. It could be removed, but keeping an int for each tile is more efficient than keeping a string. Plus, there is the cost of moving them around. The code was written a *long* time ago. 2004 to be exact. Maybe, current hardware would make it unneeded.
The fact that it could be used for whats on top is only due to the fact that the first texture would get the first index and so on.

smiley marked 3 inline comments as done.Wed, Nov 14, 9:41 AM
smiley added inline comments.
binaries/data/mods/public/maps/random/rmgen/RandomMap.js
19

Should be renamed to textureIndex

268

This is cannot be signed.

508

Not sure if its worth it, but not necessary to have two mapSize^2 arrays. Can do with one. 8bits for index, 8bits for priority.
Probably not worth it, but it would definetly help if we want to support larger sizes like 1024.

Stan added a subscriber: Stan.Wed, Nov 14, 10:29 AM
Stan added inline comments.
binaries/data/mods/public/maps/random/rmgen/Terrain.js
11

Instead of High and Low being strings might want to use an enum with freeze. Maybe you'll get some perf https://stackoverflow.com/questions/287903/what-is-the-preferred-syntax-for-defining-enums-in-javascript

smiley marked an inline comment as done.Wed, Nov 14, 11:04 AM
smiley added inline comments.
binaries/data/mods/public/maps/random/rmgen/Terrain.js
11

Personally, I dont like the complexity-use ratio of that. But agree for replacing string with a number.
const HIGH = something; is better in my opinion.

Stan added inline comments.Wed, Nov 14, 11:51 AM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
11

Sure. Also if someday we have different blends will be easier to implement :)

smiley marked an inline comment as done.Thu, Nov 15, 2:15 PM
smiley added inline comments.
binaries/data/mods/public/maps/random/rmgen/RandomMap.js
508

Experimented with in P143