Update CityOnPlanet.cpp #4913

joonicks · 2020-07-12T19:56:05Z

Optimized to use bitfield instead of a byte array for occupancy (5kb vs 40kb).
City size is calculated different and slightly randomized (based on city seed).
Uses precalculated distance-from-center instead of doing the math realtime.
Largest cities on planets with population of 1Billion+.
Mars is a 1B+ planet so Cydonia is now huge. Same with New Hope.

Fixes #4802, fixes #5198.

fluffyfreak · 2020-07-13T12:11:02Z

I'll want to see what effect this has on the number of rendered building, how it might affect framerate etc, but code looks good after a quick look once-over.

impaktor · 2020-07-13T13:08:57Z

I'm not sure I understand what changes have been made here, or why.

Optimized to use bitfield instead of a byte array for occupancy (5kb vs 40kb).

Is code now cleaner and easier to read, or if not, was this code a bottleneck?

City size is calculated different and slightly randomized (based on city seed).

What's the old way, and what's the improvement in outcome?

joonicks · 2020-07-13T13:41:10Z

I'm not sure I understand what changes have been made here, or why.

Optimized to use bitfield instead of a byte array for occupancy (5kb vs 40kb).

Old code would use bytes to store occupancy, resulting in 40kb of memory needing to be cleared for each city generated. With a bitfield the occupancy can be stored in just 5kb.

Is code now cleaner and easier to read, or if not, was this code a bottleneck?

Apart from handling 3d models, clearing the 40kb was without a doubt the biggest operation in the whole routine. Clearing memory isnt just about actually setting it to zero, it also clobbers the data cache so the smaller the better.

As for bit manipulation being readable, I think any programmer uncomfortable with it might not be suitable for game development.

City size is calculated different and slightly randomized (based on city seed).

What's the old way, and what's the improvement in outcome?

int population = planet->GetSystemBody()->GetPopulation();

Old code was based on this line. The problem is, GetPopulation returns population in multiples of 1 billion, so... returns 1.0 for 1 billion, 0.5 for 500million, 0.1 for 100million, etc. Turning it to an int right away means "population" usually ends up as 0. So all cities would be essentially the same size if GetPopulation returns <1, atmospheric cities being one size, airless being the other. Ive written new code (but I dont know how to update the PR) to make city sizes more logarithmic and partially random (based on city seed). 1Billion+ cities are now maximally huge.

impaktor · 2020-07-13T13:51:36Z

OK, thanks for explenation.

If you want to change the last commit, see https://pioneerwiki.com/wiki/Using_git_and_GitHub#Common_fixes_for_mistakes (i.e. amend and force push.)

joonicks · 2020-07-13T15:32:08Z

And to be clear, this optimizing pass was a way for me to familiarize myself with the code again for future implementation of multiblock buildings.

fluffyfreak · 2020-07-13T18:25:19Z

What are MultiBlock buildings?

joonicks · 2020-07-13T20:16:42Z

What are MultiBlock buildings?

cities are created in a 200 by 200 grid, 1 grid cell for each building. but the code supports multi-cell buildings already, the starport uses that code to prevent other buildings to be placed on top of it, but no other building does.

people have changed the cell size to accomodate bigger buildings, but that means more empty space between the smallest buildings. currently the "Library" model even spills over. but im working on changing Library to a multi-cell building so that I can shrink down the cellsize a little again. progress is good, I just finished code to load more building attributes from json.

joonicks · 2020-07-13T22:19:50Z

now support 2x2 buildings, configured in a json file: data/models/buildings/details.json

options that work:
"layout" : 1 -- when placed, allocates 2x2 cells and moves the building to the middle of it
"rarity" : <0.0 ..1.0> -- adjusts the rarity of the building (down only), 0.5 would be 50% less common
going to split this option into "atmosphere-rarity" and "airless-rarity"

not yet functional: storage <true/false>, industry <true/false>, monument <true/false>, habitat <true/false>, x-offset & z-offset

fluffyfreak · 2020-07-15T08:30:17Z

@joonicks is this PR still WIP or is it ready for review?

joonicks · 2020-07-15T12:30:19Z

I have a few more fixes and enhancements in mind at the moment but the main code changes are done I think

joonicks · 2020-07-15T18:11:55Z

ready to be inspected, I think...

src/CityOnPlanet.cpp

fluffyfreak · 2020-07-18T11:12:33Z

src/CityOnPlanet.cpp

