Upgrading public roads will book the land value costs

[Mariculous]

When upgrading a public road, the displayed cost won't include the land value costs, but the actually booked cost does include the land value costs.
This results in newly built roads being nearly as expensive as upgraded PROW roads, which I guess is not the indended behavior.

Expected behavior:
On upgrade of a public road: Do not pay the land value costs
On destruction of a public road: Do not refund land valuecosts
On making a player owned road public: Refund the land value costs to the owner

This behavior reflects a situation where an owner of a public road is not the owner of the property, which seems sensible to me.
Furthermore, this encourages players to upgrade existing roads instead of building new ones without introducing a money generator exploit.

[jamespetts]

This seems like a sensible suggestion. I have now modified the code accordingly.

[Mariculous]

Oh, I just started working on this.
Nevermid, thanks a lot

[Mariculous]

Happy double posting (I guess it's fine as there had been some time in between)

I am further investigating the code around way construction and destruction to solve another bug.
Anyways, there seems to be some duplicated code, so I want to clean this up in turn to improve code maintainability.

I further noticed the following code:

Code Select Expand

if(!upgrading && (obj == NULL || obj->get_owner() != player_builder))
{
single_cost -= welt->get_land_value(gr->get_pos());
}
Which books the land value if the player is NOT the owner of that land, but permitted to build there.
I'd expect the first player (or the public) building on that land to own it. Thus, "This land is for sale" is equivalent to

Code Select Expand

gr->obj_bei(0) == nullptr
If that land is not for sale, its land costs shouldn't be booked.
This is for example relevant when crossing other players (or public) roads. So the check should in my opinion be just

Code Select Expand

if(obj == nullptr)
The upgrading condition is not required anyways as upgrading ==true => obj!=nullptr.
Alternatively tracking the code path: upgrading is only true if there already is a way of the same type at that location, which again can only be true if there is at least one object on that tile, which in turn is true if gr->obj_bei(0) != nullptr

[jamespetts]

Changing the code as you suggest would (if I understand correctly what you intend here) lead to an exploit if the player were to upgrade the way and then demolish it. The player must be taken to buy the land when upgrading the way unless the way is a public right of way, in which case no player (including the public player) when building on the land is taken to be its owner, but only the custodian of a public right of way upon it.

I have incorporated your code clean-up pull request, however: thank you for htat.

[Mariculous]

In fact this behavior already can be exploited, because of a missing check on destruction as reported yesterday.
However, I am not sure how the suggestion (which would also fix the current exploit) could be exploited?

I'll define this more preciesely:
"land is owned by player x" :<=> (gr->obj_bei(0)->get_owner() == x && "that obj is not a PROW road")
"land is for sale" :<=> (gr->obj_bei(0) == nullptr || "that obj is a tree, groundobj or anything moving")
Then we have the following situations:
- Building a way on land for sale will book the land value.
- Removing a way on land that is for sale after that removal will refund the land value

- Builing a way on land not for sale won't book the land value.
- Removing a way on land that is not for sale after that removal has two cases:
- - If the owner of that land remains the same, don't refund the land value (this check is currently missing and causes the bug reported yesterday)
- - If the owner of that land changes from company x to y, book the land value from player y and refund it to player x (keep in mind one of these might not exist if a public way is involved)


I am not sure if I understand your example correctly.
As far as I know, players cannot upgrade other players ways at all. They can only upgrade their own ways or unowned public ways.
Anyways, even if they could I don't see where this could lead to an exploit.

[jamespetts]

Thank you - I have now made a similar change to that requested, but have added extra checks for some marginal cases and to prevent the need for checks elsewhere.