Scripting: TileLayerEdit.setTile() is affected by current selection #3482

Agreed. At least, setTile() should return an indication whether it actually changed something or not.

To be a bit more precise, it's not TileLayerEdit.setTile() that only works within the selection, it's TileLayerEdit.apply() that does this, because it internally uses the PaintTileLayer undo command, which in turn uses TilePainter::setCells, which takes into account the selection (as well as map size). It's happening in that helper class, because taking into account the selection and map size is not something I wanted to code into all the various painting tools, but should work the same way for all of them.

I guess the easiest approach here would be to add an option to TilePainter to ignore the selection, and to mirror this option on the PaintTileLayer command and the TileLayerEdit object (similar to the mergeable property, we could have ignoreSelection). Actually providing feedback about whether a change was made would be harder, but if a script really needs to know that it could just check whether the changed region intersects with the selection on its own (a TileLayerEdit.changedRegion property could be added to make this more convenient).

Since TileLayerEdit can only modify a single layer at a time, and tile editing tools can provide a TileMap to show a preview, another common way to trigger changes from scripted tools is through TileMap.merge (though it only works on open assets). This one has a parameter bool canJoin... we could add a bool ignoreSelection there, but I'm not really happy with multiple Boolean parameters, so maybe an overload could be added instead that takes some flags.

That said, a version of setTile() that does automatically respect selections (and layer locks!) could be convenient too, as a separate function that returns true when it set the tile and false when it did not.

The TileMap.merge function does take layer locks into account, but indeed TileLayerEdit.apply currently does not. I guess the latter could just be a no-op when its target layer is locked, unless a new ignoreLock option is set to true.

Or maybe we could combine ignoreSelection and ignoreLock options into a single force option?

I had a brainfart in talking about layer locks - setTile/apply are always for a single specific layer, and the lock status for that layer is easy to check, so I don't think a built-in check for that is required for that. Checking ahead of time and skipping the script's work for that layer would be more performant than checking in apply(), anyway. An option to ignore locks for TileMap.merge would be interesting though! With multiple layers that we're not otherwise accessing individually, checking locks is more annoying.

I feel that for apply(), selections should be ignored by default, and respecting them should be the option that requires changing a parameter. But, I guess if it's always been like this, it might be better to make ignoreSelection opt-in for compatibility just in case anyone was using apply() as it actually behaves rather than as it's described (its docs don't mention respecting selections either).

Since merge() already has a canJoin bool, I think flags would be a good idea since bool order can be hard to remember, and merge(true, false, true) doesn't make it easy to guess which is which. So, perhaps it can be merge(canJoin, selection/lock flags). For apply() however, I think a single ignoreSelection or respectSelection bool parameter would be more convenient, since layer respect is not needed.

Since the problem is with apply() rather than setTile(), any return value would be no more convenient than checking manually, so I'd be fine with no return values. The code to check whether a given layer is locked and whether a given cell is within the selection would probably be simpler than anything checking the return value of apply().