Exposing more terrain+unit information to WML

[viorc]

In a discussion about the WML for knockback ability (pushing defender unit by one tile on successful hit),

The ideal way of course would be to also prevent knocking units to any terrain they are normally unable to enter, but that unfortunately cannot really be done.

Well I figure out that we just need to get the value of the terrain ID of the target tile.
With it, after storing the unit, we can access to $stored_unit.movement_costs.$terrain_id. Comparing it to the $stored_unit.movement, we can know if the unit is able to enter the tile.
I have hacked in the C++ function called by the [store_locations] WML function gamemap::write_terrain (in src/map.cpp source file)(**)

Code: Select all

void gamemap::write_terrain(const gamemap::location &loc, config& cfg) const
{
        std::string loc_terrain;
        std::string loc_terrain_mvt_type;
        std::string loc_terrain_type_id;
        terrain_type loc_terrain_type;
        loc_terrain += get_terrain(loc);
        loc_terrain_mvt_type = underlying_mvt_terrain(loc_terrain[0]);
        loc_terrain_type = get_terrain_info(loc_terrain_mvt_type[0]);
        cfg["terrain"] = loc_terrain;
        loc_terrain_type_id = loc_terrain_type.id();
        cfg["terrain_type"] = loc_terrain_type_id;
}

It seems to be working fine. It basically adds a member 'terrain_type' to the array outputed by the WML function [store_locations]. This new memeber could be stored in the variable terrain_id in the aforemetionned code $stored_unit.movement_costs.$terrain_id..

Now I am pondering, is the addition of a variable concerning the terrain ID in the WML function [store_locations] a good thing in term of design or should I rather introduce a new WML function [store_terrain] ? This could be necessary for any WML code that want to access to the movement type/defense/resistance of a unit (mainly ability I guess)
If anyone could give an answer, I could write a formal patch and submit it to make that logic possible to code.

**: a little tricky as many terrains (given by a letter as returned by the function get_terrain()) are in fact just aliases for which no movement_cost is defined. Hence, one needs to find out what is the underlying terrain (the one aliased to)

[zookeeper]

**: a little tricky as many terrains (given by a letter as returned by the function get_terrain()) are in fact just aliases for which no movement_cost is defined. Hence, one needs to find out what is the underlying terrain (the one aliased to)

This was the essentially the problem that I couldn't solve, and which made me mark the idea as undoable.

I think we would need some new functionality (like a new tag), with which one could assess whether a location can be moved to (or something similar), but IMHO this would be in the realm of bloating WML for purposes it was never meant to be used for...

Anyway, just now I got a funky idea of how the goto_x,goto_y unit variables could be used to go around this problem. It might require a slight code change that shouldn't break anything however. I'll report back in a while.

[SkeletonCrew]

I haven't looked close in it, will do that later. But I think this is the wrong approach
Just to convince myself I looked at the Python API in ai_python.cpp at this function PyObject* python_ai::wrapper_unit_movement_cost( wesnoth_unit* unit, PyObject* args )

It uses unit->unit_->movement_cost(map.get_terrain(*loc->location_)) to determine the movement of the terrain. This function should take care of the aliasing for you, also the underlying terrain can differ per unit.

If you post a patch at patches.wesnoth.org ask boucman to assign it to Mordante since I'm working on the terrain system and I might need to tweak the patch before merging into my branch.

[viorc]

I think we would need some new functionality (like a new tag), with which one could assess whether a location can be moved to (or something similar), but IMHO this would be in the realm of bloating WML for purposes it was never meant to be used for...

I haven't looked close in it, will do that later. But I think this is the wrong approach
Just to convince myself I looked at the Python API in ai_python.cpp at this function PyObject* python_ai::wrapper_unit_movement_cost( wesnoth_unit* unit, PyObject* args )
It uses unit->unit_->movement_cost(map.get_terrain(*loc->location_)) to determine the movement of the terrain. This function should take care of the aliasing for you, also the underlying terrain can differ per unit.

So if I summarize, one thinks it should be a new tag giving the status of the accessibility of a tile to a unit depending of its movement type and the other thinks this could be more properly done by using the C++ function unit::movement_cost(). Is it correct ?
I would have thought that having the underlying terrain available in WML to get all the related data for any unit would have been quite handy, but you guys know better.

Anyway, just now I got a funky idea of how the goto_x,goto_y unit variables could be used to go around this problem. It might require a slight code change that shouldn't break anything however. I'll report back in a while.

Waiting to see the wizard in action I will just keep my patch for the very unlikely case you are not getting to anything.

If you post a patch at patches.wesnoth.org ask boucman to assign it to Mordante since I'm working on the terrain system and I might need to tweak the patch before merging into my branch.

No patch from me then

[Rhuvaen]

I would have thought that having the underlying terrain available in WML to get all the related data for any unit would have been quite handy, but you guys know better.

I guess that would be the in-depth solution.

But wouldn't it solve the problem if unstore_unit and teleport just checked that? In the same way unstore_unit finds vacant hexes if the target hex is occupied? (okay, I can see units being knocked through a wall, then )

Anyway, I'd like some check of accessibility as proposed here being applied to unstore_unit, at least.

[viorc]

I would have thought that having the underlying terrain available in WML to get all the related data for any unit would have been quite handy, but you guys know better.

I guess that would be the in-depth solution.

Do you mean coded at low level (compared to a high-level function) ?

