Page MenuHomeWildfire Games

Unify EdgeTile count in ICmpRangeManager, CCmpPathfinder, MapGenerator
ClosedPublic

Authored by elexis on Jul 11 2019, 3:07 AM.
Tags
None
Subscribers
Restricted Owners Package
Restricted Owners Package
Restricted Owners Package
Tokens
"Like" token, awarded by Silier.

Details

Summary

This constant is used in three different places (ICmpRangeManager, CCmpPathfinder, MapGenerator), in the future a fourth (Territory), it should be set only once to avoid the files getting out of sync.

That was the variant with the constant in pathfinder.xml:

Test Plan

Make sure that this is the most logical place to be stored (pathfinder, rangemanager, terrain, territory, lostexture, ...), see also #0ad-dev discussion today (2019-07-10).
Decide whether it is necessary to move this from a C++ constant to a XML/JSON constant to gain moddability (It seems there is no incentive to increase the shroud of darkness size other than changing the LOS texture blurring, which in turn would require changing a C++ const again).

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jul 11 2019, 3:07 AM
elexis updated the Trac tickets for this revision.Jul 11 2019, 3:08 AM
elexis edited the summary of this revision. (Show Details)Jul 11 2019, 3:10 AM
elexis updated the Trac tickets for this revision.

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpRangeManager.h
|  69| class·ICmpRangeManager·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpRangeManager:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...

binaries/data/mods/public/maps/random/rmgen/library.js
|  70| »   »   »   »   Object.keys(obstruction.Obstructions).reduce((width,·key)·=>·width·+·+obstruction.Obstructions[key]["@width"],·0))·:
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/27/display/redirect

wraitii added a subscriber: wraitii.EditedJul 11 2019, 8:36 AM

I think this is a reasonable place to put it.
I have one argument for putting it in a helper file that contains only that: it would make it slightly faster to parse for files that only need the constant, and if we change the range manager's interface it won't require recompiling 4 other cpp files.

edit: nor changes to the files iCMpRangeManager includes, which are

#include "maths/FixedVector3D.h"
#include "maths/FixedVector2D.h"

#include "simulation2/system/Interface.h"
#include "simulation2/helpers/Position.h"
#include "simulation2/helpers/Player.h"

#include "graphics/Terrain.h" // for TERRAIN_TILE_SIZE

So overall moving this constant to its own file seems like a good idea to me for (mostly iterative) compilation times and reducing interdependencies.

simulation2/helpers/EdgeTiles.h?

CinemaPath.cpp Geometry.h HierarchicalPathfinder.h Pathfinding.cpp.2.svnpatch.rej PathGoal.cpp Position.h Rasterize.h Selection.cpp Spatial.h
CinemaPath.h Grid.h LongPathfinder.cpp PathGoal.h PriorityQueue.h Render.cpp Selection.h VertexPathfinder.cpp
Geometry.cpp HierarchicalPathfinder.cpp LongPathfinder.h Pathfinding.h Player.h Rasterize.cpp Render.h SimulationCommand.h VertexPathfinder.h

Bit weird for only one line, but Player.h set the precedent; static const ssize_t EDGE_TILES = 3; then I suppose.

source/simulation2/components/ICmpRangeManager.h
77 ↗(On Diff #8829)

ssize_t, int, i32 > u8

elexis updated this revision to Diff 8835.Jul 11 2019, 11:01 PM

Move to EdgeTiles.h

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/maps/random/rmgen/library.js
|  70| »   »   »   »   Object.keys(obstruction.Obstructions).reduce((width,·key)·=>·width·+·+obstruction.Obstructions[key]["@width"],·0))·:
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/32/display/redirect

I think it's cleaner that way: the constant does not have an obvious owner and it lets us only include what we need to use.
It might be worth naming this "MapEdgeTiles" instead of just EdgeTiles - your call.

elexis updated this revision to Diff 8845.Jul 12 2019, 5:52 PM

