Allow more flexibility with regard to sprite classes when loading Tiled maps

[bunny-therapist]

Enhancement request:

What should be added/changed?

Currently, you can pass custom_class kwarg to arcade.load_tilemap, but there are several limitations:

1. your class must subclass arcade.Sprite - it is not enough to subclass arcade.BaseSprite (even though it probably should be, otherwise the usefulness of arcade.BaseSprite is very limited).
2. if there are animated tiles in a Tiled layer which is loaded with custom_class, then the custom class must subclass arcade.AnimatedTimeBasedSprite, which (more or less) means that all tiles in that layer must be animated. (This is basically a consequence of there only being a single custom class for the entire layer.)

I suggest that instead of passing in a single custom class, it should be possible to pass in some kind of callback function that creates a sprite from a tile. Given enough information about the tile (e.g. if it is animated or not), that function could then decide to return an animated sprite or something else. It should be enough for that function to return arcade.BaseSprite - it should not be necessary to subclass arcade.Sprite.

What would it help with?

• Users can create their own subclasses of arcade.BaseSprite without losing out on using arcade.load_tilemap.
• Users can create different sprites for different tiles in their map without restrictions based on if they are animated or not. For example, one could make a callback create-sprite function which uses the "class" property of tiles in Tiled to instantiate different classes for enemies, items, etc, and still use arcade.AnimatedTimeBasedSprite for some simple animated tiles.

[Cleptomania]

This should be relatively easy to do, and will represent a breaking change, so I want to get this in 3.0.

The plan is to change this all-together to be a callback function which will need to return a class . It is definitely a more advanced way to do things, but I think anyone using this feature is probably sufficiently experienced enough to be able to understand how to use it.

One problem I foresee with making it only expect a BasicSprite is that this is very likely to lead to crashes if you pass only a BasicSprite into the tilemap loader. BasicSprite lacks things like rotation and properties that the TileMap loader will expect to be there. Sure we can sort of implicitly say you need to implement these things on some custom Sprite class.

Converting to a callback function should be easy and doesn't have any real problems as far as I see, but the BasicSprite change needs some more thought. Perhaps a better solution to that might be having a way to disable some of the isinstance checks found throughout Arcade, such that you can make it work with a custom class that you know you have implemented on it what it needs, but don't have a way to make Arcade know that, or maybe the isinstance check should just like print a warning message or something rather than being a hard crash, and the hard crash can just happen when it happens.

[Cleptomania]

I've made a draft PR containing the implementation of the callback function, the gist is this:

• custom_class option is renamed to custom_class_callback
• Accepts a Callable[[pytiled_parser.Tile], Optional[type]] function which means a function like:

  def my_callback(tile: pytiled_parser.Tile) -> Optional[type]:
      if tile.animation:
          return MyCustomAnimatedSpriteClass
      else:
          return MyCustomSpriteClass
  

• Does completely remove the old option of simply providing a custom class. I think this is fine as this is fairly advanced usage anyways and most anyone using this should be able to handle adding a callback function, especially considering the level of flexibility it offers over the previous solution.
• Does not currently change any requirements around BasicSprite or AnimatedTimeBasedSprite

[bunny-therapist]

I did not know that BasicSprite lacked rotation. Given that, I would give up on that and instead I feel tempted to request that BasicSprite is just deprecated completely, or given rotation, since rotation is a quite basic feature and not having it clearly makes it incompatible with e.g. Tiled map loading (maybe you need another Sprite class inbetween, with rotation?). However, I would break out any such concerns from this issue, so for now I think we should just focus on the callback.

1. Why is the return type Optional[type]? It should not be any type, and I don't understand what returning None would do? I would expect it to return type[arcade.Sprite] (if it is supposed to return the type and not the instance, see below).
2. Was there not some option with custom_class like "custom_class_args"? Now what you are just returning the type and not the sprite itself, you lose the ability to pass in args to the sprite init. If the callback returned the instance, the instance could be created with any args, based on the tile. Was there a reason for this design choice?
3. I think the regular, built-in callback that is normally used for the Tile when no custom function is passed in should be exposed as some public definition (maybe it is, I did not check your PR) so that it can be used in custom callbacks e.g. as a fallback mechanism. For example, given your code snippet:

def my_callback(tile: pytiled_parser.Tile) -> Optional[type]:
    if tile.animation:
        return MyCustomAnimatedSpriteClass
    else:
        return MyCustomSpriteClass
    return default_callback(tile)

[bunny-therapist]

Oh, also,since you do not lift the restrictions on animated/non-animated tiles, I assume that then means that your implementation has a hard requirement that the callback must return a subclass of AnimatedTimeBasedSprite if tile.animated? Then then there really should be two callback functions to pass in, one for animated tiles (returning type[AnimatedTimeBasedSprite]) and one for non-animated tiles (returning type[Sprite]). Otherwise you are just creating a "trap" which cannot be documented using type hints but must be documented using comments.

[Cleptomania]

Yeah BasicSprite doesn’t support rotation because rotation is incredibly expensive in collision/hitbox operations, so we wanted to provide an option that doesn’t use it for users who don’t need to account for that in their hitboxes. A middle class between BasicSprite and Sprite that supports rotation might be a good solution.

1. Returning None triggers the default logic(using Sprite or AnimatedTimeBasedSprite), that’s why it has the Optional wrapper. We can add the subscript for type[Sprite] I just didn’t think about it
2. There is still custom_class_args available in this and it works exactly the same as it did. However it does that mean that any custom args would have to work with every potential class you would return from the callback. The problem is that if the callback is responsible for returning the Sprite instance, that moves a ton of the responsibilities of the built in TileMap class onto the user.
3. See point 1. I’m open to changing how this works, this was just what I went with initially. As of now there is no default callback, if the callback is None then the default logic is just inside the functions directly, that can be changed.

Yeah it does issubclass checks on the returned types to ensure they are subclassing Sprite, and if the tile is animated, AnimatedTimeBasedSprite. I’d have to split up a lot of logic inside the Sprite creation to have separate callbacks for it

[cspotcode]

Hitbox adjustment used to entirely skip rotation adjustments when rotation was zero. Did you benchmark to check how fast it was to have a sprite that supported rotation but never modified the angle, thus never did any of those computations?

Using multiple (sub)classes can prevent python 3.11's new specializations, since the type of sprite passing through a codepath will change back and forth.

[Cleptomania]

I don't think this is likely to be included functionality in Arcade unless someone brings forth a suitable PR. Anyone who needs this flexibility in the TileMap loader, I would encourage to copy the module code into their own project, and make any changes they need for their project to it.

Making the TileMap API flexible enough to suit every use case is challenging to say the least, at least with still maintaining a reasonable level of complexity/usability, so I would rather encourage it as a starting point and for more advanced needs to copy the module and build on it for any projects specific needs.