Scripting: Set fileName when using TilesetFormat.read() or MapFormat.read()

[eishiya]

A common task in scripted MapFormats is to read an external tileset from file and assign it to the newly created TileMap. There are two ways to open a Tileset from a file:

• tiled.open(), which opens the Tileset as a document in Tiled. This is not usually desirable, and takes extra code to close the tab afterwards, and extra work to only close the document if the user didn't already have it open.
• TilesetFormat.read(), which doesn't open a tab... but also doesn't set the Tileset's fileName, so when that Tileset is added to a map, it becomes embedded.

It's easy to miss the unintentional proliferation of tileset clones this causes. Here's an example from the forum of this unintentional embedding. Unless there's some specific reason not to, I think TilesetFormat.read() and TilemapFormat.read() should set the Asset's fileName, which should hopefully fix this embedding issue. Why is this not set in the first place?

Of course, sometimes it's desirable to create an embedded tileset on purpose. For this and other purposes, I think Tileset.clone() and TileMap.clone() would be good additions - the clones should have an empty fileName.

If there is a reason that TilesetFormat/TileMapFormat.read don't set the fileName, then the API should provide methods to open files, maintain their link to their file, but without opening a document tab for them, e.g. tiled.openSilently(), or tiled.open(path, silent?).

[bjorn]

I think TilesetFormat.read() and TilemapFormat.read() should set the Asset's fileName, which should hopefully fix this embedding issue. Why is this not set in the first place?

Backstory

None of the formats set the file name of the type they return, which traditionally did not even have a file name field. Like an image also does not store the file name from which it was loaded. The file name only comes into play for assets open in the editor, since they have a Document instance associated with them, which used to be where the file name was stored.

Eventually, it became a little more complicated, and there was reason to move the file name (and format) into the Map and Tileset classes. I did this reluctantly, but there were a number of places in the code where I had access to the Map instance but not its respective MapDocument instance. Actually looking at the code, this appears to only be due to the need to store a file name for navigating to an error/warning when it is double-clicked. Many such errors are reported from plugins, which don't have access to the MapDocument instance.

Another thing that made things complicated is the introduction of the EditableMap, which implements the scripting API for maps. It derives from EditableAsset, which provides the fileName property. However, this property only has a value for any asset which has a Document instance associated with it, which assets loaded directly through the format classes like TilesetFormat.read() don't have (and hence they also have no undo history).

Way forward

If there is a reason that TilesetFormat/TileMapFormat.read don't set the fileName, then the API should provide methods to open files, maintain their link to their file, but without opening a document tab for them, e.g. tiled.openSilently(), or tiled.open(path, silent?).

I think that's the best solution, yes. Naming the function is difficult, but I am considering tiled.load. This function would behave exactly like tiled.open, except for opening an editor tab. This means that apart from the returned asset having a file name, it will also have undo enabled and in case the asset was already open or was loaded before, you would get the exact same instance returned.

The tricky part may be, to unload this asset when the last EditableAsset instance is cleaned up by the JS garbage collector. Currently, EditableAsset instances for open files are owned by the respective Document and are deleted along with it (regardless of whether the script still holds a reference: such values will get invalidated). The EditableAsset may need to keep its Document alive instead.

Of course, sometimes it's desirable to create an embedded tileset on purpose. For this and other purposes, I think Tileset.clone() and TileMap.clone() would be good additions - the clones should have an empty fileName.

Yes, I think the addition of a clone method should go very well along with the addition of tiled.load. The clone would not get a Document instance associated with it, which means it has no file name and no undo history.

[eishiya]

Thanks for the info! I thought it might be something complex like that, but it's cool to have the details.
tiled.load sounds like a good name to me.

[eishiya]

Currently, if you tiled.open() a file that's already open, you don't necessarily know whether you should close() it - you don't want to close the file if it was opened by the user before. Would tiled.load() help with this? If MyFile was already open by the user in the editor, and I load() it, do some work, and then close() that handle, I don't want that to affect the MyFile tab the user had open.

[bjorn]

@eishiya I think you'd just never close an asset loaded with tiled.load(). Ideally the garbage collector would be able to take care of the asset eventually when there is no reference to it anymore, though it's tricky to deal with ownership when the same thing might be owned by JS and/or by the C++ side.

[eishiya]

This tiled.load() should ideally also work via CLI. As it is, there seems to be no way to get a Tileset/TileMap that has its fileName set in CLI scripts (except, of course, by opening them as parameters to tiled), since tiled.open() doesn't work via CLI. This means, for example, it's not to automate replacing embedded tilesets with external ones via CLI unless the user passes the external tilesets as parameters.