Page MenuHomeWildfire Games

Texture priority support for rmgen
AbandonedPublic

Authored by lyv 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.

Test Plan

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

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6675
Build 10991: Vulcan BuildJenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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

lyv 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
27

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.

lyv 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.

lyv 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.

lyv 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

lyv 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

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

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

lyv 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 ;)

lyv 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.

lyv marked 3 inline comments as done.Nov 14 2018, 9:41 AM
lyv 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.Nov 14 2018, 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

lyv marked an inline comment as done.Nov 14 2018, 11:04 AM
lyv 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.Nov 14 2018, 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 :)

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

Experimented with in P143

lyv updated this revision to Diff 7214.Jan 3 2019, 1:48 PM
lyv retitled this revision from [WIP] Texture priority support for rmgen to Texture priority support for rmgen.
lyv edited the summary of this revision. (Show Details)

Fix over/under flows.
Numerical priority.

Vulcan added a comment.Jan 3 2019, 1:49 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/890/

lyv marked an inline comment as done.Jan 3 2019, 1:50 PM
lyv added inline comments.
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

Probably the wrong place. But I could not find a better file.

Stan added inline comments.Jan 3 2019, 1:59 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

Might be better as g_TexturePriorityHigh/Low ?
Might also deserve a comment.

elexis added inline comments.Jan 3 2019, 2:46 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

