Use composition for a improved BlockType system

[Tonodus]

The currently used BlockType/ItemType system is extended by inheritance. That's a big design mistake IMO and it should be changed asap to a composition based one.

An examples of problem with the current BlockType system:

Same problem when implementing TrapDoors (place logic & openable) or FenceGates (directional placing & openable). You cannot let BlockOpenable extends BlockDoor, since that would cause FenceGate problems and you cannot let BlockDoor extend BlockOpenable, since this would cause BlockIronDoor to open with right-click... and since we write in java, BlockWoodenDoor cannot extend both BlockDoor and BlockOpenable.

Next example:

Ladder could be replaced by (redstone)torches etc.

Same as above: Inheritance causes Diamond of Deaths

So what about solutions? Instead of inheritance Glowstone should use composition! How exactly that can be achieved should be considerated and asap decided, because almost every added BlockXXX.java needs probably changes when introducing that.

My thoughts:

• Simple composition:

        class Openable { void onRightClick(GlowBlock b) {...} }

        class Door { void onPlace(GlowBlock b) {...} void onBreak(GlowBlock b) {...} }

        class WoodenDoor extends BlockType {
            private Openable openSystem;
            private Door doorSystem;

            @Override void onRightClick(GlowBlock b) { openSystem.onRightClick(b); }
            @Override void onBreak(GlowBlock b) { doorSystem.onBreak(b); }
            @Override void onPlace(GlowBlock b) { doorSystem.onPlace(b); }
        }

Pros: easy to implement Cons: much methods only for redirecting, need to call the needed methods manually (leads to bugs etc.)

• Dynamic composition with Optional:

        interface BlockComponent {
            void onRightClick(GlowBlock);
            Optional<? extends TileEntity> createTileEntity(GlowBlock);
            //...
        }

        class Openable extends AbstractBlockComponent {
            @Override void onRightClick(GlowBlock b) {...}
         }

        class Door extends AbstractBlockComponent { 
            @Override void onPlace(GlowBlock b) {...}
            @Override void onBreak(GlowBlock b) {...}
        }

        class WoodenDoor extends BlockType {
            WoodenDoor() {
                super(new Openable(), new Door());
             }
        }

Pros: Clean classes, easy to extend (just add an object to that constructor, no need to call some methods or such... all handled dynamically by BlockType.java) Cons: performance issues? (probably not); more complex BlockType class

• Dynamic composition II:

  interface BlockComponent {
      void createTileEntity(Setter<TileEntity> t);
      static interface Setter<T> {
          void set(T obj);
      }
  }
  // rest is same as above
  

Pros/Cons: Not really sure, personal I wouldn't do it, it just came into my mind

Personally I'd prefer option 2. I think active exchange would be great.

[gabizou]

Now you're beginning to think in an EntityComponentSystem manner. Blocks could very well be re-implemented in such a manner for the sanity of implementing these types of features without a problem.

[Tonodus]

@gabizou one implementation could be an ECS, where blocks are the entities and BlockTypes are the systems / event hooks; in fact, blocks and entities could be in the same ECS.... However maybe it's not worth it. The problem is not to implement sth., the problem is to decide the correct way of implementing sth.

[turt2live]

I actually ran into a critical design problem when playing with merging PRs last night. It was a small problem, but still enough to cause enough headaches.

Take anvils for example. With a mix of @Tonodus' and @Tonodus' other PR (physics and requiring specific tools to get drops), anvils are nearly completely implemented. But @Tonodus was forced to implement a BlockFalling type and the tools PR implemented a BlockNeedsTool type. Both are perfectly valid, but BlockAnvil can't extend both. What I ended up doing was choosing BlockFalling as the super class and copy/pasting the tool logic.

As described in the issue, anvils are not the only case though. There are many more that will run into conflicts. However the anvil case also demonstrates that the system in place is already "component-like" in the sense that a block can be identified as "falling" and "needs a tool to break", it's just poorly designed as-is.

[ZephireNZ]

I definitely agree this is a good way to go. I also agree with @Tonodus, in that the entity ECS and the block ECS could share the same implementation for components.

[Tonodus]

I just wanted to mention, that a ECS could be an overkill for the blocktype-system.

• There's no need for systems that pulse every tick for blocks, like (real) entities do
• There aren't that much components you could attach to the entity/blocktype, having probably
• The most important thing blocktypes need are the events, which can be easily fired through the methods every blocktype provides.