Rename to MapEdgeTiles

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unexpected space after unary operator '-'.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|  27|  27|  * Constants needed for heightmap_manipulation.js
|  28|  28|  */
|  29|  29| const MAX_HEIGHT_RANGE = 0xFFFF / HEIGHT_UNITS_PER_METRE; // Engine limit, Roughly 700 meters
|  30|    |-const MIN_HEIGHT = - SEA_LEVEL;
|    |  30|+const MIN_HEIGHT = -SEA_LEVEL;
|  31|  31| 
|  32|  32| /**
|  33|  33|  * Length of one tile of the terrain grid in metres.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|  64|  64| 		obstruction.Static ?
|  65|  65| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  66|  66| 		// Used for gates, should consider the position too
|  67|    |-		obstruction.Obstructions ?
|    |  67|+			obstruction.Obstructions ?
|  68|  68| 			new Vector2D(
|  69|  69| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  70|  70| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|  65|  65| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  66|  66| 		// Used for gates, should consider the position too
|  67|  67| 		obstruction.Obstructions ?
|  68|    |-			new Vector2D(
|    |  68|+				new Vector2D(
|  69|  69| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  70|  70| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  71|  71| 			new Vector2D(0, 0);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|  66|  66| 		// Used for gates, should consider the position too
|  67|  67| 		obstruction.Obstructions ?
|  68|  68| 			new Vector2D(
|  69|    |-				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|    |  69|+					Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  70|  70| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  71|  71| 			new Vector2D(0, 0);
|  72|  72| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|  67|  67| 		obstruction.Obstructions ?
|  68|  68| 			new Vector2D(
|  69|  69| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  70|    |-				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    |  70|+					Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  71|  71| 			new Vector2D(0, 0);
|  72|  72| 
|  73|  73| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|  68|  68| 			new Vector2D(
|  69|  69| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  70|  70| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  71|    |-			new Vector2D(0, 0);
|    |  71|+				new Vector2D(0, 0);
|  72|  72| 
|  73|  73| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|  74|  74| }
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '/*' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
| 244| 244| /**
| 245| 245|  * Create an avoid constraint for the given classes by the given distances
| 246| 246|  */
| 247|    |-function avoidClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 247|+function avoidClasses(/* class1, dist1, class2, dist2, etc*/)
| 248| 248| {
| 249| 249| 	let ar = [];
| 250| 250| 	for (let i = 0; i < arguments.length/2; ++i)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '/*' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
| 260| 260| /**
| 261| 261|  * Create a stay constraint for the given classes by the given distances
| 262| 262|  */
| 263|    |-function stayClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 263|+function stayClasses(/* class1, dist1, class2, dist2, etc*/)
| 264| 264| {
| 265| 265| 	let ar = [];
| 266| 266| 	for (let i = 0; i < arguments.length/2; ++i)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '/*' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/rmgen/library.js
| 276| 276| /**
| 277| 277|  * Create a border constraint for the given classes by the given distances
| 278| 278|  */
| 279|    |-function borderClasses(/*class1, idist1, odist1, class2, idist2, odist2, etc*/)
|    | 279|+function borderClasses(/* class1, idist1, odist1, class2, idist2, odist2, etc*/)
| 280| 280| {
| 281| 281| 	let ar = [];
| 282| 282| 	for (let i = 0; i < arguments.length/3; ++i)

binaries/data/mods/public/maps/random/rmgen/library.js
|  70| »   »   »   »   Object.keys(obstruction.Obstructions).reduce((width,·key)·=>·width·+·+obstruction.Obstructions[key]["@width"],·0))·:
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/41/display/redirect

wraitii accepted this revision.Jul 12 2019, 6:04 PM

LGTM, compiles, runs, sensible as said above. The #undef is a good practice and it's nice to add it.

This revision is now accepted and ready to land.Jul 12 2019, 6:04 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.Jul 12 2019, 6:39 PM