+	const int station_z1 = floor(aabb.min.z / cellsize) + citygridmidpoint + 1;
+	const int station_z2 = ceil(aabb.max.z / cellsize) + citygridmidpoint;
+
+	if (stationmodelname == "ground_station") {


Nasty hardcoding this, if we must have tight bounding around certain stations then it prefer there was a list of them in either the config or another json file to control them so that future stations can use it without hardcoding these values

Can't it be defined in the station.jsons themselves for fexibility?

Id like to leave it in until I can figure out the best way to implement it. It would also touch upon more src files to accomplish. Even if I drop dead tomorrow, the hardcoded example shows what can be done, and how.

fluffyfreak · 2020-07-18T11:13:39Z

src/CityOnPlanet.cpp

+	Output("CityOnPlanet: Station '%s' X %i .. %i, Z %i .. %i, pop %.0f cityradius %i\n",
+		stationmodelname, station_x1, station_x2, station_z1, station_z2, pop, cityradius);
+
+	int skipped = 0, placed = 0, emptycells = 0, occupiedcells = 0, loopcells = 0, rarityrolls = 0;


This can stay in even in non-debug, that'll help to get rid of the #ifdef DEBUG_OUTPUT spam later on in the code

Please remove all of the #ifdef DEBUG_OUTPUT tests and simply define these variables normally. The only #ifdef that should remain is the one guarding the actual Output() call.

fluffyfreak · 2020-07-18T11:15:07Z

Mostly good and it works just fine in testing, I've made a couple of suggestions and have some questions but overall good 👍

joonicks · 2020-07-19T13:40:32Z

population config & debug output taken care of...

fluffyfreak · 2020-07-21T11:32:11Z

@joonicks ok I'm alright with this going in with the hardcoded station name for now so long as we fix it later.
You just need to use the Clang-formatter to fix that now and I'll merge it.

impaktor · 2020-07-22T06:35:34Z

ok I'm alright with this going in with the hardcoded station name for now so long as we fix it later.

To my ears, this sounds like something that should be fixed in this PR before merge

joonicks · 2020-07-22T18:22:22Z

ok I'm alright with this going in with the hardcoded station name for now so long as we fix it later.

To my ears, this sounds like something that should be fixed in this PR before merge

I believe the code that loads the .model files should be revisited and rewritten to use json & add support for arbitrary attributes to be loaded, doing away with the details.json which I now realise was a mistake (cant be modded), the ultimate goal being to limit the number of files (collecting all relevant metadata in the same place) and doing away with duplicate code (loading configs from .ini, .model, .json, ...)

however, I do believe the city changes stand well on their own, while the loading of configs could spiral out of control way quicker.

me on my own deciding to put attributes in buildings/default.json is part of the problem. none of you have even realized the modding problem and thats an issue. there should probably be a clear policy on adding files somewhere for all to refer to, before this project gets yet another config file format included in its codebase.

right now I feel that further changes are a bit beyong my own capacity, so to whom it matters, please include it as-is.

fluffyfreak · 2020-09-05T10:45:59Z

@joonicks this is just waiting on you to fix the clang formatting, then I'm happy to merge it in.

Web-eWorks · 2020-09-06T20:03:53Z

@fluffyfreak if you're merging, please be sure to include TODOs as outlined by joonicks' last comment.

Optimized to use bitfield instead of a byte array for occupancy. City size is calculated different and slightly randomized (based on city seed). Largest cities on planets with population of 1Billion+. City sizes are now also slightly random (Based on city seed). Uses precalculated distance-from-center instead of doing the math realtime. Cell size reduced from 80 to 50. Added capability for buildings to span multiple cells (up to 7x7 should work). configure building details in data/models/buildings/details.json working settings include: layout, airless-rarity, atmo-rarity, x-offset, z-offset, industry, habitat (assigned by default) nonfunctional (future use): storage, monument

Web-eWorks · 2021-09-10T03:01:43Z

Will review this once I get a little time, I know it's been sitting open for a while.

joonicks · 2021-11-08T17:41:20Z

Still awating review.

Web-eWorks

Overall I'm fairly satisfied with the results of the updated city generation, although there are several rough spots in the code that need better documentation.

You'll see common refrains in the review comments talking about the parameter space of variables and asking you to explain in a comment why you're doing something a certain way - these are to improve the ability of a contributor to read and understand the code without mentally tracing the entire execution path and variable transforms that you're performing.

Numerous parts of this PR are performing very non-obvious bitwise operations on undocumented data (the bitfield being addressed in most-significant-bit first, big-endian byte order for example) and are not currently sufficiently clear to the average reader as to both what they're doing and why they're doing it that way.

I've also noted a few cases where variable storage and order are making the code inflexible and prone to breakage in future expansion - these mostly come from the C-style data structures used in this PR, and that's an area where I'd like to see some changes before giving this PR the okay to merge.

Other than those problem areas, the code seems to accomplish its goals and clear up a few existing bugs with city generation, so good work!.

Web-eWorks · 2021-11-08T19:41:42Z

src/CityOnPlanet.cpp

@@ -199,64 +284,106 @@ CityOnPlanet::~CityOnPlanet()
 	}
 }