So an ECS might be just not needed for great extendable blocktypes and an Event/Listener system might be sufficient.

[gabizou]

There's no need for systems that pulse every tick for blocks, like (real) entities do

I can think of a few blocks that can react with these types of systems, for example:

• Furnace blocks have a timing to cook
• Brewing Stands brew

All in all, granted there isn't a need to pulse over every blocktype in the world, the systems aren't designed to do that. The ECS isn't designed to just pulse, but handle all the block logic that otherwise would have to be written in to classes like BlockWoodenDoor etc.

In the case of fire burning blocks, there is a thing called FireComponent where the block has a burning time, sometimes the randomized FireBlockSystem would remove some blocks faster than others, but all the more reason for handling fire events that way. Another plus is that to add more fire burnable blocks, there wouldn't be a need to write it in directly to the Block class itself.

Furthermore, the example of Ladders is a perfect example of two components in a system.

[SpaceManiac]

I definitely agree this is needed. Inheritance is useful but it falls apart when blocks have multiple aspects this frequently. I'm somewhat leaning towards the first option @Tonodus presented because it's so easy to understand and implement but the second option is looking fairly interesting as well. Not sure why the focus on Optional though, surely null is enough in this case? (Believe me, I know the benefits of Optional, but that's a whole other discussion...)

Block ticking (timed, random, w/wo tile entities) is quite complex and probably should be addressed separately.

[Tonodus]

@gabizou

I can think of a few blocks that can react with these types of systems, for example [...]

Valid point. Let's say almost every BlockType doesn't need them.

All in all, granted there isn't a need to pulse over every blocktype in the world, the systems aren't designed to do that

Of course, they aren't. But that's the string point of an ECS: Each system fetches entities dynamically and does it's logic with the entity. However, having only one or two systems doesn't justify an ECS.

The ECS isn't designed to just pulse, but handle all the block logic that otherwise would have to be written in to classes like BlockWoodenDoor etc.

Well, that's not correct. We'd still have classes for WoodenDoors, but this time we'd have classses for open/close-handle and placement-handler. See one of my options above for a similair implementation for composition than an ECS.

Another plus is that to add more fire burnable blocks, there wouldn't be a need to write it in directly to the Block class itself.

I can't follow you. If there would be a new fire-block (which is very unlikely) we'd just have to add a line in the ItemTable.java.

Furthermore, the example of Ladders is a perfect example of two components in a system.

Which two components do you think of? Components are data, so I can think of the component attached. Also which system? What system would be needed for ladders? None. There must be a placement handler (which would be done best by event-listeners).

I am not against having an ECS for BlockTypes, I'm just pointing out that there might be not the need for one. For your entitiies-branch you used artemis. Iirc artemis doesn't support events, which would be a big disadvantage, since most of logic for BlockTypes would be handled by listeners.

@SpaceManiac

Not sure why the focus on Optional though, surely null is enough in this case?

After re-thinking about this, I agree null is enough for this. Primitive values must be wrapped then, but with Optional they would be wrapped two times...

Block ticking (timed, random, w/wo tile entities) is quite complex and probably should be addressed separately.

Let's look at the plants PR (#431). Ticking is handled quite neat event based there.

[SpaceManiac]

Based partially on @Tonodus's PR and partially on Sponge's BlockType/BlockState system, some experiments are up on the blocks branch. Interested in any feedback on the composition system or the BlockState implementation.

[PythonJedi]

(Moderately interested in this project, not familiar with production Java code)

Quick clarification: Why are interfaces not being considered here? ~~This situation is pretty much what interfaces were designed to solve in the first place (inheritance collision).~~

After glancing at at the code and the bukkit API (which makes little sense right now), I understand why interfaces wouldn't work. Here the problem is not to make sure that, for instance, WoodenDoor has both a placeDoor() and onRightClick(), while IronDoor only has placeDoor() and FenceGate has onRightClick().

That doesn't help because WoodenDoor.placeDoor() and IronDoor.placeDoor() have the same logic, along with onRightClick() for WoodenDoor and FenceGate. If Java allowed manipulation of methods themselves like Python or C, it could be easy enough to just assign the relevant logic at will. Since methods have to reside inside classes or objects, the proposed system makes the most sense.