Page MenuHomeWildfire Games

Allow to manipulate the heightmap in simulation
Needs ReviewPublic

Authored by smiley on Sep 22 2018, 10:42 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Allow to change terrain height from the simulations. Would allow map authors to be more creative with their maps. Also other interesting things like ditches created on the go from entrenchments or catapult shots.

Known issues now: Dirtying is not done on all necessary vertexes. So the renderer doesn't update. And tests would fail obviously.
Will take care of these ASAP.

A related diff allowing to change textures is in the works. Which would allow for fun things like making dirt roads on trade routes.

Test Plan

Compile and use it.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6422
Build 10638: Vulcan BuildJenkins

Event Timeline

smiley created this revision.Sep 22 2018, 10:42 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Sep 22 2018, 10:42 PM
Vulcan added a subscriber: Vulcan.Sep 22 2018, 10:49 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/736/display/redirect

smiley edited the summary of this revision. (Show Details)Sep 22 2018, 11:35 PM
elexis added a subscriber: elexis.Sep 23 2018, 6:55 PM

This feature combined with the texture changing from the simulation is a very interesting feature.
Use cases will include an earthquake map, erupting volcanoes, cliffs appearing during the game, players building trenches, streets near walls and houses.

As far as I know the data that is used, changed and serialized in the simulation should be located in a simulation component, not graphics/.
So there might be a hopefully not too long rattail of preliminary refactoring needed.
Also what the bot said https://jenkins.wildfiregames.com/blue/organizations/jenkins/differential/detail/differential/736/pipeline
Once textures and terrain are serialized, we can skip the entire rmgen / maploading stage of the loading screen (ticket somewhere).
Should estimate how big that data structure becomes, how it affects the size of savegames/rejoin state sizes (+3MB?), zipped (one can print the length of the zipped state in a Net*cpp file).

source/simulation2/components/CCmpTerrain.cpp
1

8

In D1642#65042, @elexis wrote:

As far as I know the data that is used, changed and serialized in the simulation should be located in a simulation component, not graphics/.

Does hold true for water too. Which is at the moment the only such thing that bridges simulation and graphics AFAIK.

In D1642#65046, @smiley wrote:
In D1642#65042, @elexis wrote:

As far as I know the data that is used, changed and serialized in the simulation should be located in a simulation component, not graphics/.

Does hold true for water too. Which is at the moment the only such thing that bridges simulation and graphics AFAIK.

Am I recalling incorrectly that simulation/ only uses a single number for the water level and only informs graphics/ that the watergrid used for rendering has to be rebuilt?

Other than that, that's part of the mentioned rattail. I'm reminded of the cinema path support where the data had to be moved from graphics/ to simulation/ in order to become serializationsafe, but maybe I'm missing something. CTerrain is used in several places in the simulation, it seems broken already. (So we have the choice between continuing to build upon an incorrect implementations and look the other way seek to rectify or debunk this issue).

Stan added a subscriber: Stan.Sep 24 2018, 2:19 PM

Thanks so much for working on this. I guess you could link this to the "terrain flattening ticket". Could also allow us to finally have a functional murus gallicus.

Stan added inline comments.Sep 24 2018, 2:22 PM
source/graphics/Terrain.cpp
613

We decided to use c++ casts in the convention s so static_cast<>

616

Might want to cast to float for the computation then cast back to u16.

621

Missing spaces between operators.

Thanks @Stan , will fix with the next update.

Am I recalling incorrectly that simulation/ only uses a single number for the water level and only informs graphics/ that the watergrid used for rendering has to be rebuilt?

Thats exactly how it does. Same structure could be used for terrain too. I'm guessing that the heightMap and texture patches could be stored in ccmpterrain and graphics could load it from there.

Stan added a comment.Sep 24 2018, 4:02 PM

Could you try to make foundations level ground ? This would save so much useless underground polygons, might have some positive impact on the game.

In D1642#65064, @Stan wrote:

Could you try to make foundations level ground ? This would save so much useless underground polygons, might have some positive impact on the game.

Then players will build bridges by placing and deleting storehouse foundations?

Stan added a comment.Sep 24 2018, 5:04 PM

Well you can't build storehouses along shores... And you could disable it for docks but... Yeah

IMO, it would look kinda odd when foundations suddenly flattens a square of rolling terrain.
Anyway, its not quite in the scope of this diff. This diff just makes it possible to do this in the engine. People can come up with uses in the future.

smiley added inline comments.Sep 24 2018, 9:54 PM
source/graphics/Terrain.cpp
617

Needs decision regarding this comment.

Foundations

I think if the building restrictions are more strict, so that the terrain is almost flat, then flattening it might or might not prevent such effects like being able to build bridges or impassable terrain.

source/graphics/Terrain.cpp
617

List the advantages and disadvantages, then you might find the answer already.

smiley added inline comments.Sep 25 2018, 1:54 PM
source/graphics/Terrain.cpp
617

pros:

  • Removes a pretty big restriction.
  • Allow better terrain geometry.
  • I dont think its a good idea to limit uses for something which is at the core. changeTileHeight could very well become a JS function.

cons:

  • Makes it a bit more complex.

Well, I guess that decides it then.

FeXoR added a subscriber: FeXoR.EditedSep 25 2018, 2:38 PM

Nice idea, @smiley .
I'd like to inform you that I requested such a feature before and was warned that the engine was not designed with this in mind.

So, please, seriously test this.

