Fish and mountain ressources cannot be removed when they are on grass

+1 for removing incompatible resources when changing terrain type

+1 the same as the above

Setting to incomplete for bug sweeping.

Still annoys me in r6970 when editing a map where the resources are already set.
I also vote for removing the resources when they cannot be placed on the new terrain.

Setting to incomplete for bug sweeping.

looks like one line fix. I had problems to decrease resources in current trunk, but regardless of it I will prepare a branch with a fix

The fix in #7 is faulty, it only addresses the problem in the editor. #2 and #3 are right about the fix.

In the game, we change terrains too (for example in the altantean scenario 1). We should fix this problem there: whenever a terrain changes and the field contains resources that are no longer allowed for the new terrain, remove the resources. If they are still valid, leave them. This will fix the editor (though the overlays might need to be recalculated) and logic problems in the game. The resource validation should also be used when a map is loaded, so that also broken maps are fixed.

re: "whenever a terrain changes and the field contains resources that are no longer allowed for the new terrain, remove the resources." - not against it, but what is the difference between improper resources that cannot by mined and no resources at all - very minimal. The map creator will have to re-assign correct resources in both situations.

#7 is here and 'one click away', alternative solutions - who knows when they will be programmed. :)

That is not quite true: mines do mine from a certain radius, I think fishers do too. So the resources are still available ingame.

A quick fix usually comes back to byte us in the end. #7 requires fixing this issue in all tools in the editor that deal with resources or terrains.

I agree with #10 - having resources lying around where they should not can cause all kind of problems.

See also the description: The resources stay there when one starts a game. Miners and fishers can get them. A geologist finds them (where fish is, he finds nothing because there is nothing). A well runs only at 65%.

I fixed this bug in lp:~janosch-peters/widelands/editor-remove-invalid-resources. I pushed several revisions. To see the complete diff got to http://bazaar.launchpad.net/~janosch-peters/widelands/editor-remove-invalid-resources/revision/7663?remember=7657&compare_revid=7657

Basically I did two things:

1. Map::change_terrain verifies now if the new terrain matches the current resource. If it doesen't, the resource is deleted. This fixes the bug in the editor.

2. A new method Map::check_res_consistency is called when a map is loaded. The method looks for invalid resources on every node and if present, deletes them. This "fixes" broken maps on load and affects both the editor and the game.

The second one turned out to be more work than I thought. I discovered that in some places the resource of a field was set to "no resource" by doing this: "field.set_resources(0, 0)". But this actually means (type:coal, amount:0), because the DescriptionIndex of resources starts at 0. It appears to me that (type:coal, amount:0) is currenty the semantic for "No Resource" in Widelands. This is a cumberson convention in my eyes and it also has negative side-effects. For instance, you cannot use TerrainDescription::is_resource_valid like this:

world.terrains().get(field.get_terrains().d).is_resource_valid(m_fields.get_resources())

because the convention isn't recognized in this method.

To have a clear distinction between "coal" and no resource, I changed all occurences of set_resources(0, 0) to set_resource(Widelands::kNoResource, 0). This also fixes TerrainDescription::is_resource_valid. I think this makes the code more readable and maintainable. However, I had to adjust the code in several places and also change test cases to make this work.

Testing:
1. Start the editor. Add "water" resource on a grass terrain. Then, change the terrain to mountain. The water resource should vanish.

2. I attached the map "invalid_resources.wmf". If you load the map the invalid resource "water" on a mountain terrain node (7,5) are removed. Likewise, the invalid resource "gold" is removed from (2,3), a water node.

Codestyle check and regression test suite passed successfuly.

I briefly looked at your diff, it looks good to me. Feel free to propose merging, when you think it is ready. (Yes, it looks ready as by now)

Good work

Very nice someone is working on this :-) I will test and respond to the merge proposal.

I attached a map where shift + resource is not able to remove resources. In fact I think they should be removed automatically after change of terrain. Am I correct?

+1 for removing it automatically once it is no longer legal.

@TiborB: I can confirm this. I also found some other glitches, I need to do some more work on this bugfix.

BTW, my intention is to allow resources on a vertex if at least one adjacent triangle has a matching terrain type. Is this how you expect it to be? Another possible definition would be to allow resources on a vertex only if ALL adjacent triangles have matching terrain types. Of course you can also think of definitions inbetween.

Is my laissez-faire definition OK with you?

Well, one possibility is to split resources into two groups:

1) fish - 1 of 6 is enough
2) underground ware materials - all 6 must match

Water is questionable, but could go to group 2, no big deal.

The problem with fish is that fisher can catch fishes only small distance from coast. So if we prohibit fishes on coastline, the amount of reachable fishes will drop down significantly (to half I think).

fix: ware => raw

And it must be "consistent" - if resource is preserved after terrain change, it must be manually removable (without undoing terrain change).

I understand this is more then pure fix of current status..... But is would be very nice to have it working properly...

