Add a new global method: Crafty.system()

[starwed]

It's very natural to want a non-entity object that can participate in the event system. Currently when objects like the viewport or the graphical layers want to listen to an event, they have to refer to themselves via hardcoded references, since those event handlers get run with Crafty as the context.

This introduces the idea of systems which have a set of methods for participating in events. These methods parallel those that Crafty and each entity implement.

When a system is created, a new object is created with these methods, and then it is extended with the methods/properties of a provided template object. An init method and an events property can be provided that fill the same role as for components. By default the system will be lazily created -- that is, it will only be created when referred to for the first time. (This is what we do now with Crafty.canvasLayer and the like.)

In the future, we'll probably want to extend the capability of systems. In particular, I envision that components and systems with the same name would have some connection -- perhaps the system could have methods for operating on all such entities at once.

I tested moving canvasLayer, domLayer, and webgl to this new paradigm and it seemed to work ok, but the actual switchover will happen in a new PR.

There were some small updates to the callbackMethods mixin required to implement this properly.

[mucaho]

I like the idea very much. Conceptually it's like a singleton entity and component in one, which can be further composed of smaller systems. My thoughts:

• as it's like a singleton entity and component combined, it should be familiar to and sport similar methods to Crafty.e and Crafty.c - though this can be gradually added in future
  • in particular: remove-/add-/has-/require SubSystem, defineProperty(getter/setter), attr, timeout, getSystemId
  • also make the systems work with Model
• continuing above's thought, I would like it to be able inherit multiple objects/systems like Crafty.e. In this case it would be favorable to alter the contents of subsystems when modifying a property in supersystem with this.subSystemProperty= newValue:
  • Crafty.system("PhysicsSystem", "CollisionSystem, GravitySystem", {...}); - looks up systems by name
  • Crafty.system("PhysicsSystem", "CollisionSystem", "GravitySystem", {...}); - alternative signature (altough this could be confusing)
  • Crafty.system("PhysicsSystem", collisionSystem, gravitySystem, {...}); - systems are given by object (although who has the other systems in scope when creating new system? :) )
• what about a shorthand method instead to further stress the similarities to Crafty.e and Crafty.c -> Crafty.s(...)

Now onto implementation concerns:

• Why is the remove method called on the template object, rather than on this?
• I would like to see a test case where a system extends a system Crafty.system("Super", Crafty.system("Base", {...}));

[starwed]

I should explain something about the intended usage. The object that you pass in to Crafty.system isn't the system itself, but a kind of template for it. (Much like how a component and an entity are different things.) For instance, when canvasLayer is switched to this system:

• Crafty will define an object, probably called just Crafty.canvasLayer for now.
• On init, Crafty will call .system("Canvas", Crafty.canvasLayer).
• This will help to allow multiple canvas layers; you could later call .system("CanvasUI", Crafty.canvasLayer), using the same object as the template.
• It'll also help a bit if you stop and then reinit crafty -- all systems will be created anew from the template objects.

I would like it to be able inherit multiple objects/systems like Crafty.e

This isn't quite what I'm going for -- each thing labelled a systems should be complete in and of itself, so once you've defined a system you probably don't want to inherit from it. There is the extend method, which will make it easy to share common functionality between systems through mixin objects. Since one of the main goals of this whole setup is to abstract out how we handle render layers, that'll be a good test of how well it works. You'll be able to create canvas/webgl/dom layers, but each will share a bunch of methods.

Obviously once I have some experience actually working with this code, I might change my tune on what it should do! :)

Why is the remove method called on the template object, rather than on this?

Initially I had that called destroy, rather than remove, which means it had to be called on the template. I changed the name for consistency with how component methods are named, but didn't change the logic otherwise. I'll look at that again and probably change it to make more sense!

I would like to see a test case where a system extends a system

See above for why I don't think it makes sense specifically supporting this.

[mucaho]

Ok, now I understand your use case a bit better.

So given your use-case:

// creating a system
var canvasLayer = { ... };
Crafty.system("Canvas", canvasLayer);
Crafty.system("CanvasUI", canvasLayer);
// retrieving a system
Crafty.system("Canvas");

what is the difference, advantages/disadvantages over

// creating a system
var canvasLayer = { ... };
Crafty.c("CanvasLayer", canvasLayer);
Crafty.e("Canvas", "CanvasLayer");
Crafty.e("CanvasUI", "CanvasLayer");
// retrieving a system
Crafty("Canvas").get(0);