Concerning ground below buildings: @Stan: Either we allow players to manipulate the terrain as a game feature (which is both a long shot and outside of the original design concept) or we shouldn't provide any way to the player to change the terrain (impacting passability) at all (basically what @elexis said).

Stan added a comment.Sep 25 2018, 3:04 PM

Well you shouldn't be able

In D1642#65081, @FeXoR wrote:

Concerning ground below buildings: @Stan: Either we allow players to manipulate the terrain as a game feature (which is both a long shot and outside of the original design concept) or we shouldn't provide any way to the player to change the terrain (impacting passability) at all (basically what @elexis said).

Really ? I thought it was part of it since Philip made a Trac ticket for it.
Well it makes sense to use features we implement right ?
What we could do is remember the destruction caused by a building and revert it when that building is destroyed. But smiley is right it's out of the scope

What we could do is remember the destruction caused by a building and revert it when that building is destroyed.

+1 for something like this.

FeXoR added a comment.Sep 25 2018, 3:49 PM
In D1642#65083, @Stan wrote:

Well you shouldn't be able

In D1642#65081, @FeXoR wrote:

Concerning ground below buildings: @Stan: Either we allow players to manipulate the terrain as a game feature (which is both a long shot and outside of the original design concept) or we shouldn't provide any way to the player to change the terrain (impacting passability) at all (basically what @elexis said).

Really ? I thought it was part of it since Philip made a Trac ticket for it.
Well it makes sense to use features we implement right ?
What we could do is remember the destruction caused by a building and revert it when that building is destroyed. But smiley is right it's out of the scope

I thought the flattening Philipü wanted to introduce was visual only so it doesn't change passability. I may be wrong though, didn't check the code.
And in my book it makes much more sense to implement features that we see fit for the game rather than the other way around ;)

smiley planned changes to this revision.Sep 25 2018, 5:56 PM

As per elexis' suggestion, I have decided to refactor some of the terrain system to be more suited to handle this. The current system was put in place without any plans for such levels of simulation interactivity. So this is the better than trying to make it do something it wasnt designed to do.

Stan added a comment.Sep 25 2018, 6:37 PM
<Stan`> Would be nice to be able to set passability with no change to the terrain, for special entities :P
like, bridges set the middle of the bridge passability land
<smiley> i guess needs walkable meshes first.
<smiley> (or make walking indepednt of the ground :p)

Well if you do not keep the passability grid in sync with the visible terrain you can ;) Tweak that a little and you have draw bridges.

Ah, the classic What You See Isn't What You Get. :p

smiley added a comment.EditedSep 28 2018, 6:38 PM
/**
  * Call when the underlying CTerrain has been modified behind our backs.
  * (TODO: eventually we should manage the CTerrain in this class so nobody
  * can modify it behind our backs).
  */

A comment from ICmpTerrain.h, I guess getting rid of this rattail had to be done eventually anyway. Hopefully, can start some further work on this during this weekend.

asterix added a subscriber: asterix.

Thank you smiley for taking your time and working on this. Btw there also seems to be some incomplete support for formats as said here https://trac.wildfiregames.com/ticket/2828 (last two comments from elexis).

elexis edited reviewers, added: Restricted Owners Package; removed: Stan, elexis, FeXoR, nani.Oct 3 2018, 1:32 PM
vladislavbelov added a subscriber: vladislavbelov.EditedOct 28 2018, 9:40 AM

I have to say the current way (1 vertex per call) to modify the terrain is pretty slow, especially if it's called from JS. So I suggest to use a range modifier, or even better - brushes. With brushes you only need to pass brush params and C++ part will complete a request as fast as possible. With brushes you're still able to edit terrain by 1 vertex: just pick the brush radius as 1.

Stan added inline comments.Oct 30 2018, 10:01 AM
source/simulation2/components/CCmpTerrain.cpp
78

Might want to declare that out of the loop, and assign it to something sane like 0. I'm not sure it won't break on some compilers.

119

Missing spaces between operators.
Also static_cast<ssize_t>(expr)

source/simulation2/components/CCmpTerrain.cpp
65

Parentheses around the multiplication is not needed.

74

We have to avoid raw pointers. We can use std::unique_ptr<u16[]> when it'll be able by CppSupport.

78

It can be even shorter:

for (ssize_t i = 0; i < mapSize * mapSize; ++i)
    deserialize.NumberU16_Unbounded("heightval", newHeightMap[i]);
83

Memory leak here, SetHeightMap doesn't get the ownership of newHeightMap, you need to delete it.

Stan added inline comments.Oct 30 2018, 1:54 PM
source/simulation2/components/CCmpTerrain.cpp
65

Multiplication could be moved out to save some cycles. We can't assume it'll be optimised

83

Where should the deletion take place ?

vladislavbelov added inline comments.Oct 30 2018, 2:44 PM
source/simulation2/components/CCmpTerrain.cpp
65

True, we have Loop-invariant code motion, but it's not guaranteed for loop conditions AFAIK.

But we still can make the mapSize to be const.

83

After usage, so after m_Terrain->SetHeightMap(newHeightMap);.

Stan added inline comments.Oct 30 2018, 3:22 PM
source/simulation2/components/CCmpTerrain.cpp
83

Oh yeah I forgot this is synchronous. And that the heighmap is being copied not passed as reference..

Thanks for the additional review. Would resume working on this soon.

smiley updated this revision to Diff 6958.Nov 1 2018, 5:43 PM

Minor cleanup + test fix.

smiley marked 3 inline comments as done.Nov 1 2018, 5:44 PM
Vulcan added a comment.Nov 1 2018, 6:29 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/760/display/redirect