Proposal for component lifecycle methods #103
Comments
|
My reason for it was that I thought you don't need these methods for every single component AND performance reasons. What would be interesting for me is how much slower it gets when we add onAdd/onRemove as default functions to a component and call it in the specific ComponentHolder functions where the hooks are currently called. There is a benchmark sourceSet with a manual.kt file in it. You can run it before and after your changes for a quick comparison. Run it a few times and take the average values. This is what I usually do to test such changes. The gradle benchmarkJvm task is the real one but takes veeeery long, that's why I don't recommend it for quick testing 😉 Also, ignore the result in the comments of manual.kt file. You will most likely have different numbers. Those are the ones from my PC which was also used for the benchmark table in the README.md. My laptop e.g. has completely different values and you will most likely also have different ones. That's why you should compare it with your own numbers and run it before and after your changes. |
|
Ah! I never considered that it could have a performance impact - I figured it would be the same as with the hooks. I did a spot of research, and even though the default implementation of the lifecycle methods would just be Performed the benchmarks - from what I can see, there isn't much difference 🎉. I ran 3 batches of benchmarks at different times of the day, and each benchmark ran 10 times instead 3 times to smooth out variance. Before; After; |
|
Looks indeed very similar. I could imagine it having an impact on Android. From what I know there is a limited amount of functions you can put on the stack. But theoretically it will be the same if you'd use a hook for each component. Otherwise, the current version is more optimized for that particular backend. Out of interest please run the BenchmarkJvm gradle task with and without the changes and share your result. If it is still nearly the same then I don't have a problem to change it if people prefer that. |
It would definitely be simpler for new users to pick up on and make the code a little cleaner to write. This would remove the need to add hooks to the world definition as well, if I'm understanding it correctly. |
Maybe, but technically the hooks can be defined in any scope and take any variable as a closure (not necessarily good or bad in my opinion). Lifecycle methods would be restricted to the variable scope of the component. I.e.; val foo = Foo()
engine = world {
components {
onAdd(Bar, { entity, barComponent ->
barComponent.thing = foo
})
}
}Although one could use injectables to access |
|
And the results from jvmBenchmark are in! Looks like there is little difference, although I'm not sure how thin the margins are supposed to be. Before; After; |
|
Interesting that Artemis simple benchmark performs way faster on your machine. For me on both PC and LAPTOP it is always slightly slower. Anyway, those benchmarks are imo "extreme" scenarios and the little slowdown in the simple benchmark is not that big of a deal I'd say. As you mentioned, the component hooks are then no longer necessary and can be removed (entity hooks are still needed though) and I agree, it makes it easier to define the add/remove component logic. Since it runs in the context of the world, you also have access to everything through injectables. Do you want to create a PR for it? I can review it and it can be part of 2.4 which will be released in the near future. |
|
Yeah I'll hopefully make a PR within the next few days! |
|
@geist-2501: I am currently migrating my example projects to 2.4-Snapshot and I wonder, if it is possible to pass the component as a second argument to the lifecycle methods. The reason is, that I don't like the annotated this variables a lot like here: override fun com.github.quillraven.fleks.World.onAddComponent(entity: Entity) {
this@PhysicComponent.body.userData = entity
}
override fun com.github.quillraven.fleks.World.onRemoveComponent(entity: Entity) {
val body = this@PhysicComponent.body
body.world.destroyBody(body)
body.userData = null
}I want to just name it edit: oh, I just notice you can simply remove the entire this part because it runs in the context of the component as well. I think this is fine then - nevermind! |
|
Yup - I was just about to to say that! :-) As a side note - I've updated a fork of the wiki to contain the new lifecycle method stuff - would you like me to open a PR for it or will you perform the wiki update yourself? It seems like contributing to a project's wiki is a little bit janky. |
|
I plan to update the wiki after 2.4 is live but if you already have something then please share it! Maybe just copy&paste the markdown into this issue then I can take it over. |
|
Updated the wiki locally and I can provide it as a patch file - just like in the olden days before PRs 😛 You can use |
|
Thank you! I cannot apply the patch because it seems like it tries to find a Component.md/Entity.md/... file which does not exist (?). Anyway, I will then manually take over your changes and I will keep the ComponentHook info for history reasons, if people are using a version before 2.4. Maybe on a separate wiki page. |
Hi all!
I've been tinkering with Fleks and, while the component hooks are lovely and work perfectly, I wondered why there isn't lifecycle methods on components instead.
My specific use-case is that my components can raise events in order to talk to the UI. To do this they need an event manager which I inject - however, components don't have the whole Fleks context thing so I can't inject directly. So instead, I have to initialise it via a hook (which do have a Fleks context), which works great, but took me a while to think of 🗿.
Wouldn't lifecycle methods be more developer friendly? For instance;
I've forked this repo and got the lifecycle methods working, but I'd be interested to know peoples opinions on whether this is actually a better approach :)
Also Fleks is awesome! Very happy that I made the switch from Ashley halfway through my project.
The text was updated successfully, but these errors were encountered: