Proposal for "bundler systems" and macros v2

[Trouv]

Motivation

I've been dissatisfied with the LdtkEntity and LdtkIntCell traits/derive macros for a while now. In this issue, I will mostly be focusing on LdtkEntity since it is the more complex of the two, and pretty much all of these changes should be applied to each. I go into detail about some of my complaints in #47. Basically, my complaints are..

• the available macros/attributes are too opinionated about how the users will want to construct their bundles
• the manual implementation of these traits is also very opinionated about what information the user may need access to
  • for simple manual implementations of LdtkEntity, most method parameters go unused and add a lot of boilerplate
  • for more complex manual implementations of these traits, the method parameters may not be enough
• If users want to construct the same type in different ways for multiple registrations, the plugin encourages some bad designs such as..
  • For each registration, a different singleton bundle with a different manual LdtkEntity implementation
  • All-encompassing LdtkEntity implementations that manually check the entity's identifier internally to differentiate
  • Similar designs for the components/nested bundles, but using From<&EntityInstance> instead

I think the final point is partially solved by the relatively-new #[with] attribute macro. This allows you to provide a custom constructor to a field in the bundle, without being bound to a trait. This is perfect for situations where you want to construct a component in two different ways for two different registrations. However, it is limited because your constructor can only accept an &EntityInstance. Even if we gave it more arguments, it would still have a similar issue to the LdtkEntity trait itself - we're still being opinionated about what information the user may need for construction.

The solution to the issue of "opinionated dependencies" is dependency injection. This is what I was trying to express in #47. If we could somehow allow users to provide constructors that are more like bevy systems, where they define in the function arguments what information they need from the world, many of these issues go away. I liked the idea of re-using actual bevy systems, but there were many design questions around this, and I wasn't totally sure that it was possible. For example, I wasn't sure how to best provide metadata pertaining to the entity outside of system parameters (like &EntityInstance or &LayerMetadata).

Proposal Summary

Partially inspired by bevy run conditions, and partially inspired by the more functional design of the #[with] attribute macro (thanks again @marcoseiza), here is my proposal. We should introduce a new concept, let's call it "bundler systems" for now, which are systems that accept either an In<LdtkEntityMetadata<'a>> (a new type), or no input, and return some bundle/component. LdtkEntityMetadata<'a> can contain references to all metadata relevant to an entity from the asset, like &EntityInstance, &LayerMetadata, and even a reserved bevy Entity. In the future, it could also include new things like &LevelMetadata, a map from tileset-uids to previously generated texture atlases (discussed in #87), and a map of entity iids to reserved Entitys (to support #70).

As for macros, the LdtkEntity derive macro wouldn't actually derive a trait, instead it would generate one of these bundler systems for your bundle. Furthermore, all existing attribute macros can be replaced with one: #[ldtk(my_bundler)] where my_bundler is also a bundler system, either custom-defined, provided by the library, or generated via macros. Similar to bevy run conditions, we'll have a common_bundlers module containing bundler systems like sprite_sheet_from_visual or worldly. Of course, defining your own will be as easy as defining a system, and doing so for the outer-most bundle of a registration won't be any different.

User-facing API

Basic cases, without any custom bundlers, will look something like this:

use bevy::prelude::*;
use bevy_ecs_ldtk::prelude::*;

fn main() {
    App::new()
        // These generics are an unfortunate consequence of bundles not being object-safe.
        // Ideally, we'd find a way to get rid of these in the future,
        // or at least find some way to allow users to define them more modularly.
        .add_plugin(LdtkPlugin::<(PlayerBundle, EnemyBundle)>::new())
        .register_ldtk_entity_bundler("PlayerIdentifier", spawn_player)
        .register_ldtk_entity_bundler("EnemyIdentifier", spawn_enemy)
        .run();
}

#[derive(Default, Component)]
struct Player;

