Renaming terms of is_value

[kaputtnik]

Currently we have values and terms for the is_value in world/terrains/init.lua which do not fit with their meaning. F.e.

* "green" which means arable
* "dry" which means walkable

I assume that this terms are used because of historical reasons and had formerly also a meaning to the terrain affinity. But today terrain affinity is completely independent from the is_value. So this terms should be changed to:

* "arable"
* "walkable"

To have straightforward terms we should also rename:

"mountain" -> "mineable"

Same goes for the the internal used terms:

kArable (formerly kGreen)
kWalkable (formerly kDry)
kWater
kDead
kMineable (formerly kMountain)
kImpassable

These changes should not affect loading of older maps, because only the name of a terrain is stored with a map.

This should make the code more readable, because one doesn't have to translate f.e. "dry" into "walkable" in the code.

[GunChleoc (gunchleoc)]

I think renaming TerrainDescription::Type::kDry to TerrainDescription::Type::kWalkable doesn't work, because water also has this property. How about "kPassable"? is = "arable" terrain will then be a bit of an oddity within C++, but it won't bother map editors, because they never see it.

[kaputtnik (franku)]

I've removed kDry type from most terrains, because it makes mostly no sense. kDry (meanis) is now kWalkable and walkable water is senseless (for now...). Also walkable dead terrains are senseless. I removed this property for all types which also have the property "kImpassable", because impassable and walkable is nonsense.

Renaming kWalkable into kPassable makes no difference. But renaming would may be better if the term fits better.

The term "mineable" respetively "kMineable" isn't really good. Maybe we should call it as it was: "mountain"/"kMountain"

"arable" and "mineable" are the only two terrains which are walkable but they didn't get the property "walkable" in src/logic/world/terrain_description.cc . It's some kind of logic because arable terrains has to be passable, otherwise they couldn't be reached. The possibility of walking on arable and mineable terrains is made somewhere in the code, independent from the assignment of the property "walkable".

Please take a careful look at my changes, i didn't understand all of the code and maybe we could consider making it work for future terrains... f.e. making a walkable water terrain (shallow water).

Thanks for looking into it :-)

[GunChleoc (gunchleoc)]

Watch out here - is = "dry" is NOT kDry!

dry, water, dead, mountain and impassable all have the C++ kDry property - see the table in my forum post.

https://wl.widelands.org/forum/topic/1859/?page=1#post-15369

Maybe it would be best to keep the terms in C++ different from the terms in Lua, so they can't be confused with each other?

kGreen implies the properties of kDry I think, so for some reason it isn't needed to add that in the c++ code. I say again, terrain types aren't walkable per se, but the nodes between the terrains are. If a particular node is walkable/navigable etc. depends on the 6 terrains surrounding it, and the code to determine that is quite complex - otherwise the branch for the new water type would long since have been finished ;)

How about the following: You look at the surface behaviour in the game and editor and come up with some good names for the terrain properties. I then do my best to analyze the C++ stuff and try to find better names there too.

[kaputtnik (franku)]

> dry, water, dead, mountain and impassable all have the C++ kDry property - see the table in my forum post

Could you tell me what kDry stand for in C++ code? If you search for this term you will find only one relevant function where it is used: http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/src/logic/map.cc#L1298

It is used to return BaseImmovable::None for a filed IF one (ore more) neighbor is "mountain" OR one (or more) neighbor is "dry"

Note that "mountain" and "dry" are the values from the terrains/init.lua. And that this calculation is made on nodes (where roads could be build), not on fields (where buildings could be build).

As i understand it makes it impossible to build normal buildings on the 6 fields around this node, if one (ore more) of the adjacent terrains is mountain or dry. As a result this is just to have nodes where only roads could be build.

Correct me if i am wrong.

I think we shouldn't have different terms in C++ and the LUA file. But hey, i am a noob, if you think so, you could do so :-)

The corresponding code is very old i believe. It does not reflect the recent changes we had.

What is really complicated are the nodecaps "MOVECAPS_WALK"/"MOVECAPS_SWIM" and how those interact with the terrain properties.

[GunChleoc (gunchleoc)]

Yes, it is all rather complicated. I think we should have the same names where they are exactly the same "e.g. "arable", and different names when they mean different things. kDry is the only C++ property used in multiple terrain types, so I think we should make the other names equal and give this one a special name.

[kaputtnik (franku)]

> kDry is the only C++ property used in multiple terrain types, so I think we should make the other names equal and give this one a special name.

