Opened 5 years ago

Last modified 3 years ago

#2264 new enhancement

[PATCH] Implement terrain flattening

Reported by: sanderd17 Owned by:
Priority: Nice to Have Milestone: Backlog
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

I continued to work a bit on terrain flattening. Philip started that here: http://git.wildfiregames.com/gitweb/?p=0ad.git;a=log;h=refs/heads/projects/philip/terrain-flattening

I'm taking a different approach though. In Philip's code, everything happened in the Terrain component. While in my code, the terrain component asks the wanted height to the TerrainFlattening? component.

This allows for more diversity, like instead of just flattening it, a building can lower the ground, so f.e. the Roman Army camp could have a trench around it. Or the ground can be risen to allow slope walls f.e.

Imo, this logic shouldn't be in the Terrain component, but in a different one (probably renamed TerrainModifier? instead of TerrainFlattener?). To be extra modifiable, I'd even like the height calculating function to be defined in a JS component. So it can be easily extended.

Do you guys think this patch is going in a good direction?

(Btw, there's one bug (that has always been in this patch), you can't open the Atlas Object pane to add new buildings, so to test it, you need to work on a demo map with some buildings already available)

Attachments (10)

terrainflattening.patch (25.6 KB) - added by sanderd17 5 years ago.
terrainflattening.2.patch (29.2 KB) - added by sanderd17 5 years ago.
terrainflattening.3.patch (18.3 KB) - added by sanderd17 5 years ago.
terrainModifier.diff (37.9 KB) - added by sanderd17 5 years ago.
terrainModifier.2.diff (38.1 KB) - added by sanderd17 4 years ago.
terrainModifier.3.diff (52.9 KB) - added by sanderd17 4 years ago.
terrainModifier.4.diff (45.5 KB) - added by sanderd17 4 years ago.
TerrainFlattening.diff (3.0 KB) - added by wraitii 4 years ago.
TerrainFlattening.2.diff (17.3 KB) - added by wraitii 4 years ago.
TerrainFlattening.3.diff (21.0 KB) - added by wraitii 4 years ago.

Download all attachments as: .zip

Change History (33)

Changed 5 years ago by sanderd17

Attachment: terrainflattening.patch added

comment:1 Changed 5 years ago by sanderd17

Milestone: BacklogAlpha 16

comment:2 Changed 5 years ago by Josh

After quickly scrolling through the patch the only thing that struck me was the (accidental?) deletion of some of the Doxygen commenting in Geometry.h.

Overall I like the direction you're going in (multipurpose component).

comment:3 Changed 5 years ago by sanderd17

Hmmm that comes directly from Philip's branch, I wonder if it was committed accidentaly, or if the docs weren't right (http://git.wildfiregames.com/gitweb/?p=0ad.git;a=commitdiff;h=f3474f143e5706c4f3a5a31c5341d3062a671e37#patch3)

Here's a new patch, with the height calculation code in a JS component. I've also made slope walls (of course it needs a nice model, adding slopes to the current walls is stupid, but you get an idea). The code isn't very fast, but it's only used when you're moving buildings around, so it only is a problem in Atlas. The problem is that for every bit you move in Atlas, the height map has to be recalculated, and to recalculate that, it calls a JS component for every vertex with a building on top.

Changed 5 years ago by sanderd17

Attachment: terrainflattening.2.patch added

Changed 5 years ago by sanderd17

Attachment: terrainflattening.3.patch added

comment:4 Changed 5 years ago by sanderd17

Keywords: patch review added

All real "bugs" should now be solved (segfaults on doing different things with the terrain). Terrain flattening works quite well, and the slope walls need good models, but I think it will also work well.

Changed 5 years ago by sanderd17

Attachment: terrainModifier.diff added

comment:5 Changed 5 years ago by sanderd17

Slope walls work when there's a model at least 8m wide to cover the steep sides (I've tried by thickening the existing celt walls, and apart from not-fitting models, it looks quite good IMO). The only code change still needed is better flattening for the gates, as when one of the wall corners is turned into a gate, the entire corner should be flatter.

