-
-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Region based terrain control #1476
Conversation
…rain initialisation via Terrain::InitCityRegions. Starport positions and regions are stored in std::vectors on terrains. Each region contains data to create the underlying terrain.
…ise city regions.
…d return the correct height depending on a form of distance from the city center, and the regionless height. The deviation from the target height is compressed based on the height variation calculated statistically in InitCityRegions. The normal height is blended from the inner boundary to the outer boundary.
…add city region macro. Add example colour region code.
My only question would be why did you make APPLY_SIMPLE_HEIGHT_REGIONS a macro instead of a method or function call? I mean that is a pretty epic macro. |
@fluffyfreak It started off as a trivial region check/blend, and as a way of __forceinlining:) A function would be simpler, there probably wouldn't be any noticeable performance difference or it might get inlined despite being used in a lot of places (my force inline branch showed the benefits were small even for perlin noise). I'll add an inline or __forceinline definition to Terrain. As we were discussing on IRC: flattening an area, to avoid height at edges of large buildings being to different from the center, by reducing the height variation or making it flat, will always make it stand out from the surrounding natural looking areas when those areas have huge height variation. Unfortunately it's the cases like asteroids, where there are massive height variations, where we need flat terrain:)
It's very likely that this area won't get looked at/solved in the short term. It's a case of determining if the quick solution is required/worthwhile for modellers and merging it if need be. |
With the low gravity, I don't think that asteroids should have a pad on which to land. They should have a portal, like orbital stations do, with a docking sequence. |
We can keep this as a backup plan if nothing else can be done. Otherwise I think clever hacks to the core systems should be avoided while it is still possible to rewrite or refactor them. We can afford the wait. |
I think that this solution, as it is, should go in. @Brianetta I don't have a strong feeling one way or the other on this one, both make sense. |
@Luomu I'd prefer this solution that deals with 90% of the problem and already exists, than to wait for a hypothetical solution which might solve 95% of the problems in the future that in all probability will never exist... "We can afford the wait" - seems to be why and how open source projects lose contributors and momentum. |
It should be noted this isn't a clever hack as far as the algorithmic terrain code is concerned - though it might look like it superficially. Any future terrain algorithm (city height maps/algorithmic height definitions/regions set by lua depicting disaster areas etc.) will require this region code to identify areas or the slightly more expensive latitude/longitude version I used in the Mars terrain. The key point here is whether the aesthetic effect justifies solving the terrain on landing pad issue (which is solvable by selecting different buildings on asteroids/checking local height variation on starport placement), or solving very large buildings being covered issue (which doesn't affect building development). Flat regions, even with constrained variation, definitely stand out as artificial when there's a lot of variation. Edit:
Modellers/designers:
also
|
I don't understand why this hasn't been merged yet. It does what we'd want in most cases, it looks like a good basis for building future work on regarding cities or perhaps asteroid specific region definition etc. Towns and Cities certianly do not have natural terrain under them, they may loosely follow the contours of an "original" landscape but they make vast changes. As it stands this patch looks like a good solution. If people have something that they really want to improve about it then I suggest that it gets merged in and that they then do those changes. |
I'm planning to give this PR a full review today - code and effect. I hadn't so far because I didn't (still don't) like the effect visually, but now that there's opposite opinions in the game it deserves closer scrutiny. |
The issue is that when it does work, it works in the best case: star ports are landable:) (Some of the buildings might be submerged but that's a very minor aesthetic issue). Given asteroid starports being submerged can be resolved in a short time frame by creating a asteroid starport on struts, (or as I realised, by probably be trying out a few locations to find a place where the slope isn't too high) it's not as big an issue. It's also possible to disable it for asteroids very trivially heh:). I updated/restructured my branch as I promised mkDanger on IRC that I'd look at it before the next alpha, but the problems involved are side-steppable in the very near future.
The Region Based Terrain Control works and probably will be included when the major planetary features are implemented - vast craters/fracturing spreading over a hemisphere etc.. The art for city floor is for asteroids is what matters and that is out of scope for this pull request within a short time frame. The art requires spending some time collaborating with artists and working on city placement/region code.
The structure and the concept of regions and region data/selection/updating are the important bits, and which will be used for a bunch of things. I was also planning to play around with flatness reduction based on the compression term, maybe even region based fractal roughness control, and also submit a simple asteroid starport placement fix during the freeze. But I'll update this branch to the latest code, if you still want to review it in the very near future. |
… for terrain calculations (Internally non-SSE FP stack 80 bit precision anyway, and converting between floats and doubles takes time in SSE).
I don't view the asteroid area flattening to be a negative though, that's exactly what you'd do on an asteroid, if there was a mountain in the way, you'd remove the mountain for a sufficiently large outpost. We do it on Earth all the time, I just find it to be the most bizarre criticism especially when the proposed solution is as ridiculous as putting a starport / landing-pad on stilts. You could hunt around for a flatter area to place a pad, but what if you can't find one? In the real world we just wouldn't bother, we'd figure out where we wanted the pad, then build up or dig out the landscape to match what we wanted. The pictures you showed seemed to have exactly that approach. In other words, the thing identified as a problem, seems to b the opposite, it looks exactly like the right solution that would really happen if we ever get to building on asteroids in the real world. |
Except we don't flatten an area several miles wide and then plonk a major metropolis on top of it. We grow the city, shaping the land as we go. I'm not necessarily saying this is not a step in the right direction. So far I'm just not convinced that the "right" solution in the end is something that can be built effectively on this model. But as I said, I will review in depth and offer some more specific feedback if I can. I'm working on that right now. |
Ah ok now I see what the objection is, sorry. Well ok no we don't, but then this is only the first implementation? Once it's committed anyone can create a branch and add their own custom method for returning a cities terrain height for use in different situations. When I looked at the code I just figured it was the example type which we'll derive from and put our own city/military-base/prison-complex/LRC-shipyard/etc code in later. |
FWIW I'm with @fluffyfreak on this one. It can be merged, and once we have a form of spaceport that's more suited to an asteroid, this type of terrain modification can be limited to terrestrial planets. I think that asteroids should be functionally more like orbital starports. The lack of strong gravity means that it's easier to dock with, rather than land on, an asteroid. Since that doesn't exist our current metropolis can act as a placeholder. It will mean, ultimately, that we need more starport types than |
Addendum: That shot of the flattened area on Phobos looks like somebody trained a petawatt maser on it for half an hour or so. Pretty cool. |
@fluffyfreak: The issue is that on asteroids it looks clinically flat/monochromatic from a distance, but without the artificial sci-fi detail you would expect on the city floor if it were that man-made. I might be able to fix the colour issue by manipulating the flatness in the colour code. It's mostly likely to be me working on developing it in the medium term (depending on the position in my queue of things which will be moved up if collaborators are working on it), as s20dan is away, so there's not that much benefit of merging if it makes Pioneer look worse for a while. It's also not that hard to merge the branch if anyone wants to work on it. @Brianetta: in a way:) If anyone can describe a suitable simple city floor it might accelerate things. |
(Tweaked version that builds on Linux on Asteroids are not really my concern. Here's our old favourite, Enki Catena: This looks terrible. The ground is flat and untextured, and even the hill looks kind of artificial. I'm not sure if that's part of the flattening at work of if that's just what the hills look like and this one is lucky enough to look a bit different. Either way though, we have really nice terrain and this spoils it. Assuming the code is in a sane state (I'll let you know later tonight) I'm only interested in merging this if there's an obvious plan for how to improve this (doesn't need to be detailed, just needs to give me confidence that we're not going to be stuck with this for the next year).
I had honestly not considered the need for different kinds of flattening algorithms. That makes me even more wary of this. I don't see how this could be done generally without considering the building layout. |
@robn haha that looks awesome, ok maybe not "we should commit this" awesome, but still pretty cool :) Another solution might just be to have to drastically smooth the central city area instead of completely flattening it? That way you'll still have some variation and thus texturing, it'll look a bit more natural and the buildings should mostly stay out of the ground. Could this be achieved by changing the blend function to run across the whole area so that it runs from 0.0 -> 0.5 (50% blended to flat) instead of the current 0.0 -> 1.0 (100% flat)? I guess that might cost more cpu time. Yes I'd expect that in future there might well be different flattening algorithms as people come up with uses for them, don't worry about it in relation to this commit because those future designs or requirements are unknown but it makes sense to me that not only will this version get tweaked but that there would be a place for different choices later on. |
It's not the flattening but the smooth interpolation. The height algorithm currently chooses a target height, keeps variance in a circular region to building scale..then it interpolates from the target height to the height the land has normally.
That's what's being done:) We can further do things like modulate it using fractals etc. but that can be looked at later.
The main thing about this is that there's an extremely fast region identification method. It allows each region to have a type and arbitrary data on how to shape it - parameters, height/colour textures etc.
There's no particular hurry, merging it won't really make a difference to progress and the region stuff will probably get merged as part of 'large terrain features' whenever that is looked at. The only considerations are whether artists work is stopped (we've established it isn't and if it becomes the case we can create a custom solution for buildings or merge this), and if the landing bug is an issue (it isn't much of an issue, and can be side-stepped).
|
Code reviewed. Comments:
Apart from that its a pretty nice bit of code! |
I think a relatively quick fix might be to scale the smoothing with radius. The centre of the city can be scaled to the 100m order, while rapidly moving to the native terrain scale with linear or square proportion (whichever works) to the distance from the centre. This flattens the middle, but doesn't create a visible circle of flattened land. Buildings might still interact badly with the terrain near the edges of the city, but the starport itself won't, because it's always at or near the middle. |
// set up regions which contain the details for region implementation | ||
RegionType rt; | ||
double height = sb->GetRadius()*terrain->GetHeight(pos); | ||
rt.height = height/sb->GetRadius(); // height in planet radii |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're multiplying by sb->GetRadius() in the previous line and dividing by it in this line... that seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I no longer use the one in meters.
My general opinion on this is:
Overall, I would say it's worth merging. If we could make some minor improvements to reduce the weird perfect-flat-circle appearance then that would be great too. I don't have any code notes beyond what robn said and the note I added in the diff. |
My recommendation is to let me play around with tweaking it to look more natural (colours, expanding dynamic range etc.), and not merge for a26.
I'll probably submit a better starport placement fix during the freeze (unless most asteroids are turn out to be too steep everywhere). |
Terrain always needed access to SystemBody to get all the parameters (metallicity, iciness, volcanism etc.). It accesses these in the constructor of Terrain. The reasoning for placing setting up regions under terrains control was that Terrain should know how to shape itself, in much the same way all bodies know how to draw themselves. For that they need access to what ever information they need. Terrain is created in the Geosphere constructor so Geosphere would need to pass that information as well as the sbody pointer.
No, it looks that way as I'm using a lot of const doubles to demonstrate the intermediate stages of the calculation:). It could be condensed into something like 2-3 lines of regular terrain code.
The simplest way to check is to comment out code in Terrain::ApplySimpleHeightRegions and see if there's a difference sitting on the landing pad on Earth:). @Brianetta wrote: The centre of the city can be scaled to the 100m order, while rapidly moving to the native terrain scale with linear or square proportion (whichever works) to the distance from the centre. This is what is done (linear interpolation), but the 'center of the city' is much larger than the city generally as it uses a max city radius value that already exists..these types of things are part of the tweaks i was talking about.. |
Done, in 1503. Hopefully this makes unnecessary that part of the discussion and, the related urgency. |
I've branched this code to here but am closing this PR for now after discussion about how old the PR is and some other changes. |
This pull request implements region based terrain control. As described in detail in issue #924 and outlined in one of the posts in the wiki (including extensions). Closes #7.
Overall I'm now happy with the structure (as I'm now familiar with the relevant surrounding higher level structure) and the terrain algorithm has improved since 927. The terrain should know how to shape itself based on the city parameters (and the city code should know how to shape itself based on lesser shaped terrain, leading to a back and forth between the two in future).
Algorithm:
Considerations:
Notes:
Future extensions:
Phobos: Note blending causes a raised area down-slope and a caved in area up-slope.
Earth:
Further images of earlier code showing terrain with and without regions: http://imgur.com/a/EoFJT