Volcano

[Chosenundead15]

Volcano is fully working, armed_grenade was made a component again as it was a dependency of volcano previously and is required to spawn them.

[kindaro]

Did you consider using standardized explosives from #113? Possibly it could be an economical way to achieve the effect of armed grenades and erupted items with much less code.

[Chosenundead15]

Did you consider using standardized explosives from #113? Possibly it could be an economical way to achieve the effect of armed grenades and erupted items with much less code.

Didn't even notice the standarized explosive were merged already. I don't think they were when I started working on them. EruptedItem doesn't make sense to use it as is purpose is not to make explosive but a trait to spawn a lot of items from volcano(and don't have the collider be an obstacle). Having a volcano spawn swords should be valid for example. In contrast, having explosive implement the trait could make sense.

At the moment, ArmedGrenade already existed in the codebase and necessary for Grenades(initially sharing files) and Volcano, doing the refactor will add changes to Grenades that might be not the best idea to do for this Volcano PR, but I'm open to using the standarized explosive on armed grenades and agree it would be more economical. If doing the refactor first or merging this PR and refactoring later is better, I'm not sure.

[kindaro]

I should suggest to merge first, refactor later.

Why? Because a pull request accumulates merge conflicts over time, so the longer it exists, the more work it creates. Better to merge sooner.

But I am not an authority on this topic. This is nothing more than an advice.

[grufkork]

This looks good, but I'd like to raise the question of adding some kind of entity-component system to the game. ArmedGrenade is required to call eruption_update from its fixedupdate regardless of whether it is spawned from a volcano or not. What would be neater is if fixedupdates could be registered to the node, which then calls each of them. That way, it could even be possible to add an item as EruptedItem without modifying the regular fixedupdate. This would be a greater issue if you want to turn an explosive into an erupted item, as then you'd have to change the fixedupdate for all non-erupting explosives. I am facing the same issue for adding kickbombs, which behave exactly like a regular explosive except that if a player collides with it, it is kicked away. Being able to "tack on" behaviour like that would be very useful, and might allow any item to be erupted (especially once more items are generalised (guns are as of #136)) without modifying the source. Thoughts on this?

[olefasting]

I agree with what is being said here, and I don't like "bidirectional" dependencies and think the grenades should be completely separated from volcano, except for the trait implementation, but I'll merge this and we can refactor later.

[olefasting]

Also I agree withj @grufkork that an ecs should be discussed. It promotes better workflows, in my opinion, and it would eliminate a lot of the problems we discuss on other issues. I have not run into performance problems with MQ scenes yet, but from what I can gather, ECS will offer far bettet performance, as well. There are also libraries for this that offer parallelization etc.

We should probably create a separate issue for ecs, though