The pathfinder is now updated to use the visual terrain map (so the passable tiles change, but are also updated when the building is destroyed). This results in more realistic behaviour (like cavalry not riding up steep slopes). The only issue it still has is that by changing the passability, units may end up on impassable tiles. I think this can be fixed by automatically moving units to the nearest passable tile.

This code (including the pathfinder changes) could also be used to let certain civs build bridges.

comment:6 Changed 4 years ago by sanderd17

Found a new problem with simulation tests in Atlas. It crashed on pressing reset. Now it doesn't crash anymore, but the wrong hightmap is saved (not the original one, but the one modified by objects is saved as original one). Should be fixable though.

Changed 4 years ago by sanderd17

Attachment: terrainModifier.2.diff added

Changed 4 years ago by sanderd17

Attachment: terrainModifier.3.diff added

Changed 4 years ago by sanderd17

Attachment: terrainModifier.4.diff added

comment:7 Changed 4 years ago by sanderd17

Restructured the patch a bit. Now CTerrain keeps the original and the visual heightmap, and can switch between the two. This should make fixing all issues easier, as it can't happen anymore that the visual heightmap is initialized, while the sim heightmap isn't.

There's also a commented out part of code to make the calculation faster (only updating the parts that are possibly modified), but it crashes somewhere for some reason when placing too many entities with a terrainmodifying component.

comment:8 Changed 4 years ago by wraitii

Early patch for my proposal. I use the actual terrain instead of keeping a separate "visual" terrain because I think that's just messy. Algorithm currently flattens the terrain around a placed foundation in-game (not in Atlas, since the logic is called in Commands.js). It's not really bright, it'll flatten a much too big area usually. However you may notice that it doesn't flatten stuffs that are obviously impassable (such as huge drops in terrain elevation…). It's still heavily WIP, since it doesn't really flatten perfectly, and still leaves some areas impassable which I don't really want. It's probably not the best implementation either since I've changed it several times over the course of this morning.

Also needs a smoothing pass imo.

Changed 4 years ago by wraitii

Attachment: TerrainFlattening.diff added

Changed 4 years ago by wraitii

Attachment: TerrainFlattening.2.diff added

comment:9 Changed 4 years ago by Ben Brian