#[derive(Bundle, LdtkEntity)]
#[ldtk(spawn_player)]
struct PlayerBundle {
    player: Player,
    #[ldtk(sprite_sheet_from_visual)]
    sprite_sheet_bundle: SpriteSheetBundle,
}

#[derive(Bundle, LdtkEntity)]
#[ldtk(spawn_enemy)]
struct EnemyBundle {
    #[ldtk(sprite_from_visual)]
    sprite_bundle: SpriteBundle,
}

Notice that the registrations accept a function instead of just being generic over the bundle. This allows users to define a separate custom system bundler for the same bundle, and submit a different registration for it.

Speaking of defining custom bundlers, let's see what that would look like. Let's say we want to give both the PlayerBundle and EnemyBundle some new Health component:

#[derive(Component)]
struct Health {
    maximum: f32,
    current: f32,
}

impl Health {
    fn from_maximum(value: f32) -> Health {
        Health {
            maximum: value,
            current: value,
        }
    }
}

Over the course of the game, the player can pick up health upgrades which persist in a resource. When the player spawns, we want their max health to be based on how many upgrades they have. In this case, we could define a bundler for the health component like this:

#[derive(Resource)]
struct PlayerUpgrades {
    health_upgrades: usize,
}

fn player_spawn_health(upgrades: Res<PlayerUpgrades>) -> Health {
    Health::from_maximum(upgrades.health_upgrades as f32 * 50.)
}

As for the enemy, we want to set how much health each enemy has in the level design, so we store it in some field instance "Health". We also want to scale up the enemy's health based on some Difficulty resource. So, we can design a separate bundler for the same component:

#[derive(Resource)]
enum Difficulty {
    Easy,
    Medium,
    Hard,
}

impl Difficulty {
    fn scale(&self) -> f32 {
        match self {
            Easy => 1.,
            Medium => 1.5,
            Hard => 2.,
        }
    }
}

fn enemy_spawn_health<'a>(In(metadata): In<LdtkEntityMetadata<'a>>, difficulty: Res<Difficulty>) -> Health {
    let health = metadata
        .instance
        // This field instance api doesn't exist yet: #175
        .get_float_field("Health")
        .expect("enemy should have a Health field");

    Health::from_maximum(health * difficulty.scale())
}

Finally, we can add the health component to the bundles using these two different bundlers in the ldtk attribute macro:

#[derive(Bundle, LdtkEntity)]
#[ldtk(spawn_player)]
struct PlayerBundle {
    player: Player,
    #[ldtk(sprite_sheet_from_visual)]
    sprite_sheet_bundle: SpriteSheetBundle,
    #[ldtk(player_spawn_health)]
    health: Health,
}

#[derive(Bundle, LdtkEntity)]
#[ldtk(spawn_enemy)]
struct EnemyBundle {
    #[ldtk(sprite_from_visual)]
    sprite_bundle: SpriteBundle,
    #[ldtk(enemy_spawn_health)]
    health: Health,
}

No matching against entity_instance.identifier, no singleton bundles, much simpler macros with a more standard design, and we get dependency injection with access to the entire bevy world.

Implementation

Much of the bundler system infrastructure will be very similar to bevy's conditions. We'll likely have a trait representing bundler systems like..

pub trait LdtkEntityBundler<'a, B>: ReadOnlySystem<In = LdtkEntityMetadata<'a>, Out = B> {}

There will be something similar for bundler systems that don't accept an In<LdtkEntityMetadata<'a>>.

We'll also have a trait like IntoLdtkEntityBundler that can be implemented on all functions with the appropriate parameters and return type. This will all result in us being able to convert such systems into some storable system type like..

type BoxedBundler<B> = Box<dyn for<'a> LdtkEntityBundler<'a, B>>;

Generic output

The main difference between this and conditions is that the output is generic over some bundle B. We want to be able to store all of these bundlers in a resource that we can access during the spawn system. However, storing BoxedBundler<B>s in a collection in some resource will necessarily make the collection/resource itself generic. This means that every bundle would have a different resource storing its registered bundlers. These resources wouldn't be easy to access from the level spawning system. We have to know each bundle type the user is registering at compile time to create such a system.