+bool CityOnPlanet::isCityCellsOccupied(uint32_t x, uint32_t z, uint32_t layout)


This function (and setCityCellsOccupied) needs a lengthy documentation comment explaining:

how and why you're choosing to do MSB indexing of the underlying bitfield;

the input space of the parameters (layout in particular will break with a value > 7);

how the testbits variable accomplishes it's conversion of seemingly arbitrary constants into a bitmask for the object's square footprint;

and most importantly in my mind, what guarantee you make that the inner loop will not read off the edge of the Z subarray when it's making a uint16 to index into.

Web-eWorks · 2021-11-08T19:44:51Z

src/CityOnPlanet.cpp

+#ifdef __BIG_ENDIAN__
+		citybits[x + i][z >> 3] |= bits.bytes[0];
+		citybits[x + i][(z >> 3) + 1] |= bits.bytes[1];
+#else
+		// x86/x64
+		citybits[x + i][z >> 3] |= bits.bytes[1];
+		citybits[x + i][(z >> 3) + 1] |= bits.bytes[0];
+#endif


This bitfield is never serialized and is never addressed as a multi-byte datatype of endianness other than host-native; there is no reason to have different behavior here for big-endian CPUs (and it will almost certainly break the bit-test in isCityCellsOccupied).

Web-eWorks · 2021-11-08T19:49:24Z

src/CityOnPlanet.cpp

+	for (int i = 0; i < 5; i++) {
+		if (pop > population2radius[i]) {
+			cityradius = (atmo * cityradius_mod[i]) + rand.Int32(cityradius_rnd[i]);
+			break;
+		}
+	}


Please define these related parameters as a single array of structs (and similarly load them from JSON as an array of objects). It's better for cache occupancy, and avoids duplicating fragile hardcoded array sizes across the code.

Web-eWorks · 2021-11-08T19:51:58Z

src/CityOnPlanet.cpp

+		assert(cellsize_i == 50); // this block is specifically tuned to cellsize 50.
+		int z = (citygridmidpoint >> 3);
+		citybits[citygridmidpoint + 0][z] |= (64 + 32 + 16 + 8 + 4 + 2 + 1);
+		citybits[citygridmidpoint - 1][z] |= (64 + 32 + 16 + 8 + 4 + 2 + 1);
+		citybits[citygridmidpoint + 1][z] |= (64 + 32 + 16 + 8 + 4 + 2 + 1);
+		citybits[citygridmidpoint - 2][z] |= (64 + 32 + 16 + 8 + 4 + 2 + 1);
+		citybits[citygridmidpoint + 2][z] |= (64 + 32 + 16 + 8 + 4 + 2 + 1);
+		citybits[citygridmidpoint - 3][z] |= (32 + 16 + 8 + 4 + 2);
+		citybits[citygridmidpoint + 3][z] |= (32 + 16 + 8 + 4 + 2);


Please replace the multiple additions with hexadecimal constants (or bit constants if you want to visually see the "cell" definition) as this is confusing and obscures the point of this block. Additionally, this block needs a comment explaining both what you're doing (the purpose behind the array offset constants) and the input space of the ground_station mesh that translates into this particular sequence of bit updates.

Web-eWorks · 2021-11-08T19:53:36Z

src/CityOnPlanet.cpp

+	Output("CityOnPlanet: Station '%s' X %i .. %i, Z %i .. %i, pop %.0f cityradius %i\n",
+		stationmodelname, station_x1, station_x2, station_z1, station_z2, pop, cityradius);
+
+	int skipped = 0, placed = 0, emptycells = 0, occupiedcells = 0, loopcells = 0, rarityrolls = 0;


Please remove all of the #ifdef DEBUG_OUTPUT tests and simply define these variables normally. The only #ifdef that should remain is the one guarding the actual Output() call.

Web-eWorks · 2021-11-08T20:07:23Z

src/CityOnPlanet.cpp

+		list->buildings[i].index = i;
+		list->buildings[i].layout = 0;
+		list->buildings[i].x_offset = 0;
+		list->buildings[i].z_offset = 0;
+		list->buildings[i].airless_rarity = 1.0;
+		list->buildings[i].atmo_rarity = 1.0;
+		list->buildings[i].storage = false;
+		list->buildings[i].industry = false;
+		list->buildings[i].monument = false;
+		list->buildings[i].habitat = true;


