Exposing more terrain+unit information to WML

[SkeletonCrew]

The problem with this code is that this part "underlying_mvt_terrain(loc_terrain[0]);" gets the movement alias of the terrain. Most of the time it's a single letter but sometimes a multiple characters which can be prefixed by a + or -, which is an invalid terrain. This means you have to parse the string. The real underlying terrain differs per unit. unit->unit_->movement_cost(map.get_terrain(*loc->location_)) does this for you.

Yeah, I would say the line

Code: Select all

loc_terrain_type = get_terrain_info(loc_terrain_mvt_type[0]); 

was powerfully buggy. But as I said it was a hack, more like a kind of proof of concept.

I know it was a proof-of-concept hack, that's why I wanted to point you to a better solution.

The other problem with the direct access to underlying_mvt_terrain() is, when a developer changes the underlying system it will break the WML. With the indirect access the developer will also change unit->unit_->movement_cost() so your WML doesn't need to care about the changes.

My lack of knowledge of fundamentals in terrain handling prevents me from understanding this point. But I happily trust you.

This function returns at the moment strings like "-Sf" but that can change in the future, in fact I'm working on that part of the source, but not sure whether it's going to change.

So where do you think I should go from here ? Use the movement_cost() function obviously.
But if done in [teleport] function I think is still not good. For example, in some campaign, teleport is used to move units on the map for story purpose. For example, in A New Order, in scenario "She-wolf of Haeltin", a 6 MP unit is moved from (10,7) to (8,18 ) with teleport. A check that this movement is possible would break it.
Could we meet such kind of problem if put into [unstore_unit] ?
Or again modifying the [store_locations] tag. This time is becomes more tricky as in the call to this function, we have no garrantee to have a unit as input (after reading the function movement_cost(), I see that without unit,it is impossible to know what terrain an alias terrain is aliassing to as it depend on the type of the unit itself).
Create a new [store_...] function [requiring a unit as input] ?

If I read this correctly I think I haven't explained good enough what movement_cost() does. This function determines the movement cost on a certain unit on a certain terrain type. Note if the unit is slowed the cost is doubled so you should disable the slowed status of the unit (a bit hacky) or write another C++ function. The output of this function is also seen in Wesnoth if you select a unit and move the mouse over certain locations.

Could we move the logic of de-aliassing terrain from unit::movement_cost() to a new unit::underlying_terrain() ?

I would call it movement_cost_internal It recurses via movement_cost() so adding a function which calculates the normal movement is a bit a hassle at the moment. I haven't figured out why it recureses via movement_cost() but I'm tempted to remove it in my expirimental branch and see what breaks.

[viorc]

Could we move the logic of de-aliassing terrain from unit::movement_cost() to a new unit::underlying_terrain() ?

I would call it movement_cost_internal It recurses via movement_cost() so adding a function which calculates the normal movement is a bit a hassle at the moment. I haven't figured out why it recureses via movement_cost() but I'm tempted to remove it in my expirimental branch and see what breaks.

My idea is more a function movement_cost_internal_internal() which's code would be something like (another quick hack, take care):

Code: Select all

gamemap::TERRAIN unit::movement_cost_internal_internal(gamemap::TERRAIN terrain, int recurse_count) const
{
	const int impassable = 10000000;
	gamemap::TERRAIN res = terrain;

	wassert(map_ != NULL);
	//if this is an alias, then select the best of all underlying terrains
	const std::string& underlying = map_->underlying_mvt_terrain(terrain);
	if(underlying.size() != 1 || underlying[0] != terrain) {
		bool revert = (underlying[0] == '-'?true:false);
		if(recurse_count >= 100) {
			return gamemap::VOID_TERRAIN;
		}

		int best_value = revert?0:impassable;
		for(std::string::const_iterator i = underlying.begin(); i != underlying.end(); ++i) {
			if(*i == '+') {
				revert = false;
				continue;
			} else if(*i == '-') {
				revert = true;
				continue;
			}
			const int value = movement_cost_internal_internal(*i,recurse_count+1);
			if(value < best_value && !revert) {
				best_value = value;
				res = *i;
			} else if(value > best_value && revert) {
				best_value = value;
				res = *i;
			}
		}
	}

	return res;
}

returning a basic (underlying) terrain and that could be called from either unit::movement_cost_internal() and gamemap::write_terrain().
With basic terrain I mean one for which a movement cost is known from the unit stats.

[Sapient]

Why not make a new WML tag that takes a unit and an x,y location and sets a movement-cost variable and/or defense variable? That's what you really need, and it would keep the implementation details encapsulated from the WML, and easily accessible by the WML.

[SkeletonCrew]

Why not make a new WML tag that takes a unit and an x,y location and sets a movement-cost variable and/or defense variable? That's what you really need, and it would keep the implementation details encapsulated from the WML, and easily accessible by the WML.

Tha's the idea yes, the question is how to code it
The 'problem' is unit::movement_cost also calculates the poison at the moment, so I would be nice if that could be ignored. The other 'problem' is unit::movement_cost_internal recurses via unit::movement_cost. The solution would be to make unit::movement_cost_internal recurse itself and add and optional parameter to unit::movement_cost to calculate it for the normal case. (Add taking stoned in account would be nice *hint*).