My first reaction to this would be to have an extra layer of abstraction, where we could wrap the bundler system in another system that returns a Box<dyn Bundle> instead. However, as mentioned in a comment in an earlier example, bundles are not object safe, meaning we can't use them to create trait objects like this.

So, this introduces a controversial implementation detail. The LdtkPlugin simply needs to know on construction what bundle types the user plans to create registrations for, so it needs to be generic over those types. So, we need to change the plugin to LdtkPlugin<B: Bundle = ()>, and use macros to implement Plugin for it on tuples up to a pre-determined length.

However, this isn't all bad. Actually, this has some surprising benefits to it if we implement it correctly. Instead of having the level-spawning system try and use these bundlers, we could have it pipe the necessary metadata into some generic internal system that does..

fn handle_ldtk_entity_registrations_for<B: Bundle>(
    In(metadata_tree): In<LdtkEntityMetadataTree>,
    world: &World,
    mut commands: Commands,
    mut registry: ResMut<LdtkEntityBundlerRegistry<B>>
) -> LdtkEntityMetadataTree {
    // flesh out the entities whose identifiers appear in LdtkEntityBundlerRegistry<B>
}

And then LdtkPlugin<(PlayerBundle, EnemyBundle)> would have some internal schedule handling this piping..

app
    // Other scheduling..
    .add_system(
        process_ldtk_levels
	    .pipe(handle_ldtk_entity_registrations_for::<PlayerBundle>)
	    .pipe(handle_ldtk_entity_registrations_for::<EnemyBundle>)
	    .pipe(consume_and_handle_remaining_ldtk_entities)
	)

Note that each of these registration-handling systems now only concerns itself with one bundle type. This makes it easy for us to use batch-insertion to flesh out those entities, which could increase performance considerably. This will likely make it easier to use batch-insertion within the process_ldtk_levels system itself, since it wouldn't be responsible for any of the complicated entity customization logic anymore.

Also note that process_ldtk_levels itself does not have to be exclusive - just the registration-handling systems. So, if users don't want to participate in this feature, they can simply use the default LdtkPlugin (with B: ()), and the process_ldtk_levels pipe would remain non-exclusive.

Lifetimes

I've been denoting the input type for these bundler systems as generic over a lifetime: LdtkEntityMetadata<'a>. This is because I want to be able to pass much of the metadata associated with an entity in by reference, to avoid too much data cloning. However, I haven't quite figured out whether or not this is possible with bevy systems. Logically it makes sense that it shouldn't break any borrowing rules, but I haven't been able to convince the compiler of this in my testing yet.

At the very least, we'll be able to use smart pointers for this metadata like Rc, so that the borrow checker rules apply at runtime instead of compile time.

Macros

Macro implementation with this design is actually simplified a lot. As I mentioned earlier, now there will be only 1 attribute macro for these bundles: #[ldtk(...)]. We may need a second one for having different input types (In<LdtkEntityMetadata<'a> vs ()), but I'm not sure yet. All logic for the existing attributes can now be moved to public bundler systems, which will make them easier to write, test and maintain.

Drawbacks

Admittedly, some of these aren't really drawbacks in my opinion, and some are just unknowns. The first item is definitely a drawback though.