Why is it 0 and 1 here but u16 when passing it to the engine?
If cpp counterpart supports u16 priorities, why not support them here too and avoid the constants. (no questionmark)
Alternatively one could also introduce a Terrain prototype and add two Terrain.protype.FooPriority constants. That prototype could also be used to store the "Terrain entity" thing D1589. (But just ideas. In both cases it's affecting 70+ random maps)

lyv marked an inline comment as done.Jan 3 2019, 2:49 PM
lyv added inline comments.
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

This is for the function parameter.

elexis added inline comments.Jan 3 2019, 4:28 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

But (same question)?

lyv added inline comments.Jan 3 2019, 4:37 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

Well, I could just use the string "HIGH" and "LOW"

elexis added inline comments.Jan 3 2019, 5:41 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

Why not simply no constants and having each texture define a number if it wants to use priorities (0 otherwise)?

lyv added inline comments.Jan 3 2019, 6:06 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

If one wants to have a texture submissive around other tiles who also are submissive to some tiles, then the user would have to keep track of each of the tile's priorities and select a number which is less than the priority of those tiles. Which could quickly go out of hand when working with multiple tiles close by having different priorities.
Or maybe I am over estimating how much this would be used.

elexis added inline comments.Jan 3 2019, 6:33 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

The only time I noticed this was in #4816. Maybe it could be used at the shoreline or when drawing paths to ensure that the intended border texture is visible.

If we want the priority system, it seems the easiest / most streamlined and most versatile way to mirror the engine as closely as possible.
Users can mess up the priorities sure, but they can do that with boolean priorities too.
Usually there is only one file (the mapscript) that defines the textures at the header, so it should not be terribly hard to keep track of the texture priorities.
A use case for 3 priorities = shoreline, path, ground texture all meeting in one place?

(I still didn't check what the C++ code actually does.)

lyv added inline comments.Jan 3 2019, 6:40 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

What if you want the tile to the right to be rendered on top and the tile next to that and so on. It could go up pretty high technically speaking.

But logically speaking, it won't probably go higher than 3 as you mentioned.

I guess what @FeXoR suggested might work too. Calculating actual values when exporting.

elexis added inline comments.Jan 3 2019, 7:03 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

It can also be 4+ by wanting to paint the path-texture always on top, the shore always on top of ground, the cliff texture always on top of the ground, the X always on top of the Z?

lyv added a comment.Jan 4 2019, 7:13 AM


That's a nice visualization of the numbers involved when using Atlas. Which also raises the question of why I am using a Uint. Atlas seems to be far more sane.

lyv planned changes to this revision.EditedJan 4 2019, 1:48 PM

The current implementation is pretty bad compared to Atlas. Unlike atlas, whats written now would have priority values which are unnecessary and all over the place. Especially when painting a large enough area. The values would have a chain-effect which would keep on incrementing/decrementing.

This at it's current state is pretty much unacceptable.

(why do I feel like I am reviewing this?)

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

Should mirror the engine and make this signed.

elexis added a comment.Jan 4 2019, 3:04 PM

priority values which are unnecessary and all over the place

Default priority to 0 and specifying the priorities per texture (not per vertex) in the mapscript limit the 'data effort' (not) to a feasible level? Or you mean the library functions are spammed with priority occurrences?

chain-effect

What?

Which also raises the question of why I am using a Uint

JS only provides the one JS Number type and typed arrays.
I suppose you wanted the performance benefit.

Notice that if JS supports the exact same priorty types as C++, then we can also load the priorities from the PMP file from rmgen scripts losslessly.
(Hypothetical use case as random maps are should be generated procedurally where possible to actually implement the randomness aspect).

(why do I feel like I am reviewing this?)

(long generic answer)

lyv added a comment.Jan 4 2019, 3:09 PM

What?

Simplest case I could think of; if we wanted to paint a 2x2 square with a low priority, the first tile would have -1, the next one -2 and so on. This is unnecessary when the whole square could just have had -1. (as Atlas does it)

lyv added a comment.Jan 4 2019, 3:10 PM

JS only provides the one JS Number type and typed arrays.
I suppose you wanted the performance benefit.

I meant unsigned as opposed to signed.

FeXoR added inline comments.Jan 5 2019, 7:13 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

smiley said: "then the user would have to keep track of each of the tile's priorities"
That's why I proposed an array where texture priorities are given by the index in the array. The user then could search for the index of a given texture and insert another one before or after it. @elexis proposals like "cliff above other textures" could be realized by pushing them to the end of the list.
The texture IDs (less memory usage?) or the texture objects (less search for what texture relates to which ID so faster?) could go into that list.

(Is it really needed to give "terrains" (texture+entity/actor pairs) priorities? I really think that should be a property of the "terrain only" objects so the same "texture+priority" can be used in multiple "terrain"s.)

FeXoR added a comment.Jan 5 2019, 7:18 PM

Oh, and if a map doesn't want to use texture support they could be just pushed to the list I keep talking about and the resulting behavior will be as it was before this patch ;)

elexis added a comment.Jan 6 2019, 4:00 PM

Perhaps we should go back to the use case analysis first. We don't have an existing map where we notice it's relevant, can we construct a hypothetical use case?
If I recall you started looking into this after I was unable to do paths that are smaller than one tile (which LordGood was presenting using path actor entities)? I don't think we can do anything with priorities to achieve that.
If the priority is linked to the texture definition, some use cases aren't possible anymore (as internally it's linked to a tile coordinate, not a texture ID), for example loading the priorites from PMP files.
The current patch leaves texture priority use open to arbitrary use cases (in case the future reveals some).

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

g_AdjacentCoordinates

FeXoR added a comment.Jan 6 2019, 4:15 PM

I think the usecase for basically all maps is to paint plants/grass or other textures that are "higher"/have a more uneven "surface" above flat textures like e.g. sand. (For that we don't actually need to give the map editor much control about texture priority though.)

Other cases are those @elexis mentioned like cliffs above other textures (not entirely sure why but absolutely possible it's preferable) or similar rules.

To make paths broader or narrower is another one.

Sometimes it would be nice to make the texture priority depend on the height (heightmap) of the tile it's placed on so higher texture are painted above lower ones.

IMO texture priority would depend on (connected) areas rather than textures, but the current system only allows one ID per texture type (otherwise textureToId and IdToTexture wouldn't work properly because not bidirectional). If Atlas supports this (which I thought it does) we should also consider this for random maps.

lyv added a comment.Jan 8 2019, 11:47 AM

Better blending. Generate a map like a African_plains, you would see what I am talking about. Straight lines, squares and whatnot.

The square shape of terrain tiles becomes very noticeable on diagonal shorelines (zig zag pattern, for example rivers map with many teams). If that could be solved with priorities, that would be quite an important fix. Screens?

lyv added a comment.Jan 8 2019, 12:05 PM

The one with the numbers up there show it already. Compare left vs right.

lyv added inline comments.Jan 8 2019, 1:29 PM
binaries/data/mods/public/maps/random/rmgen/RandomMap.js
468

Also, why pretend this doesn't exist. As of now, it's pretty much a useless duplicate of tileIndex. That is if you don't count it's purpose of painting textures in the order defined in the textureID array.
(This seems somewhat related to what fexor had in mind)

binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

I am not sure how that would work. Each tile should have a priority. Not each texture. One could paint "red" in high position at one place and as low on another. I suppose this array should be storing tile coords or something. Which would be way less memory efficent and slower. Int16Array is faster than an Array of Uint16Arrays.
Unless of course, I misread what you said.

FeXoR added inline comments.Jan 8 2019, 2:09 PM
binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

In a texturePriorityList IDs could be stored
In the IDToName array texture strings are stored by texture ID.
If we then use texture IDs before export to handle "textures" we can get the texture string for the texture map from IDToName and the priority for the priority map from texturePriorityList.indexOf(textureID). However, since texture strings won't be unique in the IDToName array any longer it couldn't be used to return an unambiguous ID. So IDToName and "if (texture in this.nameToID)" in getTextureID would have to be removed AFAICS (there are no other usages of that anyways).

I'm not in general opposed to a cell object but if we add it it should be extendable from within a map script (and I think that's not going to work without headaches?). Otherwise I'm for one map for each cell property (like it's now). But in case of texture priority we only need the property at map export so no need to add and update such a map during map generation.

elexis added a comment.Jan 8 2019, 3:02 PM

Observations:

  • CMapReader::ParseTerrain uses std::vector<u16> tilePriority;
  • CMapIO::STileDesc uses u32 m_Priority;
  • maybe atlas just displays it differently.
  • CMapWriter::PackTerrain voodoo looks like it writes the u32

Requirements analysis:

  • To avoid data loss when passing priorities from A to B, it should use the same number of bytes in any case.

Stakeholder analysis!

  • To avoid confusion of code readers, it would be better to use it consistently. But, I recall there was some catch with regards to number types in Atlas, so not sure if it's better to use signed or unsigned consistently, globally, or whether to continue display something else in atlas as long as it stores correctly.
  • To avoid mapmakers having to update all their maps to a new pmp format, pmp files should keep their current format.

Implementation:

  • CMapGeneratorWorker::LoadMapTerrain misses one or two lines for priority support too.
binaries/data/mods/public/maps/random/rmgen/RandomMap.js
486

Unneeded fallback, also unexpected if one didn't read this line but the other priority ones.

binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

Setting the priority as an argument of texture / Terrain creation sounds easy to handle for new maps and probably sufficient to gain the benefit at places where textures overlap?
The downside would be having to change all texture string definitions and library functions expecting strings to be Terrain instances?

The alternative would be to pass the priority to the TerrainPainter, but then it seems easy to run into the confusion troubles you mentioned earlier?

lyv added a comment.Jan 8 2019, 3:09 PM

I never checked the map generation side of things. Only checked it’s type in tile defnition which is int.

elexis added a comment.Jan 8 2019, 3:18 PM

Which definition do you refer to?

lyv added a comment.Jan 8 2019, 3:28 PM

MiniPatch.h which from what I have seen is the lowest level. I.e a single tile.

FeXoR added a comment.Jan 8 2019, 3:53 PM

@elexis I agree with all of that but that's mainly about what's exported.
That priority array I keep talking about is just an IMO easier way to handle the shifting of e.g. one texture above all neighbor textures.
Placement methods can just receive "low" or "high" parameters (the default being high would keep existing maps as they are).

elexis added a comment.Jan 8 2019, 4:53 PM

int is 16 bits or more.

binaries/data/mods/public/maps/random/rmgen/Terrain.js
5

Currently:

const tDirt = "medit_dirt_b";
const tDirt2 = "medit_rocks_grass";
const tDirt3 = "medit_rocks_shrubs";

What I think FeXoR means:

const tDirt = "medit_dirt_b";
const tDirt2 = "medit_rocks_grass";
const tDirt3 = "medit_rocks_shrubs";

const tPriorities = [
 tDirt,
 tDirt2,
 tDirt3
];

But wouldn't this leave the header cleaner?

const tDirt = new Terrain("medit_dirt_b", 0);
const tDirt2 = new Terrain("medit_rocks_grass", 1);
const tDirt3 = new Terrain("medit_rocks_shrubs", 2);

(Just that it would come at the cost of possibly having to update all maps.)

lyv added a comment.EditedJan 8 2019, 4:58 PM

Still not convinced of that model. Priority being a tile property, it should be handled in rmgen as such.

16 bits

I think I had changed it to Int16Array. Although, 16 is the miminum I think.

FeXoR added a comment.EditedJan 9 2019, 1:14 AM

@elexis yes, just I'd use IDs. And we also need a priority "high"/"low" (I'd actually prefer paintBelow True/False, False by default to have unchanged maps).
But scrap that for now since we can use most of the code that is present and only hook things in at two points.
(Won't enable map authors to change the priorities later if placement order does not allow the desired texture order which I hoped for)

When RandomMap.getTextureID is called (e.g. by setTexture as is) that does IDToName[id] = texture with a new ID (always, even if that texture is already in nameToID) and returns it.
When setTexture is called it adds that ID (from above) to the tile in textures (as is) AND the texture priority (high/low) to the new RandomMap.texturePriority array.

So a possible (simplified) state at the end of map generation would be:

IDToName =
[
    medit_dirt_b,
    medit_rocks_grass,
    medit_dirt_b
];
texturePriority =
[
    high,
    high,
    low
];
texture =
[
    [0, 2, 2],
    [0, 1, 2],
    [2, 1, 1]
];

At map export store minPriority and maxPriority number used - 0 for both at start, after that minPriority-- when texture priority is low, maxPriority++ when it's high.
While going through the map IDToName.length times (Urgh! I didn't anticipate that..., guess @smiley did :D) and gather all tiles with that ID and give it the priority minPriority-- if low, maxPriority++ when it's high.

Resulting tileData for export would be:

{
    "index" = 
        [0, 2, 2,
        0, 1, 2,
        2, 1, 1],
    "priority" = 
        [0, -1, -1,
        0, 1, -1
        -1, 1, 1]
};

If the negative values are not supported (I guess) subtract the final minPriority from all numerical priorities for export - since it's negative it will make the numerical priorities start at 0 (and, yes, mind the array type).

EDIT: And nameToID will still become useless because non unique texture strings.

(I could now have written the code myself :p)

To actually make use of that the placement functions obviously have to take priority arguments (high/low or True/False), I guess in another patch. (But at least that wouldn't require to change all existing maps)

@smiley this is just a proposal!
This one has it's flaws for sure: No easy resorting of texture priorities for the map author and cycling IDToName.length times through all tiles won't go unnoticed.
So other ideas welcome :D
If you want to add priorities to the tiles right away that won't save us going through the map IDToName.length times though for it's the order of the texture strings therein that matters.

Hope at least this "stub" is somewhat telling ^^

FeXoR added a comment.Jan 9 2019, 2:09 AM

My last proposal also doesn't work because each tile will get it's own ID and thus priority (which is neither what I wanted nor what would look decent) ;/
(At least, since the texture IDs will then be unique they would be much faster to search for ;D)

And I though I could go without g_AdjacentCoordinates x)

Noone said RandomMap should stop using a 2D array. But the point is about how the user will pass the information to the rmgen library, not how the rmgen library passes it to MapGenerator.cpp.
The rmgen library methods need to consume the priorities somewhere (unless the user would create a mapsize*mapsize texture priority array by hand without knowing what the placers and painters know).
(Compare the described implementation (const tTerrain = new SimpleTerrain("textureName", priority);) to the current texture implementation that also stores things as a 2D array in RandomMap, while the map author specifies them only once in the head of the mapscript file.)
The current revision of the revision proposal only supports priorities for SimpleTerrain; consequentially RandomTerrain, createTerrain, TerrainPainter,paintTerrainBasedOnHeight, LayeredPainter would also need a priority argument each.
If the priority numbers are required to be globally comparable, they would be posted at the head of the mapscript, similar to the height variables that shall be comparable without having lot of code between each number.
Both approaches should be feasible (they are not even mutually exclusive as the priority could be left undefined in the model from my previous comments).
But both approach also have the exact same user freedom and limitations. The texture priority can still only be specified once per painting of a single area! (Unless the user implements a new Painter).
So far

const tTexture1 = new SimpleTerrain("textureName1", 3);
const tTexture2 = new SimpleTerrain("textureName2", 4);

still looks nicer to me than:

const tTexture1 = "textureName1";
const tTexture2 = "textureName2";
const tTexturePriority1 = 3;
const tTexturePriority2 = 4;

and it would minimize the number of arguments passed throughout the entire engine (this also lessens the copying of the texture priority number from function to function, which to my surprise made some mapscripts faster after the Vector2D rewrite).
(Although I guess that may not be more than a dozen of lines or so.)

One edge case to examine would be using the same texture with different priorities.

const tTexture1 = "textureName1";
const tTexturePriority1 = 3;
const tTexturePriority2 = 4;

vs

const tTexture1 = new SimpleTerrain("textureName1", 3);
const tTexture2 = new SimpleTerrain("textureName1", 4);

The two approaches aren't mutually exclusive (leaving a priority undefined to not write to it), just that it would cost code complexity to support both syntaxes in the rmgen library (without increasing the expressiveness / feature richness of the rmgen language).
And implementing the other approach will be a lot of work, rewriting every rmgen script (which may be decisive already).
If we find more weird edge/use cases, one can consider adding new painter classes.

Either way, actually implementing it to be used by maps will yield more experience, information, reasoning for decisions.
(Usually things are always rewritten 3 times anyway until there are no more antipatterns.)

lyv abandoned this revision.Jun 25 2019, 7:53 PM

Status correction. Changes are not planned. (at least not relevant to this status)
Also, this is not the only changes relevant for priority.

lyv added a subscriber: marder.EditedJan 30 2022, 8:25 PM

@marder , perhaps you want to continue this. This has extreme potential to make the maps nicer. Currently random maps cannot mess with texture blending priorities. If this were available, most jagged patches can be eliminated.

Also, this is not the only changes relevant for priority.

I don't remember what I was talking about.

thanks @smiley . It does look like a nice thing to have for sure. Just need to read through the whole diff and find out if this is something I could continue.

marder added a comment.EditedJan 31 2022, 6:13 AM

[comment posted twice- problems with pahbricator]

lyv added a comment.Jan 31 2022, 9:00 PM

I was mostly interested in having another person look at it. As you can see, back when there were multiple rmgen guys around, the chains weren't also really moving smoothly. Although we were reaching a consensus slowly before the almighty stall.

After a long time, my schedule is going to allow for wasting time not too far off, and I am hoping to waste it on the previously mentioned terrain generation thingy.

marder added a comment.Feb 1 2022, 9:01 AM

terrain generation thingy iirc a library that allows visual scripting of random maps? very much looking forward to see that.

For this diff (after reading the discussion):
It would be super helpful to have that, the rectangular terrain blending is really ugly. So it would be very helpful if you also had time to continue this.
Personally I wouldn't mind just going with the list of textures approach.
While it would of course be nice to have the option import maps from atlas and keep the exact same texture priorities, it would already be an improvement to have any form of texture priorities.

lyv added a comment.Jun 28 2022, 4:08 PM

I believe simplifying it rather than allowing for tile by tile priorities is actually the way to go. Which can then fit in nicely with D4680.