Please move these initializations to a struct constructor (or C++11 variable initializers) instead of hardcoding it here in this loop.

Web-eWorks · 2021-11-08T20:09:44Z

src/CityOnPlanet.cpp

+		population2radius[0] = data["global"].value("pop-0", 1000000000.0);
+		population2radius[1] = data["global"].value("pop-1", 200000000.0);
+		population2radius[2] = data["global"].value("pop-2", 20000000.0);
+		population2radius[3] = data["global"].value("pop-3", 1000000.0);
+		population2radius[4] = 0.0;
+
+		cityradius_mod[0] = data["global"].value("mod-0", 1.0);
+		cityradius_mod[1] = data["global"].value("mod-1", 0.8);
+		cityradius_mod[2] = data["global"].value("mod-2", 0.6);
+		cityradius_mod[3] = data["global"].value("mod-3", 0.4);
+		cityradius_mod[4] = data["global"].value("mod-4", 0.2);
+
+		cityradius_rnd[0] = data["global"].value("rnd-0", 30);
+		cityradius_rnd[1] = data["global"].value("rnd-1", 15);
+		cityradius_rnd[2] = data["global"].value("rnd-2", 14);
+		cityradius_rnd[3] = data["global"].value("rnd-3", 12);
+		cityradius_rnd[4] = data["global"].value("rnd-4", 10);


Similarly to the prior comment, please prefer loading this from JSON as an array of objects into an array of structs, rather than a manually-suffixed set of fields. If the defaults are significant, please move them to a code comment on the appropriate struct definition and/or fill any missing fields in the data file.

Web-eWorks · 2021-11-08T20:10:05Z

src/CityOnPlanet.cpp

+	if (config_atmo_size > 68 || config_atmo_size < 5)
+		config_atmo_size = DEFAULT_ATMO_SIZE;
+	if (config_airless_size > 68 || config_airless_size < 5)
+		config_airless_size = DEFAULT_AIRLESS_SIZE;


The parameter space of these constants requires an explanatory comment.

Web-eWorks · 2021-11-08T20:11:17Z

src/CityOnPlanet.cpp

 	int skipMask;