• LdtkPlugin becomes generic over all bundle types the user plans to register. For bundles the user only plans to create one registration for, they will basically have to type the name of their bundle twice as many times in their app configuration. Worse, this means that the LdtkPlugin type will have to accept all bundles in the same construction, defying modularity of the user's app. I will keep trying to find a way around this one - it's almost a dealbreaker for me on its own, but not quite.
• More verbose macros. Instead of #[sprite_sheet_bundle], we'd have #[ldtk(sprite_sheet_from_visual)]. I don't think verbosity is that big of a problem due to modern code-completion in every editor. However, It's worth mentioning since this plugin is billed as "super-ergonomic".
• Unknown performance implications. I've mentioned in the implementation details that there may actually be some performance gains, mostly due to batch-spawning. However, it might not make up for the exclusivity of the level processing, and the additional memory allocated by piping things around. For this reason, I propose we start by adding benchmarks to the repository, so that we can compare and know for sure.
• Manually nesting bundler systems is unsafe and requires using a &World system param. When we design the macros we can ensure that nesting bundler systems isn't done in an unsafe way, but if users want to call one bundler system from another, this will be possible, but unsafe. I don't think this is so bad, doing this much bundle nesting customization is probably an anti-pattern we would like to discourage anyway.
• Bundler systems are read-only. Again, I don't think this is that much of a drawback. Limiting the user's construction of bundles to not mutate anything outside of the bundle makes sense to me and encourages good patterns (though they still can do some things via Commands). However - some of our previous LdtkEntity functionality did mutate the world, like #[sprite_sheet_bundle] appending the TextureAtlas asset store. For this particular case - we definitely need to implement Texture Atlas caching for sprite sheet bundles #87, but even after doing that we'd have to come up with some new solution for the #[sprite_sheet_bundle(no_grid)] functionality, which allows spawning entities whose visual is a rectangle of tiles.

I hope to get some feedback from the community on this design before going forward with this and coming up with a more detailed milestone/to-do list. I'm especially interested in hearing from @geieredgar since you implemented an alternative to this in #156.

[dmlary]

I would love to see something like this to reduce the complexity & boilerplate as you build out ldtk maps. A couple of notes:

From the examples, and some text, it looks like the #[ldtk()] will dynamically generate the system function if such a function doesn’t already exist. This confused me for a while as I went through the examples looking for the implementation. My understanding for rust is that explicit behavior is preferred, and this one place that holds true. Conditionally dynamic generation of code will lead to someone (me) banging their head against a wall why their changes aren’t taking affect (spoiler, i typo’d the function name, but the code helpfully generates a default that doesn’t do what I want with the misspelling).

I hope this won’t remove the existing From<EntityInstance> code path. Even though this pattern requires a match against entity fields, it’s probably preferred in some cases (example: one function for assigning collider layer mask, vs N). That said, if From<LdtkEntityMetadata> is an option, it looks like the new metadata structure may give easier access to needed data.

For the limitations, yea, the generalization of LdtkPlugin, and really the requirement that all bundles be declared there, does become a significant hurtle. I know ldtk focuses a lot on side-scrolling, but I’m actually using it for a top-down rpg. From my perspective this means I’m going to have significantly more bundles I need to implement for entities. I just ask you significantly increase the number of bundles you imagine when thinking about generalizing LdtkPlugin.

For the generalization, maybe there’s a path to avoid this. You’re calling these spawning functions “systems”; have you looked at how bevy implements IntoSystem, and FunctionSystem? Those are “generic” functions packed into BoxedSystem, and stored into SystemNode. There’s some magic here that you may be able to leverage.

[dmlary]

Oh, and from the discord, here’s a page that simplifies how systems are defined: https://promethia-27.github.io/dependency_injection_like_bevy_from_scratch/chapter1/system.html

[Trouv]

Thanks for the feedback!

I've actually been rethinking this a bit after some discussion on discord. I think I want to try improving the "Blueprint pattern" as much as possible and see if it could be good enough to just replace most of the registration/macros and all the complexity and design headache that comes with them. Plus, it's not like improving that aspect of the plugin will hurt anybody in the mean time.

And by "blueprint pattern" I mean fleshing out entities manually via systems that query for Added<EntityInstance> or some other marker component the user registered. I'm thinking about all the benefits that the macros/registrations provide and how we could add resources or apis that implement similar benefits in that pattern. Will likely make a more detailed post about this later. I want to add many of the features I've thought of regardless so that'll help me see if it's feasible as a replacement somewhere down the line.