Shower Thoughts - Make the Goose (now the Duck) always quack like the Duck

[Zidane]

Awesome title huh?

So I had the thought yesterday while, you guessed it, in the shower to create a new type of mixin that would cover the following situation:

For Inventory, Minecraft has an IInventory interface. Lets take these two scenarios:

class InventoryBasic implements IInventory {}

class InventoryBackpack implements IInventory {}

While it would be nice if mods used InventoryBasic, most just implement IInventory. Not a problem as step one in becoming a fake Duck is:

@Mixin(IInventory.class)
abstract class MixinIInventory implements api.Inventory

Now I'm not silly enough to think that it'll magically implement the interface. I'm banking on it NOT doing so, I just want to add our api interface to the hierarchy. If possible (or can be made possible), I would like for my new Duck to always quack like a Duck amongst other Ducks.

Riddles aside, I'd like a Mixin where I can say "Take the following methods and mix them into anything that implements the target". Basically my thought is I could magically make mod inventories actually have OUR methods by injecting the code into it that wouldn't change. Lets change our Mixin...

@Mixin(IInventory.class, children=true)
abstract class MixinIInventoryChildren implements api.inventory {
// Perform intrinsic magic here so api and minecraft interfaces don't conflict
// Implement api interface methods, such as the ones for querying, by using what we have...the minecraft interface
}

That is as rough as it gets both hopefully someone out there understands what I'm going for.

[Mumfrey]

There's two things here: there's a reason what you're suggesting will never happen, and there's a second reason as to why it doesn't have to.

1. Mixin is never going to get "scattergun" targets like this, primarily because to do so would require loading, parsing and inspecting every loaded class simply to find out if it implements an interface, and then doing the extremely expensive pre-application processing which is normally done as a one-off for each target in a single pass (and is the entire point of environments). Basically doing anything of this sort violates a lot of the core principles of Mixin which are the source of its efficiency and robustness. If you're not convinced that hierarchy validation is complicated enough, then go read this article which should illuminate the issue significantly. Basically, as it stands, Mixin is not designed to do (for specific reasons, rather than arbitrary ones) what you're talking about here, and the issue extends outside the immediate one of "identify the targets" to the entire framework of class metadata which is used to do the underlying mixin pre-processing.

2. All of this seems a bit doom and gloom, but the point is that it's wholly unnecessary - on both a general level and for the case you're talking about in particular.

   For the general case, what you're talking about can basically be supported by adding support for interface mixins when running under Java 8, this would allow adding default methods to a target interface, and thus solve the vast majority of what you're talking about.

   For this specific case, there's no point anyway. With inventories, the bulk of the heavy-lifting when "talking" to an inventory is actually handled by Lenses. Known inventories have a known set of Lenses which represent their virtual internal structure and are used to access the underlying target inventory. The Lens objects are just wrapped in an InventoryAdapter which actually implements the Inventory interface and delegates access to its Lenses. Known inventories simply act as their own InventoryAdapter, which saves an object, whereas any unknown inventory is simply an InventoryAdapter with a single, default, Lens inside it.

The benefit of this is that the only thing that mods need to do to provide support for our inventory system (directly, all inventories get support indirectly) is supply lenses for themselves, they don't have to implement the complex and somewhat cumbersome Inventory interface, since querying and otherwise interacting with the inventory is actually handled by the adapter.

So in a nutshell, once we move Sponge onto Java 8 I will look at adding support for default methods and mixins being interfaces, which will provide a better answer to this than something which would be incredibly inefficient by design.

[Zidane]

@Mumfrey

For this example, can you go into your design a bit with InventoryAdapter and Lens?

Vanilla

// Could be an interface, maybe
abstract class AbstractLens<T extends IInventory> {
    T wrapped;

    abstract Iterable<T> slots();
}

// The basic lens, used pretty much for chests
class BasicLens extends AbstractLens<InventoryBasic> {
    // Does the legwork in doing transactions
}

@Mixin(InventoryBasic.class)
abstract class MixinInventoryBasic implements OrderedInventory {
    // TODO Need a registration system so mods can register their own lens and handle all API 
    // interactions
    private AbstractLens lens;

    @Inject(method = "<init>", at = @At("RETURN"))
    public void onConstructed(String name, boolean displayCustomName, int slots, CallbackInfo ci) {
        // TODO Lookup lens from registry by class and set it on inventory construction
        lens = new BasicLens(this);
    }

    @Override
    <T extends Inventory> Iterable<T> slots() {
        return lens.buildSlots();
    }

    // Would handle query methods and delegate back to the Lens to ask for items
}

Yeah after throwing down some pseudocode, I see what you are doing. I might not be completely right with my code above but I think I'm headed in the right direction.

I've also left out the fallback adapter and lens but I know how it would work as well.

[Zidane]