IIRC the recursion change is trivial and shouldn't break anything and the other change is also trivial.

@viorc, I'm not really fond of code duplication if it's not required. In this case I think there's a much better way to achieve the goal, so a tested patch doing that would be nice

[viorc]

@viorc, I'm not really fond of code duplication if it's not required. In this case I think there's a much better way to achieve the goal, so a tested patch doing that would be nice

Probably a misunderstanding as I do not plan any code duplication (as said current movement_cost_internal() would call new movement_cost_internal_internal(), the later replacing thus most of the code in the former).
For the tested patch, I can test it but I want to make sure first it goes in the right direction. I would not like to spend time testing a patch that would be rejected by design

[zookeeper]

Why not make a new WML tag that takes a unit and an x,y location and sets a movement-cost variable and/or defense variable? That's what you really need, and it would keep the implementation details encapsulated from the WML, and easily accessible by the WML.

Yeah, maybe something like that would be good. Something like...this:

Code: Select all

[store_location_details]
    [filter]
        description=Jane
    [/filter]

    x,y=10,12
    variable=foo
[/store_location_details]

...after which variable foo would look like this:

Code: Select all

[foo]
    x=10
    y=12
    id=forest
    name="Forest"
    char=f
    movement_cost=2
    defense=60
[/foo]

[Sapient]

There is no point in combining location details and unit-specific terrain details in the same tag. The two functions should be separate.

[zookeeper]

There is no point in combining location details and unit-specific terrain details in the same tag. The two functions should be separate.

They could be split into two, sure. However, in that case I'd rather move the extra terrain details into [store_locations] (and add a key to it that defines what details are stored, to make WML tasks presumably a bit faster). Or they could even all be bundled into [store_locations]. I like having a smaller number of versatile tags and constructs, instead of a dozen specialized ones.

[SkeletonCrew]

@viorc what I meant is the following patch (untested)

Code: Select all

Index: src/unit.cpp
===================================================================
--- src/unit.cpp        (revision 14028)
+++ src/unit.cpp        (working copy)
@@ -2139,7 +2139,7 @@
                                revert = true;
                                continue;
                        }
-                       const int value = movement_cost(*i,recurse_count+1);
+                       const int value = movement_cost_internal(*i,recurse_count+1);
                        if(value < ret_value && !revert) {
                                ret_value = value;
                        } else if(value > ret_value && revert) {

Then you can change unit::movement_cost() to ignore posion and you have a function which can determine the movement cost for a unit on a certain terrain. I don't have time to test the patch, but I'm quite sure it works

[Sapient]

I assume by poison you mean 'slowed.' Being slowed essentially doubles the movement cost-- it seems like a useful behavior to me, so I don't know why you are trying to work around it.

[SkeletonCrew]

I assume by poison you mean 'slowed.' Being slowed essentially doubles the movement cost-- it seems like a useful behavior to me, so I don't know why you are trying to work around it.

yes I mean slowed

The reason why you want to get around slowed is since viorc wants to know whether a unit can move on a certain terrain. If not a unit shouldn't be knocked on that terrain. If you ignore slowed a slowed unit has some advantages. Also the recursion seems odd to me I'm even considering to change it anyways.

[Sapient]

I assume by poison you mean 'slowed.' Being slowed essentially doubles the movement cost-- it seems like a useful behavior to me, so I don't know why you are trying to work around it.

yes I mean slowed

The reason why you want to get around slowed is since viorc wants to know whether a unit can move on a certain terrain. If not a unit shouldn't be knocked on that terrain. If you ignore slowed a slowed unit has some advantages. Also the recursion seems odd to me I'm even considering to change it anyways.

But what if it's a new unit type that is always 'slowed'? Then it really can't move onto the terrain ... this knockback ability seems like a special case-- if it wants to ignore slowed movement, it should set the unit to not be slowed, then check the function result, IMO.

[viorc]

There is no point in combining location details and unit-specific terrain details in the same tag. The two functions should be separate.

They could be split into two, sure. However, in that case I'd rather move the extra terrain details into [store_locations] (and add a key to it that defines what details are stored, to make WML tasks presumably a bit faster). Or they could even all be bundled into [store_locations]. I like having a smaller number of versatile tags and constructs, instead of a dozen specialized ones.

That is why I would like to have access to the underlying terrain only (through the [store_locations] function) as the movement cost and defense can be retrieved from the [store_unit] tag if we got the value of the basic terrain underlying the location.

[Sapient]

The 'underlying terrain' depends on whether you are interested in DEF or movement, it requires handling special cases (such as 'slowed') and aliases, and it involves assumptions about the unit structure which may change between versions. IMO, this function deserves a C++ implementation.

[SkeletonCrew]

The 'underlying terrain' depends on whether you are interested in DEF or movement, it requires handling special cases (such as 'slowed') and aliases, and it involves assumptions about the unit structure which may change between versions. IMO, this function deserves a C++ implementation.

I said the same before and yes I agree. I don't even know whether WML has the string manipulation functions to extract the proper data. And the internal terrain structure *is* being changed at the moment.