[zookeeper]

Anyway, just now I got a funky idea of how the goto_x,goto_y unit variables could be used to go around this problem. It might require a slight code change that shouldn't break anything however. I'll report back in a while.

Waiting to see the wizard in action I will just keep my patch for the very unlikely case you are not getting to anything.

Ok, my idea was simply this: when assigning the goto coordinates, the engine would trim inaccessible locations from the path. So, we could just check whether the HI can move one hex north by modifying it's goto_x,goto_y and then checking those values again to see if they still match the north hex. However, this doesn't (as I expected) work like this now, and on second thought I'm not sure if it would be worth the trouble. Anyway, that would be the slight code change I referred to, and would be one way of solving the issue (it would be easy to macroify the check to a simple {IF_ACCESSIBLE UNIT X Y} macro, too).

But wouldn't it solve the problem if unstore_unit and teleport just checked that? In the same way unstore_unit finds vacant hexes if the target hex is occupied? (okay, I can see units being knocked through a wall, then )

Anyway, I'd like some check of accessibility as proposed here being applied to unstore_unit, at least.

Hey, that's a pretty decent idea. Something like ignore_inaccessible=yes|no (default yes) being allowed in both [teleport] and [unstore_unit], that would check whether the unit could normally move there and only allow the unstore/teleport if it could. I'd vote for this solution. The only problem would be some graphics glitches (the unit blinking on and off) when you'd just test for accessibility by unstoring/teleporting a unit to a hex.

[viorc]

But wouldn't it solve the problem if unstore_unit and teleport just checked that? In the same way unstore_unit finds vacant hexes if the target hex is occupied? (okay, I can see units being knocked through a wall, then )

Anyway, I'd like some check of accessibility as proposed here being applied to unstore_unit, at least.

Hey, that's a pretty decent idea. Something like ignore_inaccessible=yes|no (default yes) being allowed in both [teleport] and [unstore_unit], that would check whether the unit could normally move there and only allow the unstore/teleport if it could. I'd vote for this solution. The only problem would be some graphics glitches (the unit blinking on and off) when you'd just test for accessibility by unstoring/teleporting a unit to a hex.

Does it mean that is would be checked based on that flag on the C++ level ?
If so, this idea is not so suitable for my problem, as I think that if the unit is not allowed to be pushed back from one square, the combat should be continued until the end (using remaining attacks).
But generally speaking it is still a good idea.

[Rhuvaen]

Something like ignore_inaccessible=yes|no (default yes) being allowed in both [teleport] and [unstore_unit], that would check whether the unit could normally move there and only allow the unstore/teleport if it could.

Does it mean that is would be checked based on that flag on the C++ level ?

That is what both zookeeper and I seem to be suggesting.

If so, this idea is not so suitable for my problem, as I think that if the unit is not allowed to be pushed back from one square, the combat should be continued until the end (using remaining attacks).

Why? You would simply - during the attack - store the unit, unstore a copy to x2,y2, check if the unit at x2,y2 is identical to the stored unit -> if yes, kill the original unit, if not, don't. Combat should continue as long as the unit remains on the original location.

[SkeletonCrew]

If so, this idea is not so suitable for my problem, as I think that if the unit is not allowed to be pushed back from one square, the combat should be continued until the end (using remaining attacks).

Why? You would simply - during the attack - store the unit, unstore a copy to x2,y2, check if the unit at x2,y2 is identical to the stored unit -> if yes, kill the original unit, if not, don't. Combat should continue as long as the unit remains on the original location.

I think this idea would be a kludge for the orginal problem. I think it's not unreasonable for WML to know whether or not a unit can move over a certain terrain. The only problem I have with the patch is that I think it's looking at an internal which shouldn't be exposed. I'll look at the source in a couple of hours and post back after that. After I looked at it I'll post a more verbose explanation about why I think these things, that is if I remember the code correctly

[sparr]

I think it would be nice to have access to that information in WML. On my tides maps, where water moves around and changes depth, it might be an interesting idea to make units that arent allowed to enter deep water drown (take damage, or die, or suffer some larger penalty) if they find themselves in deep water.

[SkeletonCrew]

Code: Select all

void gamemap::write_terrain(const gamemap::location &loc, config& cfg) const
{
        std::string loc_terrain;
        std::string loc_terrain_mvt_type;
        std::string loc_terrain_type_id;
        terrain_type loc_terrain_type;
        loc_terrain += get_terrain(loc);
        loc_terrain_mvt_type = underlying_mvt_terrain(loc_terrain[0]);
        loc_terrain_type = get_terrain_info(loc_terrain_mvt_type[0]);
        cfg["terrain"] = loc_terrain;
        loc_terrain_type_id = loc_terrain_type.id();
        cfg["terrain_type"] = loc_terrain_type_id;
}

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.
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.

[viorc]

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.

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.

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] ?


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

[zookeeper]

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] ?

How at least I see the checks in [unstore_unit] and [teleport] should work would be that they only check whether the terrain is inaccessible to the unit or not, not whether the unit would currently have enough MP's and such to actually get there via normal means.

[viorc]

How at least I see the checks in [unstore_unit] and [teleport] should work would be that they only check whether the terrain is inaccessible to the unit or not, not whether the unit would currently have enough MP's and such to actually get there via normal means.

Fair enough. I will check how to do. Thank for your guidance.