Further thoughts:

A fallback adapter and inventory is only needed if the mod implements IInventory directly. If they extend one of the Vanilla impls then the Vanilla injection above for each impl would act as the fallback.

Otherwise:

class GenericInventory implements api.Inventory {
    AbstractLens<IInventory> lens;

    public GenericInventory(IInventory inventory) {
        // TODO Same as MixinInventoryBasic, look to see if a mod registered a lens
        lens = new GenericLens(inventory);
    }
}

class GenericLens extends AbstractLens<IInventory> {
    // Do Generic things
}

With this, the only thing I haven't solved is things like PlayerInventory which contain multiple inventories...but that should be easy enough by mixin into it and storing the lenses.

[Mumfrey]

Lens is an interface and doesn't wrap an IInventory, it provides methods for accessing one. Thus all Lens methods take an IInventory as their first argument, this allows entire lens hierarchies to be shared if necessary, and allows them to be kept around and re-used without memory leakage of keeping a reference to an actual IInventory lurking around. In my early versions I only had adapters but the adapters got very complex and messy very fast. Now that adapters have no logic in them at all, they're very simple and all the complexity is condensed down into various Lenses and the query executive (which also only cares about lenses and doesn't care about the adapters).

[Mumfrey]

With this, the only thing I haven't solved is things like PlayerInventory which contain multiple inventories...but that should be easy enough by mixin into it and storing the lenses.

You are duplicating work that I've already done, Lenses are hierarchical.

[Zidane]

@Mumfrey

I don't have the source you've worked on and Inventory is currently a blocker for entering beta phase of Sponge so unless you can pass the source privately, I have to start implementing this myself now.

I'm putting the pieces together of your vision for the Inventory API and how it will work in the implementation.

[Mumfrey]

Well SoS is coming up in just over a week, you will have my implementation in plenty of time to put together some demo plugins for that, that's my goal. I'm unavoidably busy this week but between these events and going to Madrid I have a brief respite to release the impl to general consumption. I have been insanely busy these last two months and things just haven't worked out as planned. In just under two weeks everything will be done with and I'll be able to resume a normal level of service, I'm aware that's after SoS but at least by then you'll have an idea of what I'm harping on about.

I realise you need a contingency plan though, so I understand why you're trying to do what you're doing, but trying to duplicate my system you're going to run into the same obstacles I have, and I believe I've come up with the best answers to all the technical challenges which implementing this API has presented.

[Zidane]

@Mumfrey

SOS is this Saturday and the idea was to demo something this Saturday to the public.

[JBYoshi]

@Mumfrey So basically, if someone wants to mix in to an interface, they should just use an adapter?

[Zidane]

@JBYoshi

No, this is for something else in Sponge...nothing really to do with Mixin.

[Mumfrey]

SOS is this Saturday and the idea was to demo something this Saturday to the public.

Oh. I'm sure I remember discussing this at SPAM and it being while I was in Madrid, which is next week. Plus I though SoS was always the first saturday in the month. Oh well in that case do whatever you need to, you'll have my code next week, I don't have time before then.

[Zidane]

@Mumfrey

We scheduled SOS for this Saturday as it was the time in-which you wouldn't be gone so therefore you could be there to talk about the pushed Inventory implementation.

We'll have to shuffle up the lineup a bit then.

[Mumfrey]

I'm not around this weekend either, so either way I've got my dates all wrong. At the moment I'm just glad when I get through a day without going completely insane from sleep-deprivation, so I'm not all that surprised. As I said, once I'm back from Madrid Games Week and had a couple of days to recover, normal service will be resumed. Until that time please keep your arms and legs inside the vehicle, and your lifejacket is under your seat.

[bloodmc]

@Mumfrey I have a very special case for needing this functionality... I am working on a new branch which captures blocks during a single tick and fires off various events. One type of capture I am trying to do is WorldGenerators(Populators such as cactus, flowers, swamps etc.). In order to do this efficiently, I need to be able to inject into the "generate" method for any class that extends the abstract class 'net.minecraft.world.gen.feature.WorldGenerator.class'. At the HEAD of the method, I would set a flag to start my capture and at the RETURN I would call my post capture method to fire off an event for all block transactions that took place. This would allow me to easily identify all blocks related to a particular generator such as WorldGenFlowers. Currently the only way to do this is to check the caller class during World.setBlockState but this causes a problem since there could be multiple instances of said generator and I would have to keep track of each one.

The following is what I would like to be able to do

@Mixin(value = net.minecraft.world.gen.feature.WorldGenerator.class, children = true)
public class MixinWorldGenerator {

    @Inject(method = "generate", at = @At("HEAD"))
    public void onGeneratePre(World worldIn, Random rand, BlockPos position, CallbackInfoReturnable<Boolean> ci) {
        IMixinWorld world = (IMixinWorld)worldIn;
        world.setGeneratorCapture(true);
    }

    @Inject(method = "generate", at = @At("RETURN"))
    public void onGeneratePost(World worldIn, Random rand, BlockPos position, CallbackInfoReturnable<Boolean> ci) {
        IMixinWorld world = (IMixinWorld)worldIn;
        world.handlePostTickCapture(this);
    }
}

There is one issue from what we've been discussing in dev chat, and that seems to be subclassing. I'll just use my alternative method for now by checking getCallerClass

[Mumfrey]

@bloodmc I follow you, injections are definitely a plausible reason for wanting to do this which aren't covered by the cases mentioned above. However the issues with achieving it still stand so I'll need to put some thought into how it can be done without being quite ostentatiously inefficient and/or impossible without rewriting a big chunk of how Mixin does its class metadata generation.

If we're only talking about classes in the game codebase it's potentially simpler, but if you (for example) wanted to mix into all mod-provided classes which implement an interface as well then it gets quite messy very quickly.

Let me apply some brain-runtime to it and I'll see what I can come up with.

[bloodmc]

I would need this functionality for mod classes too. Thanks for looking into it!

[simon816]

@Mumfrey I know this is unrelated to the mixin issue, but this inventory Lens idea seems very interesting and I'd like to know if you have any code on it?
We're about to start implementing the inventory API and it'd be great if we could get a head start. Just a dump of code on a branch in SpongeCommon would be fine.

[yuuka-miya]

Is there any further update on this? As discussed earlier, I'd like to bring back my Entity injection to Mixin, and are waiting on progress for scattergun mixins before I can do that.

[gabizou]

@luacs1998 there's a problem with that sort of injection, namely, if you ever have a case where you have an X extends Entity where X overrides attackEntityFrom(DamageSource;F)Z, if I'm understanding your injection correctly, you'd have any hierarchical extension that calls super.attackEntityFrom(DamageSource;F)Z to throw yet another duplicated event. An easy example of this: Entity is the parented method, EntityLivingBase overrides with what powers most all living entities to "die" after their health has reduced to nothing, EntityPlayer overrides yet again to perform some more logic depending on the player's sleeping state, and difficulty, then EntityPlayerMP yet again, extends and performs even more logic and finally calls super.

As it stands, there'd be now 4 injections, three of which are redundant, and two of which are entirely out of order with regards to what is happening in the game.

[yuuka-miya]

So basically, I can just patch Entity, and hope everyone else has the decency to call super() back to Entity?

cc @olee

[Mumfrey]

@luacs1998 yes this is very much alive and is close to being merged, 0.5.6 represents preparatory work for this since 0.6 is a big change so I'll be merging it in stages so I can test each part.

[olee]

@luacs1998 The reason why we need those "scattered" mixing is exactly because we know not every implementation calls super.
Instead we want our code to run at the beginning of each implementation of the function (handling multiple calls to our injected code because of super calls is our responsibility then).

[Mumfrey]

You may need to keep your transformers for now then, since the multiphase update will only support direct implementors of an interface rather than classes subclassing a particular class, though I can look into extending to support that in a subsequent version.

[olee]

That would be great!
Currently it works with that lightweight injector I wrote quite nice, but it's missing all that security stuff etc. that is part of the Mixin environment so that would be nice.

[Mumfrey]

Don't worry, I'll definitely put it on the wish list. As you know I'm a little over-methodical and over-cautious with adding features so progress tends to be slow and measured rather than fast and gung-ho, but supporting it will definitely be possible with multiphase so I will put it down as a feature on the to-do list.

[olee]

Maybe even a higher priority feature? 😄
Because at least we use it to patch any attackEntityFrom method in entities to allow permission control of this event (which is otherwise not possible in all situations).

[JBYoshi]

@olee If you put it at the start of every implementation, then if one implementation does call super(), then wouldn't your injected code get called twice?

[olee]

@JBYoshi YES, and that's what we need (want).
Simple example:

class EntityLivingBase extends Entity {
    public void attackEntityFrom(dmg) {
        damagePlayer(dmg);
        super.attackEntityFrom(dmg);
    }
}

class EntityPlayer extends EntityLivingBase {
    public void attackEntityFrom(dmg) {
        playHurtSound(dmg);
        super.attackEntityFrom(dmg);
    }
}

If we only add our code once - let's say to EntityPlayer - and another entity that extends EntityLivingBase is hurt, it would not run our code at all.
On the other hand, if we inject it into EntityLivingBase, we cannot prevent the playHurtSound from EntityPlayer being fired.
That is why we need a code that is injected into every class that implements the attackEntityFrom method.

Handling of duplicate calls is done by our injected code.
In this case we have a map of already checked entities that is cleared on each update which suffices for this task.