Not working in Atlas is a pretty big drawback of that approach IMO, though in Atlas regardless of implementation, there should be an option to toggle terrain flattening. (I say it's a drawback because we either have to duplicate the flattening algorithm to get the same looks, or use a different approach leading to inconsistent looking maps).

Something else to consider is random maps, it would be nice if they could reuse the same terrain flattening under buildings, which I think is an advantage of the flattening being a property of the structure itself rather than the construction process.

Also keep in mind that we currently don't serialize terrain. So if this component only modifies terrain when an entity is constructed, the change will be lost in saved games, multiplayer rejoining, etc. (they reload the map first of all, then deserialize the components). I think we should serialize terrain if it's going to change, here's another reason to do so: #2147

I have to agree with what you said in IRC though, once the terrain is flattened, it shouldn't "unflatten". It should be undoable for map editing, but it's weird for terrain to magically undeform when a structure is destroyed.

Last edited 4 years ago by Ben Brian (previous) (diff)

comment:10 Changed 4 years ago by wraitii

Basically the fact that it doesn't work in Atlas is because Atlas doesn't call the "flattening" (a simple function of the component), and I've ruled that players could just flatten terrain in Atlas on their own. We could totally have an Atlas function that calls that function, if it exists, for selected entities, we wouldn't even have to copy code or whatever.

For Rms, you're right that this would be nice. However RMs can't access the simulation right now afaik, which makes basically any kind of flattening impossible (even custom is hard to do since you don't know the template data).

One more important shortcoming right now is that it affects terrain under other buildings, which it probably should avoid doing, but that can be fixed.

What I don't like about making it a permanent property is that it sorts of imply that you'll also reflatten whenever terrain changes around, which is sort of weird. And it doesn't allow to "not" flatten, since people in Atlas might not want to flatten.

I hadn't noticed we didn't serialize terrain, but it does seem like something we probably should do if we're going to start being fancy. It's not really hard so I'll add that.

Last edited 4 years ago by wraitii (previous) (diff)

comment:11 in reply to:  10 Changed 4 years ago by Ben Brian

Replying to wraitii:

Basically the fact that it doesn't work in Atlas is because Atlas doesn't call the "flattening" (a simple function of the component), and I've ruled that players could just flatten terrain in Atlas on their own. We could totally have an Atlas function that calls that function, if it exists, for selected entities, we wouldn't even have to copy code or whatever.

True, it could be added to the object placement handler in Atlas, maybe passed as a parameter tied to a setting in the Atlas UI (imagine a checkbox for "flatten terrain under structures"). If it's a direct call to the component, it can indeed happen even within the normally paused Atlas simulation.

For Rms, you're right that this would be nice. However RMs can't access the simulation right now afaik, which makes basically any kind of flattening impossible (even custom is hard to do since you don't know the template data).

It's not impossible, if I understand sanderd17's approach, it would be a property of the entity template, so when the entity came into existence (technically when it's position changed), terrain would be flattened automatically. Regardless of whether it was the map designer in Atlas, a player in-game, or an RMS that placed it. We could also extend the map format slightly to allow disabling the effect on individual structures (in scenario XML it's easy to envision a new tag, and I guess it would similarly be a new property in the RMS entity data).

I'm thinking sanderd17's patch could also be modified to "remember" a structure's influence on the terrain when it's destroyed, to avoid the un-flattening effect. That's probably more about personal preference than a limitation of approach. Though I don't like having a fake heightmap either, it's quite a messy approach for a mostly irrelevant feature (honestly, it is), but it does guarantee pathfinding won't be affected.

comment:12 Changed 4 years ago by wraitii

I meant impossible to do in the RM script itself. If it's a permanent property, then yeah we can definitely flatten once the game loads. It's a little harder to not not flatten terrain for some entities though (as you said, it'd probably require a change in the map format, and a custom value in the component since otherwise that'd be overwritten pretty quickly) in that case, which could be annoying. We could probably hack something together in the RM script or something with my approach but it's not really nice, I have to agree.

The dealbreaker with sanderd's and Philip's patch is the fake heightmap, for me. I really think this will just come back and bite us in the bottom, since it requires any terrain change to affect both, or only one, and it could lead to weird pathfinding… Or you also change the pathfinder and then there's no difference with having only one map.

My approach is to have the automatic flattening be careful enough to not create new impassable tiles, so pathfinding wouldn't be an issue in that aspect.

Changed 4 years ago by wraitii

Attachment: TerrainFlattening.3.diff added

comment:13 Changed 4 years ago by wraitii

Optimized, improved. I think the flattening is now RC. It has a smoothing pass to make it look better. I haven't managed to make it act weirdly, it mostly handles as I'd expect it too. Updates the terrain properly (including water data). I've disabled it for docks and fields.

No serialization of the terrain, no Atlas support, no RS support yet.

comment:14 Changed 4 years ago by leper

Milestone: Alpha 16Alpha 17

comment:15 Changed 4 years ago by Itms

Keywords: review removed

Hi wraitii, I don't know if you have some time to take a look at this again, but I'm making a few code remarks. :)

  • There is some commented out code, probably remaining from the old implementations, that should be removed (in ICmpTerrainModifier.cpp, in CCmpTerrainModifier.cpp line 132)
  • Macros make the code more understandable but rather unclean, especially when undef-ing them. I would go for creating inline helper functions, and it will allow you to put indentation in the code.
  • If you want to exclude docks from flattening, you mustn't forget the Carthaginian uber-dock (the Cothon). Maybe a more generic solution would be to check for the "Dock" category in BuildRestrictions??

Apart from this you'll have to regenerate this patch for the current SVN, so I didn't test the functionality. However, if I'm not mistaken, the only thing necessary would be to move the CCmpTemplateManager code into ps/TemplateLoader.cpp.

I'm moving it out of the review queue, as there will probably be some design discussions about how it should work, before it's committed.

comment:16 Changed 4 years ago by stanislas69

I have a stupid question. Would this patch allow me to have buildings that don't have foundation extending into the ground (e.g the rice field of ROTE) ?

comment:17 Changed 4 years ago by Josh

Yes. However I think you might be able to accomplish that without terrain flattening by using something similar to how we do wheat fields (which is that individual props adjust to fit terrain).

comment:18 Changed 4 years ago by stanislas69

Well problem is water. Water is always flat no matter what angle it has.

comment:19 Changed 4 years ago by Josh

You might be able to make separate terraces and set them as props. Ideally, we would want a result similar to real rice terraces.

comment:20 Changed 4 years ago by stanislas69

problem is water don't blend and terrace are as big as the field in reality...

comment:21 Changed 4 years ago by stanislas69

But this is mostly for another mod I'm making, that I can't share because of copyright, but where all the buildings don't have foundations.

comment:22 Changed 4 years ago by leper

Milestone: Alpha 17Backlog

comment:23 Changed 3 years ago by Philip Taylor

Hmmm that comes directly from Philip's branch, I wonder if it was committed accidentaly, or if the docs weren't right (​http://git.wildfiregames.com/gitweb/?p=0ad.git;a=commitdiff;h=f3474f143e5706c4f3a5a31c5341d3062a671e37#patch3)

I thought the docs were massively over-verbose and repetitive and virtually unreadable, and missed out some information that was actually important, so I rewrote them to be concise.

I think we should serialize terrain if it's going to change, here's another reason to do so: #2147

If it's going to change there's no choice, we have to serialize it. But heightmaps aren't tiny - typical maps are about 320x320 tiles, so ~200KB uncompressed, and compression only seems to reduce it to ~50-100KB. #2147 would require storing all the texture data too, which is some factor larger. So it's a fairly significant amount of data that will have to be saved, transmitted on multiplayer rejoins, etc, and I'd be happier to avoid that cost if possible.

(Looks like saved games are already excessively bloated, but that should be fixed rather than exacerbated. E.g. I think there's ~9MB (uncompressed) entity template data and other entity state serialised by AIManager, none of which is needed at all. And script components are stored very inefficiently (loads of duplication of property names) etc.)

The dealbreaker with sanderd's and Philip's patch is the fake heightmap, for me. I really think this will just come back and bite us in the bottom, since it requires any terrain change to affect both, or only one, and it could lead to weird pathfinding…

I think precisely the opposite :-) . With my patch, there was no effect on pathfinding at all, so no weird gameplay could possibly occur. There would only be slight visual weirdness where a unit might walk along a slope that looks a bit too steep for it, and I think that matters much less than any gameplay weirdness.

If you're changing the terrain that the pathfinder uses, you'll inevitably create cases where passability changes (because of slope or water depth) - units might get stuck, or suddenly find themselves on impassable tiles (which the pathfinder gets very confused by), or a dock that was placed on a shore might find that it's no longer on a shore, or players might exploit the feature to flatten a cliff and reach a previously inaccessible area (particularly important for scripted scenarios where the designer expects the player to stay within a confined area).

You can try to minimise the cases where those passability changes happen, or minimise the effects of those changes, but I believe you can't get rid of them entirely, and they'll come back and bite us.

(FWIW, TerrainFlattening.3.diff applied to current trunk seems to behave quite badly on Death Canyon (2) - if I just build a house on a little bump, it excavates a big hole and sticks the house at the bottom. So whatever the complex algorithm is actually doing to try to avoid changing passability, it doesn't seem to work yet.)

Also, changing the pathfinder terrain is(/will be) quite expensive (lots of state to recompute), so it can't be done frequently - you have to do the whole flattening in one go when the building is first placed. With a visual-only heightmap, it's much cheaper to change it, so it could be flattened gradually and incrementally as the foundation progresses, which may look nicer.

I'm thinking sanderd17's patch could also be modified to "remember" a structure's influence on the terrain when it's destroyed, to avoid the un-flattening effect.

I think my original expectation was that we'd either use the existing 'building rubble' entity or some new invisible entity, which would continue to apply the flattening influence after the building had gone. (Then it might gradually fade away over time.)

Note: See TracTickets for help on using tickets.