Create or delete banners when tile elements are changed by plugins

[Sadret]

Problem:

The plugin API can change (allmost) any property of tile elements. Usually not much sanity checks are done, which may lead to problems, e.g. with banners. The banner data of tile elements like BannerElement, LargeSceneryElement or WallElement are stored separately from the tile element data; the tile element only stores a reference to the banner data (the id of the banner or bannerIndex). When placing or removing elements via actions, this is taken care of. This is not the case for the situation described above. For example, if an element changes its type, there is no check to see if the original element was a banner (and thus the banner data needs to be deleted) or if the new element is a banner (and thus new banner data needs to be allocated and initialised).
A direct visible result of this problem is that it is impossible to insert a working BannerElement onto a tile.

Proposed Solution:

Whenever a tile element is changed in a way that transforms it from / to being a banner-having element, the banner data is deleted / created. This affects Tile.removeElement, TileElement.type, WallElement.object and LargeSceneryElement.object.
Large scenery requires special care here. It is the only of the affected element types that can span multiple tiles. We need to decide what to do if only some of the parts are changed. You can observe in-game that regarding visuals, the banner rendering only depend on exactly the very first part of the large scenery (sequence == 0). Therefore, we will delete/create banner data if and only if it affects the first part of the large scenery element. I think this is the best solution, since we cannot assume that all parts of the element are present at all times.
One consequence of this is that there will be situations where the plugin programmer needs to manually set the banner index of the non-first large scenery parts. This is unavoidable since, again, we cannot assume that all parts of the element are present at all times. Therefore, if a non-first part is changed, we simply cannot infer the correct banner index in all cases.
(Why is the banner index important for the non-first element parts? Because right-clicking on them will open the banner window associated to the part.)
A different approach is followed by the tile element inspector, which checks the location of all parts and only deletes the banner data if none of the other parts is present. While this made sense in a time where tile element changes were more limited, it certainly does not work anymore considering how many assumptions are invalidated by the power of plugins. For example, tile elements are more freely moved and copied all over the map; it takes too many ressources (iteration over the whole map) to find out if all references to a banner are gone. This is why I decided against this approach.
Edit: I changed the implementation closer to what the tile inspector does. When a large scenery element is changed, it checks for the existence of at least one other element that fits into the sequence. This should cover all cases that can occur using both plugins and the tile inspector, assuming that no plugin writes to bannerIndex, which is deprecated.

Implementation Details:

• Bumped API version.
• New function ScTileElement::RemoveBannerEntry: Deletes the banner data associated to this tile element, unless it is a large scenery element and another element is still using it.
• New function ScTileElement::CreateBannerEntry: Checks if this tile element should have associated banner data and creates it. The banner data is initialised appropriately. If the element is large scenery, it only creates a new banner if it cannot find another element that fits into the sequence.
• ScTileElement::type_set, ScTileElement::sequence_set and ScTileElement::object_set now call the remove function before and the create function after the property change.
• ScTile::removeElement now deletes the banner data associated to the deleted tile element, unless it is a large scenery element and another element is still using it.

Documentation Changes:

• LargeScenery.bannerIndex is back to being writable. The reason was explained above.

[ZehMatt]

Allowing plugins to change the tile element type should have not been possible, this is prone for exactly this kind of bugs, that should be read-only and only game actions should be able to to modify tile properties, the raw access besides reading the data is a terrible idea. We want to move away from storing data in the tile elements directly and do more of it like the way banners work so we would have to add way more code to deal with such situations which I'm against.

[duncanspumpkin]

I would much prefer it if plugins only had a "safe" interface. I.e. multi tile elements can only be modified as a multi tile elements. That would mean removing/adding banners to large scenery affects all of the elements in the multi tile. The same would apply to track and entrances. But I also understand that a lot of the hacks the community love depend on using things unsafe. Can you think of a way to do this safe (i.e. handle multi tile) but also having the ability to opt into an unsafe way through some other method?

[ZehMatt]

