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.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 6324 Build 10493: Vulcan Build Jenkins Build 10492: arc lint + arc unit
Event Timeline
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 |
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. |
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.
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.
Use two typed arrays which are more performant and return texture string in getTexture.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/712/display/redirect
binaries/data/mods/public/maps/random/rmgen/RandomMap.js | ||
---|---|---|
19 | Just noticed the singluar and plural variable names, will fix with next update |
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. |
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
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 ;)
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.
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. |
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 |
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. |
binaries/data/mods/public/maps/random/rmgen/Terrain.js | ||
---|---|---|
11 | Sure. Also if someday we have different blends will be easier to implement :) |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/890/
binaries/data/mods/public/maps/random/rmgen/Terrain.js | ||
---|---|---|
5 | Probably the wrong place. But I could not find a better file. |
binaries/data/mods/public/maps/random/rmgen/Terrain.js | ||
---|---|---|
5 | Might be better as g_TexturePriorityHigh/Low ? |
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? |
binaries/data/mods/public/maps/random/rmgen/Terrain.js | ||
---|---|---|
5 | This is for the function parameter. |
binaries/data/mods/public/maps/random/rmgen/Terrain.js | ||
---|---|---|
5 | But (same question)? |
binaries/data/mods/public/maps/random/rmgen/Terrain.js | ||
---|---|---|
5 | Well, I could just use the string "HIGH" and "LOW" |
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)? |
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. |
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. (I still didn't check what the C++ code actually does.) |
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. |
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? |
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.
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. |
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)
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)
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.
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" (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.) |
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 ;)
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 |
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.
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?
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. | |
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. |
binaries/data/mods/public/maps/random/rmgen/Terrain.js | ||
---|---|---|
5 | In a texturePriorityList IDs could be stored 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. |
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 alternative would be to pass the priority to the TerrainPainter, but then it seems easy to run into the confusion troubles you mentioned earlier? |
I never checked the map generation side of things. Only checked it’s type in tile defnition which is int.
@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).
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.) |
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.
@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 ^^
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.)
Status correction. Changes are not planned. (at least not relevant to this status)
Also, this is not the only changes relevant for priority.
@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.
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.
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.
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.