{"comment":{"created_at":"2011-10-20T22:31:07Z","body":"Looks good, only a few issues that I found. Some colours changed which can be worked around, I was surprised to see how many planets remain almost identical to before.\r\n Gasgiants are all using Saturn colour scheme.\r\n Star terrains seem messed up, wrong colour selections perhaps but they dont seem right, Ill look into it.","updated_at":"2011-10-20T22:31:07Z","url":"https://api.github.com/repos/pioneerspacesim/pioneer/issues/comments/2475344","id":2475344,"user":{"avatar_url":"https://secure.gravatar.com/avatar/9874830be0fabe310f0614ea9f5e690c?d=https://a248.e.akamai.net/assets.github.com%2Fimages%2Fgravatars%2Fgravatar-140.png","gravatar_id":"9874830be0fabe310f0614ea9f5e690c","url":"https://api.github.com/users/s20dan","id":457110,"login":"s20dan"}},"action":"created","issue":{"number":699,"created_at":"2011-10-20T11:07:04Z","pull_request":{"diff_url":"https://github.com/pioneerspacesim/pioneer/pull/699.diff","patch_url":"https://github.com/pioneerspacesim/pioneer/pull/699.patch","html_url":"https://github.com/pioneerspacesim/pioneer/pull/699"},"body":"Refactor of `GeoSphereStyle`. My goals here are as follows:\r\n\r\n* Remove the giant height/colour type switches in `GetHeight()` and `GetColor()`. There is no point doing this tests on every call as they never ever change.\r\n* Move individual height/colour fractals and their initialisers out into their own files for easier maintenance.\r\n* Do not do anything that will impact on performance (this is extremely heavily-used code).\r\n\r\nI've built a set of templates to allow a class to be composed of the wanted height/color fractal combo. The idea being that when you request a terrain of a particular type you get only that terrain and nothing else.\r\n\r\nThe `HeightFractal` and `ColorFractal` classes are not completely self-contained. They each depend on a bunch of shared data, most notably `m_fracdef`. I could have put this out into a separate object but I didn't want the additional overhead to access its data. Instead I've gone for a kind of composition using virtual inheritance to get a common base object containing the needed data. `GetHeight()` and `GetColor()` are pure; my understanding is that the compiler can optimise this to something very close to a direct call. Even if it can't though, it should still end up (probably imperceptably) faster than the switch and less convoluted than a separate data object.\r\n\r\nAtmosphere properties (`PickAtmosphere()` and `GetAtmosphereFlavor()`) have been moved to `SBody`, as they are entirely dependent on body type.\r\n\r\nI've deliberately not touched the actual algorithmic code. There's some reshuffling that can be done but its really just deckchairs until I properly understand the algorithms.\r\n\r\nThere's a little bit of cleanup work that could happen in `TerrainNoise.h`, but its all pretty low impact. I couldn't decide if the function names in `TerrainNoise` and `TerrainFeature` should be renamed to be more consistent with the rest of the code. I opted to leave it for now as its all fairly self-contained and it made it easier to just copy the code over.\r\n\r\nI've tested by looking at a few objects in Sol and Eridani. Star and planet terrains look right, atmospheres appear to have the right colours, heightmaps are working. Detail changes appear to work. It might be worth doing a closer check if you're so inclined just to make sure, but it feels right.\r\n\r\nCloses #54.","comments":1,"title":"Terrain refactor","updated_at":"2011-10-20T22:31:07Z","url":"https://api.github.com/repos/pioneerspacesim/pioneer/issues/699","id":2003666,"assignee":null,"milestone":null,"closed_at":null,"user":{"avatar_url":"https://secure.gravatar.com/avatar/e0111ec54e2ff06c4661eda1a0585f9f?d=https://a248.e.akamai.net/assets.github.com%2Fimages%2Fgravatars%2Fgravatar-140.png","gravatar_id":"e0111ec54e2ff06c4661eda1a0585f9f","url":"https://api.github.com/users/robn","id":130670,"login":"robn"},"html_url":"https://github.com/pioneerspacesim/pioneer/issues/699","labels":[{"name":"Patch supplied","url":"https://api.github.com/repos/pioneerspacesim/pioneer/labels/Patch+supplied","color":"ff00f2"}],"state":"open"},"legacy":{"issue_id":2003666,"comment_id":2475344}}