-	switch (Pi::detail.cities) {
-	case 0: skipMask = 0xf; break;
-	case 1: skipMask = 0x7; break;
-	case 2: skipMask = 0x3; break;
-	case 3: skipMask = 0x1; break;
-	default:
+	if (Pi::detail.cities >= 0 && Pi::detail.cities <= 3)
+		skipMask = (16 >> Pi::detail.cities) - 1;
+	else


I would appreciate it if you would add a comment here explaining the higher-level purpose of the skip mask for easier reading of this function.

Web-eWorks · 2021-11-08T20:16:22Z

src/CityOnPlanet.cpp

+static const char *CITY_CONFIG_FILE = "models/buildings/details.json";
+static uint8_t citydistancegrid[CityOnPlanet::citygridlimit][CityOnPlanet::citygridlimit];
+static double population2radius[5];
+static double cityradius_mod[5];
+static uint32_t cityradius_rnd[5];
+
+int simplebuildings[50];
+int numberofSimplebuildings;


These variables should all be static class variables.

joonicks · 2021-11-08T21:07:55Z

Jesus. That went from "just fix the clang" to "do total rewrite" real fast. You know what? Never mind.

Gliese852 · 2021-11-08T21:37:12Z

@joonicks come on, it's not that much to rewrite

fluffyfreak · 2021-11-09T21:42:50Z

@joonicks it's a few comments to explain stuff that is difficult to understand, one bit that needs clarifying how it avoids overflowing the data-type, and changing some arrays to structs.

That's a very mild set of requests.

joonicks · 2021-11-10T14:13:07Z

@fluffyfreak @Gliese852
Request 1: "lengthy documentation"
Request 2: unknown - jargon overload instead of plain english
Request 3: rewrite
Request 4: rewrite
Request 5: remove #ifdefs so that compiler throws multiple unused variable warnings instead
Request 6: minor
Request 7: minor
Request 8: documentation
Request 9: not sure
Request 10: rewrite
Request 11: rewrite
Request 12: documentation
Request 13: rewrite

OmarAglan · 2022-11-14T06:12:31Z

@joonicks @fluffyfreak @impaktor @Web-eWorks @Gliese852
any Update on This PR?

impaktor · 2022-11-14T10:07:25Z

@Omaranwa yes, the other day @Web-eWorks contemplated on reviving this, (rebase to master, and implement suggested changes above), but if you're looking to do it, that would also be great :)

Web-eWorks · 2022-11-14T21:45:21Z

I'm about 40% done with a complete rewrite of this PR (the underlying algorithm being very similar, but the expression of said algorithm entirely different) to address some of the fundamental issues that were mentioned during review process, as well as dealing with the architectural and code-style issues. As joonicks has indicated he has no desire to make further changes to this PR, that work will be merged via a new pull request when completed.

OmarAglan · 2022-11-17T13:18:27Z

@impaktor @Web-eWorks thanks
looking for it

OmarAglan · 2022-11-17T13:26:24Z

@joonicks as for this

@Omaranwa yes, the other day @Web-eWorks contemplated on reviving this, (rebase to master, and implement suggested changes above), but if you're looking to do it, that would also be great :)

I have other plans to implement more features in the graphic area.

fluffyfreak requested review from Web-eWorks and fluffyfreak July 13, 2020 12:11

joonicks force-pushed the CityOnPlanet-review-2020 branch 3 times, most recently from 2e87dcd to 18b2484 Compare July 13, 2020 15:16

joonicks force-pushed the CityOnPlanet-review-2020 branch 2 times, most recently from 5ca9790 to 99095a9 Compare July 14, 2020 23:32

impaktor marked this pull request as draft July 15, 2020 14:50

joonicks force-pushed the CityOnPlanet-review-2020 branch from 99095a9 to 60fc4ce Compare July 15, 2020 18:09

fluffyfreak reviewed Jul 18, 2020

View reviewed changes

src/CityOnPlanet.cpp Outdated Show resolved Hide resolved

fluffyfreak reviewed Jul 18, 2020

View reviewed changes

joonicks force-pushed the CityOnPlanet-review-2020 branch from 60fc4ce to f4aa635 Compare July 19, 2020 13:39

fluffyfreak marked this pull request as ready for review July 21, 2020 11:32

Web-eWorks force-pushed the master branch from 3c682cf to 5443bb9 Compare November 20, 2020 23:17

impaktor force-pushed the master branch from b92b389 to ffcf3bb Compare June 27, 2021 10:32

joonicks force-pushed the CityOnPlanet-review-2020 branch from f4aa635 to 97e783a Compare September 5, 2021 14:02

joonicks added 5 commits September 5, 2021 16:07

clang formatting

deb86c2

clang whitespace police

7d81ad0

clang whitespace nazi

6eef54e

something changed in the past year

df2e77e

missing default values

0bdaf19

This was referenced Sep 24, 2021

Number of buildings in city depends on previously loaded game #4802

Closed

Cities are not always visible #5198

Closed

Web-eWorks requested changes Nov 8, 2021

View reviewed changes

impaktor force-pushed the master branch 3 times, most recently from 876b6fc to b5bc47c Compare July 6, 2022 13:11

Web-eWorks mentioned this pull request Nov 27, 2022

Rewritten and Improved City Generation for Spaceports #5430

Merged

Web-eWorks closed this in #5430 Dec 2, 2022

Update CityOnPlanet.cpp #4913

Update CityOnPlanet.cpp #4913

Conversation

joonicks commented Jul 12, 2020 • edited by Web-eWorks

fluffyfreak commented Jul 13, 2020

impaktor commented Jul 13, 2020

joonicks commented Jul 13, 2020

impaktor commented Jul 13, 2020

joonicks commented Jul 13, 2020

fluffyfreak commented Jul 13, 2020

joonicks commented Jul 13, 2020

joonicks commented Jul 13, 2020

fluffyfreak commented Jul 15, 2020

joonicks commented Jul 15, 2020

joonicks commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluffyfreak commented Jul 18, 2020

joonicks commented Jul 19, 2020

fluffyfreak commented Jul 21, 2020

impaktor commented Jul 22, 2020

joonicks commented Jul 22, 2020

fluffyfreak commented Sep 5, 2020

Web-eWorks commented Sep 6, 2020

Web-eWorks commented Sep 10, 2021

joonicks commented Nov 8, 2021

Web-eWorks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joonicks commented Nov 8, 2021

Gliese852 commented Nov 8, 2021

fluffyfreak commented Nov 9, 2021

joonicks commented Nov 10, 2021

OmarAglan commented Nov 14, 2022

impaktor commented Nov 14, 2022

Web-eWorks commented Nov 14, 2022

OmarAglan commented Nov 17, 2022

OmarAglan commented Nov 17, 2022

joonicks commented Jul 12, 2020 •

edited by Web-eWorks