pioneer
pioneer copied to clipboard
pioneerspacesim
Update CityOnPlanet.cpp
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.
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.
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?
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.
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.)
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.
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.
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
I have a few more fixes and enhancements in mind at the moment but the main code changes are done I think
Mostly good and it works just fine in testing, I've made a couple of suggestions and have some questions but overall good 👍
@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.
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
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.
@joonicks this is just waiting on you to fix the clang formatting, then I'm happy to merge it in.
@fluffyfreak if you're merging, please be sure to include TODOs as outlined by joonicks' last comment.
Will review this once I get a little time, I know it's been sitting open for a while.
Jesus. That went from "just fix the clang" to "do total rewrite" real fast. You know what? Never mind.
@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.
@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
@joonicks @fluffyfreak @impaktor @Web-eWorks @Gliese852 any Update on This PR?
@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'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.
@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.