I think we should aim for some behavior that is similar to the current one. I think it is:
- fish: one triangle is enough (reason in #19)
- mountain resources: four matching triangles
- water: no idea

I also don't know if dead terrain (lava) has/should have an influence on this.

Resources that do not match the new terrain combination should be removed automatically, so there is acutally never a need to remove them manually.

In an ideal Widelands world, resources would live on triangles. That would make it much easier to understand where they can/cannot be. Also for the fisher, the triangles are always reachable. This is a bigger change though and would impact more in the engine - I do not think that it is infeasible to do it though.

Given that we stick with the current solution of resources on a node, I consider it suboptimal to distinguish individual resources like water/fish/mountain resources inside the engine - it meddles engine and data.

I also do not understand the reason and would like to hear some arguments for that. Does it really impact the game so much if fish and mountain need a different number of adjacent triangles?

I think #19 brings a very good argument why 1 triangle must be used for fish. I suggest using 1 triangle for everything (like Janosch did it already) and see if that 'feels weird' and only then make the rules more complex.

Or, you know, we could move the resources onto the triangles.... that be cool :)

>I think #19 brings a very good argument why 1 triangle must be used for fish. I suggest using 1 triangle for everything (like Janosch did it already) and see if that 'feels weird' and only then make the rules more complex.

OK, one triangle should be enough - after some thinking I agree.... But in Janosch's branch is not made that way AFAIK, it check trianges down and right

Well to avoid misunderstanding - 'one triangle' means that all 6 has to be tested and if at least one matches the resource...

Yes, #25 is also what I had in mind. Janosch, can you change the code to that?

I discovered that there is already a method used by the editor to check if a resource is allowed at a given point. As I only want to fix the bug and not change functionality, I re-used this method to do the resource verification on-load.

The old and new rule in Widelands is (see Map::is_resource_valid):

1. If the resource belongs to an impassable terrain (fish), there have to be at least two matching terrains adjacent.
2. For all other resources, there have to be 5 matching terrains adjacent.

If we simplify the rule to rule 1. for all resources, it will be possible to place minig resources on the border between mountain and grass. However, in those location it is typically not possible to build mines. So the player sees more resources but cannot use them. I think the best long-term solution would be to add an attribute in the resource configuration, which defines how many adjacent matching terrains are needed. This would lead to compact code and easy gameplay fine-tuning by just changing the configuration. But this should be done with a seperate bug report. Im eager to get the current fix into trunk.

TiborB's problematic map works now with the latest revision: The resources are removed because they have too few matching terrains nearby.

One problem remains: If changing a terrain leads to removed resources, they are not recreated when you undo the terrain change. I looked briefly into it, but it involves a lot of work and Im not sure if it is worth the effort.

I merged the trunk into the branch, hopefully the CI build works now. Please review the code again.

@SirVer: I didnt see your post when I did my last post.

The latest implementation consideres all six triangles around a resource vertex, but applies the legacy recource-terrain matching rule. I can simplify the rule as you wish. It will make the code simpler but has a slight negativ impact on gameplay IMHO (potentially a lot of resources which cannot be mined).

> to place minig resources on the border between mountain and grass. However, in those location it is typically not possible to build mines. So the player sees more resources but cannot use them.

I think this is not correct. A mine has an action radius so such resource would be within reach of a mine and thus mineable.

> If changing a terrain leads to removed resources, they are not recreated when you undo the terrain change.

Interesting side effect .... but I personally am OK with this, I dont want to ask you for additional work....

I am also satisfied with mentioned two rules. As long as it is consistent.....

@TiborB: I did not know about action radius of mines.

I am going to simplify the rule as follows: Resources need to have at least two matching terrains adjacent.

This is basically the "fish-rule" applied to all other resources. It gives map authors more liberty in the distibution of resources and makes the code simpler. And we dont mix up engine and data stuff.

Lol, so we already had rules in place. funny that I did not remember :).

I do not care about the undo thingy - so what. The rest of the change lgtm some nits not withstanding. Once you are happy feel free to merge yourself Janosch.

Thanks for your contribution. Sorry for taking so long with this but as you noticed but there was quite a bit of need for discussion and bunnybot being created let to some uncertainty about the proper merge etiquette.

sorry, didn't see #30. Simplyfing the rule sgtm too - I doubt it will be noticable in daily play anways.

mine working radius is a thing, tibor is right. It is the first parameter to the mine command (e.g. [1]). It means that resources in this area are considered. For most mines the radius is 2, I think it is bigger for the atlanteans.

http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/tribes/buildings/productionsites/barbarians/coalmine/init.lua#L67

I changed to confirmed because it happen again. In r 7000 all is fine, in r7002 fish and ore resources aren't automatically removed when the terrain changes. Maybe a side effect of "Split overlay manager"? https://bugs.launchpad.net/widelands/+bug/536606

Very likely I messed that up. I'll look into it.

Fixed in build19-rc1.