I would much prefer it if plugins only had a "safe" interface. I.e. multi tile elements can only be modified as a multi tile elements. That would mean removing/adding banners to large scenery affects all of the elements in the multi tile. The same would apply to track and entrances. But I also understand that a lot of the hacks the community love depend on using things unsafe. Can you think of a way to do this safe (i.e. handle multi tile) but also having the ability to opt into an unsafe way through some other method?

There should be no unsafe way, we can add specific functionality either via game actions or special functions, but as this PR shows that having raw access adds so much boilerplate code which just gets worse the more we separate the data.

[Sadret]

I agree that the unsafe way is not preferable, but there are too many use cases for it that are actively used in the sandbox community (e.g. NE, DK) to straight up discard it now.
Having GameActions for every possible use case would be amazing to have, but also a lot of work.
Additionally, a lot of plugins already use the direct data access, so I fear there is no way around keeping the old API alive.

Anyway, that discussion should be done somewhere else and not in this PR.

[Basssiiie]

I'd like to add that game actions are not a fit-all solution either. Using game actions for modifying the map has a lot more overhead for plugins, and plugins may see their map changes rejected by the game action because of clearance checks, money constraints, or another check. Many place-actions can also remove elements that are already there, which may not be what the plugin would want to do.

I do agree it should be a safe and easy to use API though. Plugin creators should not need to worry about how the internals work. I think this PR does take some steps to support that in the sense of trying to avoid exposing the internal banner objects.

At last I'd also prefer the idea of using a single API to update all elements of a multi-tile element, instead of having to keep in mind the different sequences. But I don't know if the code base currently keeps track of that in any way? Maybe only if they're still in the expected offsets? Maybe that's good enough for keeping them linked?

[Basssiiie]

Does this now also remove and recreate the banner object when you set the type to banner if it's already a banner?

[Sadret]

Yes, and the same goes for all the other affected properties.
Do you think this is a problem? Performance-wise certainly not and it will not happen a lot anyway; only if the plugin dev is careless.

[ZehMatt]

So here is where I stand on this, I think we can probably go with those changes for now but we should deprecate the usage of this API and remove it at some point, this hinders us from separating the data from the tile elements in the future which we have planned for quite some time now and keeping it like this would constantly add more and more checks to ensure associated data is removed.

[Sadret]

Sounds reasonable, but which part of the API exactly are you refering to? Changing TileElement.type? I think the only reason why this has to exist is because Tile.insertElement always insert a surface element.

[ZehMatt]

I'm talking about type, users should replace the element to promote it to another type which leaves only two places to deal with the data.

[Sadret]

I fully agree with that.

[ZehMatt]

Great, I don't expect you to do this in this PR but we should address this as soon as possible and give plugin authors the time to fix their code after deprecating it, perhaps we can provide a utility function for swapping tile elements which underneath will be just delete/insert into the same position on the tile stack.

[Basssiiie]

@Sadret Do you think this is a problem? Performance-wise certainly not and it will not happen a lot anyway; only if the plugin dev is careless.

The only potential (minor) issue I see here is that it may remove the banner text of that banner.

@ZehMatt I'm talking about type, users should replace the element to promote it to another type which leaves only two places to deal with the data.

I agree with this as well. The current way to do it is a bit clumpsy and most (if not all) of the time the setter is only used directly after insertElement(). Setting it at other moments can already produce some undefined/buggy effects. Some better APIs for this down the line would be nice.

[Sadret]

I changed the implementation for large scenery, please see the updated description of this PR at the top of this page.

[Basssiiie]

LGTM :)

[Sadret]

What's the status here? Can I get approval from @duncanspumpkin or @ZehMatt ?

[Sadret]

Could I please get a merge here?

[ZehMatt]

I think its best to give it also a network version bump

[Sadret]

I think its best to give it also a network version bump

Can do, but why? (I mean, in which cases is that necessary?)

[ZehMatt]

I think its best to give it also a network version bump

Can do, but why? (I mean, in which cases is that necessary?)

You have changed quite a bit of logic in these functions, all clients should do the same.

[Sadret]

Freshly rebased. Please merge.

[Sadret]

thank you!