Page MenuHomeWildfire Games
Differential D2068

Cleanup MapGenerator ScriptInterface
ClosedPublic
Actions

Authored by elexis on Jul 13 2019, 2:40 PM.

Details

Reviewers
None
Commits
rP22466: Cleanup MapGenerator.cpp, use VfsPath, create RegisterScriptFunctions, add…
Trac Tickets
#4034
#4964
Summary
  • Move RegisterScriptFunctions, which is a commonpractice function, compare with JSInterface*.cpp files.
  • Use VfsPath directly instead of std::wstring and std::string.
  • Add comments to the header.
  • Revise rP20504 and register the global TERRAIN_TILE_SIZE directly. It removes the informative aspect of the JS function to the JS developer, but it doesn't require code to add comments.
Test Plan

The VfsPath diff can be changed by loading the red sea map (PNG file), the jebel barkal map (PMP file). Should be done on multiple platforms. Read whether the header comments are accurate. Find a philosophy on future engine global documentation.

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 13 2019, 2:40 PM
Vulcan added a comment.Jul 13 2019, 2:44 PM

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

Linter detected issues:
Executing section Source...

source/graphics/MapGenerator.h
|  40| class·CMapGenerator
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCMapGenerator{' is invalid C code. Use --std or --language to configure the language.
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| const MAX_HEIGHT = MAX_HEIGHT_RANGE - SEA_LEVEL;
|  33|  33| 
|    | [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
|  58|  58| 		obstruction.Static ?
|  59|  59| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  60|  60| 		// Used for gates, should consider the position too
|  61|    |-		obstruction.Obstructions ?
|    |  61|+			obstruction.Obstructions ?
|  62|  62| 			new Vector2D(
|  63|  63| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  64|  64| 				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
|  59|  59| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  60|  60| 		// Used for gates, should consider the position too
|  61|  61| 		obstruction.Obstructions ?
|  62|    |-			new Vector2D(
|    |  62|+				new Vector2D(
|  63|  63| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  64|  64| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  65|  65| 			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
|  60|  60| 		// Used for gates, should consider the position too
|  61|  61| 		obstruction.Obstructions ?
|  62|  62| 			new Vector2D(
|  63|    |-				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|    |  63|+					Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  64|  64| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  65|  65| 			new Vector2D(0, 0);
|  66|  66| 
|    | [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
|  61|  61| 		obstruction.Obstructions ?
|  62|  62| 			new Vector2D(
|  63|  63| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  64|    |-				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    |  64|+					Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  65|  65| 			new Vector2D(0, 0);
|  66|  66| 
|  67|  67| 	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
|  62|  62| 			new Vector2D(
|  63|  63| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  64|  64| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  65|    |-			new Vector2D(0, 0);
|    |  65|+				new Vector2D(0, 0);
|  66|  66| 
|  67|  67| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|  68|  68| }
|    | [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
| 238| 238| /**
| 239| 239|  * Create an avoid constraint for the given classes by the given distances
| 240| 240|  */
| 241|    |-function avoidClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 241|+function avoidClasses(/* class1, dist1, class2, dist2, etc*/)
| 242| 242| {
| 243| 243| 	let ar = [];
| 244| 244| 	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
| 254| 254| /**
| 255| 255|  * Create a stay constraint for the given classes by the given distances
| 256| 256|  */
| 257|    |-function stayClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 257|+function stayClasses(/* class1, dist1, class2, dist2, etc*/)
| 258| 258| {
| 259| 259| 	let ar = [];
| 260| 260| 	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
| 270| 270| /**
| 271| 271|  * Create a border constraint for the given classes by the given distances
| 272| 272|  */
| 273|    |-function borderClasses(/*class1, idist1, odist1, class2, idist2, odist2, etc*/)
|    | 273|+function borderClasses(/* class1, idist1, odist1, class2, idist2, odist2, etc*/)
| 274| 274| {
| 275| 275| 	let ar = [];
| 276| 276| 	for (let i = 0; i < arguments.length/3; ++i)

binaries/data/mods/public/maps/random/rmgen/library.js
|  64| »   »   »   »   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/49/display/redirect

Harbormaster completed remote builds in B8320: Diff 8856.Jul 13 2019, 2:44 PM
elexis added a comment.Jul 13 2019, 7:53 PM

On the VfsPath:

  • RegisterScriptFunction using VfsPath would be done consistently for multiple JS interface files, in particular for replays, this diff would only be one of them.
  • The pattern has been in use so far for LoadMapSettings which loads the XML map settings JSON data.
  • It is supported code-wise because the conversion explicitly exists in FromJSVal<Path> in ScriptConversions.cpp and in vfs_path.h: typedef Path VfsPath.
  • Tested on arch with gcc 9 and win 10 / vs 2015 in accordance with test plan.
  • On the tested platforms, using a non-ASCII filename such as african_plains?.js or fields_of_meroë.js did not work (file not found), not before nor after the patch. Using VfsPath will rather make it easier if all calls use VfsPath rather than all calls converting manually from wstring to VfsPath.
historic_bruno added a subscriber: historic_bruno.EditedJul 13 2019, 8:17 PM

I can give a little historical context here. Once upon a time VfsPath was:

typedef std::wstring VfsPath;

Two days after I committed rmgen (rP9096), janwas committed rP9107 which changed VfsPath to what we now know:

typedef Path VfsPath;

This is a good change, VfsPath is far more robust than simple strings :)

elexis added a comment.Jul 13 2019, 8:28 PM

May I use the phrasing "VfsPath change accepted by historic bruno" in the commit message?

historic_bruno added a comment.Jul 13 2019, 9:56 PM
In D2068#86242, @elexis wrote:

May I use the phrasing "VfsPath change accepted by historic bruno" in the commit message?

Sure!

This revision was not accepted when it landed; it landed in state Needs Review.Jul 13 2019, 11:41 PM
Closed by commit rP22466: Cleanup MapGenerator.cpp, use VfsPath, create RegisterScriptFunctions, add… (authored by elexis). · Explain Why
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jul 13 2019, 11:41 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
maps/
random/
rmgen/
6 lines
source/
graphics/
107 lines
72 lines

Diff 8875

View Options

ps/trunk/binaries/data/mods/public/maps/random/rmgen/library.js

Loading...
View Options

ps/trunk/source/graphics/MapGenerator.h

Loading...
View Options

ps/trunk/source/graphics/MapGenerator.cpp

Loading...
Wildfire Games · Phabricator