It seems like Crafty already has the tools to achieve something like this (no need to reinvent the wheel), or am I missing something here? Perhaps you want to avoid looping over all entities in the selector?

[starwed]

It seems like Crafty already has the tools to achieve something like this (no need to reinvent the wheel), or am I missing something here? Perhaps you want to avoid looping over all entities in the selector?

There are a few reasons for adding a new type of object, rather than reusing components/entities.

• Entities are initialized directly on creation, while we'll often want to init systems later on
• I want each system to have its own name, which is a bit tricky to do with a collection of entities
• Like I mention, I'd want systems to have some built in methods for handling collections of entities and other special interactions

It also just didn't feel right to me to make the viewport or graphical layers into entities. It's probably technically possible, but I think it makes sense to make the distinction clear in the code. They're different concepts, and they're going to be used in different ways.

[mucaho]

this is what I meant: why not call the remove method with this as the context, rather than this._systemTemplate? (the remove method should get copied over from the template)

if (typeof this.remove === "function") {
    this.remove();
}

[mucaho]

It also just didn't feel right to me to make the viewport or graphical layers into entities

Ok, my main concern is to avoid code duplication. Now we will have e.g. Crafty.bind, Crafty.e.bind and Crafty.system.bind. It would be nice to abstract out this methods somehow (could be done in a later PR).
Other than that, looks good.

[starwed]

Ok, my main concern is to avoid code duplication. Now we will have e.g. Crafty.bind, Crafty.e.bind and Crafty.system.bind. It would be nice to abstract out this methods somehow (could be done in a later PR).

I actually tried to do this, but the functions really do require different logic in different contexts:

• on the global Crafty object, trigger fires every callback, and in the future, I'd like for systems to trigger all the components they own (i.e. Crafty.system("Grid").trigger() works on all "Grid" components.)
• All those functions can all be called on a group of entities (all the entities matching a selector) which requires logic for looping through them -- that doesn't make sense in the context of a system or the global Crafty.

What I ended up doing was having the underlying logic abstracted out -- that's what the _callbackMethod stuff that landed in #869 is for -- then Crafty, entities and systems implement thin wrappers around that.

[starwed]

Other than that, looks good.

Thanks for the review, because these are definitely choices that should be well justified!

There are a couple of small changes I'll push (using Crafty.s instead of Crafty.system is a good idea).

[starwed]

I pushed theses changes: renamed to Crafty.s and made it so that remove is called directly on the system, rather than from the template.

[mucaho]

Great, I suggest adding defineProperty(getter/setter), attr, timeout, Model integration, construction example and retrieval example of systems in the documentation later down the road.

[mucaho]

Can we merge this?

[starwed]

While working on a (failed, I got really sick last week) 7drl, I didn't see any huge issues with this, but there are a couple things that should be addressed.

• I realized that you can't refer to a system inside the init function of that system, or you'll get stuck in an infinite loop. I'll add a note to the documentation (don't create entities in init())
• More importantly, there is some bug in the generic _callback code I need to sort out. I think the problem crops up when you destroy an entity in response to an event when there are callbacks left.

Once I fix that bug we can merge this.

[starwed]

Fixed the bug (and added a test for it), and added the doc note.

I'll go ahead and merge this once Travis reports back (the tests all passed locally, though).

[mucaho]

I'm sorry that you fell sick and didn't finish your project. It's a very exhilarating experience to finish such a project successfully in a few days :)

I just played with event binding in my code, and had a programming error when binding to an event on the global Crafty instance.

Crafty.c("MyComponent", {
    init: function() {
        Crafty.bind("MyEvent", function() {
            this.myData = "example"; // this refers to Crafty here, not the entity that will have "MyComponent"
        });
    }
});

Was this always the case that this referred to Crafty when Crafty.bind was called and conversely to the entity when entity.bind was called?
It's not a bug of course, because this behavior is clearly documented. I am just wondering if in an earlier Crafty version the this context was captured by the bind caller, so that the above code would have actually worked?

[starwed]

Was this always the case that this referred to Crafty when Crafty.bind was called and conversely to the entity when entity.bind was called?

I'm pretty sure -- we always triggered the callback with something like callback.call(context, data), which would override any previous call to bind.