I don't know why this property should have a special meaning other then walkable (or passable).... only because it is used in multiple terrains. kImpassable is also used in multiple terrains. The branch works fine with the changes i've made. I could not see a difference to trunk.

[kaputtnik (franku)]

Just a screenshot of the editor which shows the new icons to display the properties.

[kaputtnik (franku)]

I have merged the linked branch from here and the forested_mountain branch for testing. The resulting terrain menu and the tooltip is fine :-)

[GunChleoc (gunchleoc)]

Screenshot looks good :)

"like*s* trees" though ;)

[kaputtnik (franku)]

:-D thanks

[GunChleoc (gunchleoc)]

I just tested this branch, and the changes messed up road building. I recommend that you keep the programming as it is (e.g. kDry needs to go back where it was) and base the tooltips in the editor / the editor info tool off the is value instead of the terrain type.

[kaputtnik (franku)]

Thanks for testing :-)

Could you please give me an example where road building is messed up?

[kaputtnik (franku)]

Sorry that i am so relentless to this issue. Your a good programmer and i believe what you say, but i couldn't see a difference while playing using my branch or current trunk. Maybe i didn't get the situation where a difference appears... so it would be great if could tell me where you found a difference.

Attaching two screenshots: One played with my branch (is_values_new.png) and one played with current trunk (is_values_trunk.png).

[GunChleoc (gunchleoc)]

I can't reproduce the problem now - so it does seem to work after all. Can you make sure that road building still works where it's supposed to work across all terrain type transitions?

[kaputtnik (franku)]

 > Can you make sure that road building still works where it's supposed to work across all terrain type transitions?

I am convinced about 98%. There is always a bit of uncertainty in such a huge code and if only one person is play testing the changes.

Let me say i love widelands and i wouldn't do anything that breaks the game. I am aware of that this changes affects one of the heart's of widelands. So i also fear that i have overlooked something. On the other side i have played several test games, also tested walking/swimming of animals and all looks fine.

I will try to explain what i have done with swamp terrain, the only one that had need changing (in reality is a bit more complicated, so this is just an overview):
The special things about swamp are:
1. not arable
2. no roads but
  2.1 roads on the edges possible
  2.1 if the space between two flags is only one field, swamp could be crossed with a road

This special things are made by returning type BaseImmovable::NONE. This is in fact the same as "not arable". Not arable implies not walkable, so it is impassable.
swamp had formerly the properties "kImpassable" and "kDry". The decision to let swamp has his special things (like: "no roads, but...") was made in map.cc line 1298. All terrains that have the property "kDry" get caught here and BaseImmovable::NONE is returned.

Now i have changed the swamp terrain to have only the property "kImpassable", so it misses the "kDry" property and couldn't be caught by the code in map.cc line 1298. I solved this by adding the return value BaseImmovable::NONE to all terrains which have the property "kImpassable" in map.cc line 1289-1290. This should be ok, because impassable is equal to BaseImmovable::NONE.

Could you understand this? Fell free to ask if something is unclear. As said its all bit more complicated, because the terrains (the is value) decide the possibility of roads and buildings and the calculation is made depending on the nodes (where roads are), not on the triangles where the terrains is shown.

If you think 98% isn't enough, i will make more tests until i am convinced about 99%. !00% isn't reachable... And in general i think this change is needed and makes the code more understandable.

[GunChleoc (gunchleoc)]

I think 98% is enough. You change removes a bit of complexity from the code, so we should go with it. It is also small, so if we should end up hitting the remaining 2%, it will be easily reverted. I'd say let's go with it.

[kaputtnik (franku)]

Great :-)

Then i would rename "dead" to "unreachable" play test again and propose for merging. The branch of bug 1515006 (forested mountain) should be ready soon, but should be merged after this one. I want to merge forested mountain with trunk before to be sure that there is no conflict regarding the is values.

[GunChleoc (gunchleoc)]

Sounds good :)

[kaputtnik (franku)]

All looks fine :-)

Will we rename "impassable" to "unwalkable"?

[GunChleoc (gunchleoc)]

I am in favor of renaming.

[kaputtnik (franku)]

Done renaming impassable -> unwalkable

There is also the definition of "unreachable" as "unreachable and unwalkable". Looks unlogic but i don't want to touch this until we have an understanding of the logic and influences behind this definition(s).

[GunChleoc (gunchleoc)]

Yes, this is sort of a "duh, of course unreachable will also be unwalkable" moment. I agree that we can leave that one until later.

[GunChleoc (gunchleoc)]

Fixed in build19-rc1.