Access Transformers and Mod Compatibility

[Reika]

I used access transformers for the first time today to avoid the need to reflectively call or set fields or methods, to avoid the performance overhead of reflection (as the code is called very frequently).

This became a problem when changing an EntityLivingBase method (dropFewItems(ZI)V) from protected to public. When running the gradle script, I was spammed with errors as it tried to apply my changes, as the subclasses of EntityLivingBase all override that method and keep it as protected. Since a protected field cannot override a public one ("cannot reduce visibility" error), this is not unexpected, but poses a serious problem:

Even if I were to patch all of the vanilla entity classes to use the method as public, what happens with mods, who likely also override the method and keep it protected? Is there some handler in Forge to fix this and avoid a crash, or to change the visibility for all subclasses of a given class?

[Reika]

Noone?

[Reika]

Are access transformers really something noone else uses?

[sequituri]

You may have to use reflection as I think you hit a hard point of ATs.

[Reika]

You may have to use reflection as I think you hit a hard point of ATs.

I would expect that there is some way to mark a method to be modified in all subclasses; this system is old enough, and I am surely not the only one to need something like this.

 

I can use reflection, but I am concerned with the performance; this method (used by a mob harvesting machine) is going to be invoked on every mob that passes over it, a number which can be in the hundreds per second.

[sequituri]

Some modders have used a cached reflection field without suffering lag penalties. You should only have to do this once for any field in a class and cache the accessor.

[Reika]

Some modders have used a cached reflection field without suffering lag penalties. You should only have to do this once for any field in a class and cache the accessor.

I already do that, but profiling has in the past indicated that method.invoke() and field.get() are still about 5-20x slower than direct access.

[Black]

One thing I sometimes use to access methods I cannot get to with reflection and am burdened by efficiency is code redundancy.

 

Basically partially recreating the method you need that is private or protected in your own class or subclass and use that one. Now I know that EntityLivingBase is quite complex but what you can do is create a new EntityLivingBase class, invoke it on load time to take from the original class the data it needs, build in a timer and at regular intervals adjust the data again. That way you can use that class and use it on maximum efficiency. The downside is that it produces duplicate code, reduces transparency, and depending on wether you need it or not, speed of updating it can weigh it down a bit, but still, it should be faster than reflection. And if you tweak it to fit your situation I believe you can just about do anything you need to do.

 

This isn't really a wrapper class as such since it creates its own methods but it can be used like that.

 

Just a note though, I wouldn't call this proper coding practice, but it works when it has to.

[Reika]

One thing I sometimes use to access methods I cannot get to with reflection and am burdened by efficiency is code redundancy.

 

Basically partially recreating the method you need that is private or protected in your own class or subclass and use that one. Now I know that EntityLivingBase is quite complex but what you can do is create a new EntityLivingBase class, invoke it on load time to take from the original class the data it needs, build in a timer and at regular intervals adjust the data again. That way you can use that class and use it on maximum efficiency. The downside is that it produces duplicate code, reduces transparency, and depending on wether you need it or not, speed of updating it can weigh it down a bit, but still, it should be faster than reflection. And if you tweak it to fit your situation I believe you can just about do anything you need to do.

 

This isn't really a wrapper class as such since it creates its own methods but it can be used like that.

 

Just a note though, I wouldn't call this proper coding practice, but it works when it has to.

 

I am not trying to do this on custom entity classes. This is for vanilla and other modded mobs, like Creepers, Slime Beetles, and Wisps.

[Black]

Well to be honest, I have no idea what you are trying to do, apart from making another nice machine, but from your previous reply I deduce a mob grinder.

If this is the case, how about this:

 

List<Entity> entities = ParWorld.getEntitiesWithinAABBExcludingEntity(player, player.boundingBox.expand(5.0D, 5.0D, 5.0D));
	for(Entity Ent: entities)
	{
		Ent.setDead();
	}

 

If you call this every say, quarter or half a second you should easily get them all without needing to cast, although that function too takes some time. player.boundingbox can obviously be substituted by your blocks boundingbox or coords.

 

And if you're trying something more interesting, please enlighten us.

[Reika]

Well to be honest, I have no idea what you are trying to do, apart from making another nice machine, but from your previous reply I deduce a mob grinder.

If this is the case, how about this:

 

List<Entity> entities = ParWorld.getEntitiesWithinAABBExcludingEntity(player, player.boundingBox.expand(5.0D, 5.0D, 5.0D));
	for(Entity Ent: entities)
	{
		Ent.setDead();
	}

 

If you call this every say, quarter or half a second you should easily get them all without needing to cast, although that function too takes some time. player.boundingbox can obviously be substituted by your blocks boundingbox or coords.

 

And if you're trying something more interesting, please enlighten us.

This is for the mob harvester. Depending on the enchantments and power supplied, it modifies the drop counts of the mobs it kills. I do that by watching the LivingDropsEvent:

@SubscribeEvent
public void enforceHarvesterLooting(LivingDropsEvent ev) {
	if (ev.source instanceof HarvesterDamage) {
		HarvesterDamage dmg = (HarvesterDamage)ev.source;
		int looting = dmg.getLootingLevel();
		EntityLivingBase e = ev.entityLiving;
		ArrayList<EntityItem> li = ev.drops;
		li.clear();
		e.captureDrops = true;
		try {
			ReikaObfuscationHelper.getMethod("dropFewItems").invoke(e, true, looting);
			ReikaObfuscationHelper.getMethod("dropEquipment").invoke(e, true, dmg.hasInfinity() ? 100 : looting*4);
			int rem = RotaryCraft.rand.nextInt(200) - looting*4;
			if (rem <= 5 || dmg.hasInfinity())
				ReikaObfuscationHelper.getMethod("dropRareDrop").invoke(e, 1);
		}
		catch (Exception ex) {
			RotaryCraft.logger.debug("Could not process harvester drops event!");
			if (RotaryCraft.logger.shouldDebug())
				ex.printStackTrace();
		}
		e.captureDrops = false;
	}
}

This is a mob whose type is only known at runtime and will be different for whatever the player decides to kill (including potentially exotic mobs like Withers and Minotaurs).

[Black]

Well, how about this then.

 

public final class KillMobs {

static ArrayList<EntityItem> ItemDropList;

public static void KillPresentMobs(World ParWorld, EntityPlayer player, int ItemDropAmount)
{
	List<Entity> entities = ParWorld.getEntitiesWithinAABBExcludingEntity(player, player.boundingBox.expand(5.0D, 5.0D, 5.0D));
	for(Entity Ent: entities)
	{	
		Ent.captureDrops = true;
		Ent.attackEntityFrom(DamageSource.fallingBlock, 100f);
		ItemDropList = Ent.capturedDrops;

		for(EntityItem DropItem: ItemDropList)
		{
			Ent.dropItem(DropItem.getEntityItem().itemID, ItemDropAmount);
		}

		ItemDropList.clear();
}}
}

 

It adds to the usual drops the amount you wish to add without reflection and displays the kill animation. It's not perfect yet, e.g. the arraylist should be replaced with something that can be modified more quickly, it doesn't modify the droprate(which you didn't ask) and it can't drop nothing(unless you destroy entities on the floor) but in general it works. And I don't think I need to tell you that the above is a static class so it is easy to test.

[Reika]

Well, how about this then.

 

public final class KillMobs {

static ArrayList<EntityItem> ItemDropList;

public static void KillPresentMobs(World ParWorld, EntityPlayer player, int ItemDropAmount)
{
	List<Entity> entities = ParWorld.getEntitiesWithinAABBExcludingEntity(player, player.boundingBox.expand(5.0D, 5.0D, 5.0D));
	for(Entity Ent: entities)
	{	
		Ent.captureDrops = true;
		Ent.attackEntityFrom(DamageSource.fallingBlock, 100f);
		ItemDropList = Ent.capturedDrops;

		for(EntityItem DropItem: ItemDropList)
		{
			Ent.dropItem(DropItem.getEntityItem().itemID, ItemDropAmount);
		}

		ItemDropList.clear();
}}
}

 

It adds to the usual drops the amount you wish to add without reflection and displays the kill animation. It's not perfect yet, e.g. the arraylist should be replaced with something that can be modified more quickly, it doesn't modify the droprate(which you didn't ask) and it can't drop nothing(unless you destroy entities on the floor) but in general it works. And I don't think I need to tell you that the above is a static class so it is easy to test.

How does this allow me to control things like dropping armor and/or player-only drops?

[imadnsn]

I'm pretty sure you can do DamageSource from player. I guess it wouldn't make sense to have a player for a machine but maybe a fake player would do?

[Reika]

I'm pretty sure you can do DamageSource from player. I guess it wouldn't make sense to have a player for a machine but maybe a fake player would do?

Yes, but that does not give me control over the normally-miniscule drop chances.

[Black]

Excuse the fact that I am somewhat late but I needed to find the time to give the minecraft code a look.

In any case I wrote these few lines and they work splendidly, the only thing that they don't do is increase the chance for rare drops, which, unsurprisingly, are safely put away behind dumbly written methods and variables.

 

If you do want that, the best way I see is a wrapper class around EntityLivingBase, something which I believe you stated you didn't want so I avoided it.

The other possibility is creating a hashmap with the entities as keys and everytime an entity is encountered it doesn't know yet, spawn about a thousand of them, capture the drops, check which drop are below a certain amount and add those drops in a List<EntityItem> to the value in the hashmap.

A bit convoluted to say the least, although I don't think the mobs would show themselves so the temporary lag should be negligent.

 

Other than that, is just about regulates everything you could want.

 

public final class KillMobs {

public static void KillPresentMobs(World parWorld, EntityPlayer parPlayer, int parItemDropAmount, AxisAlignedBB parAxisAlignedBB, int parArmourDropChance)
{
	List<EntityItem> OldentityItems = parWorld.getEntitiesWithinAABBExcludingEntity(parPlayer, parAxisAlignedBB);

	Random rnd = new Random(); //Loops through current entities and selects the mobs.
	for(EntityLivingBase Ent: Iterables.filter(parWorld.getEntitiesWithinAABBExcludingEntity(parPlayer, parAxisAlignedBB), EntityLivingBase.class))
	{
		if(rnd.nextInt(100) <= parArmourDropChance)
		{	//drop armor if the conditions are right(random is below chance and selected slot has armor)
			Ent.entityDropItem(Ent.getCurrentItemOrArmor(rnd.nextInt(4)), 0);
		}
		//kills entities and captures their drops(it does not prevent them from falling)
		Ent.captureDrops = true;
		Ent.attackEntityFrom(DamageSource.causePlayerDamage(parPlayer), 100f);
		List<EntityItem> ItemDropList = Ent.capturedDrops;

		for(EntityItem DropItem: ItemDropList)
		{ //adds extras for the amount specified by parItemDropChance for each item dropped
			Ent.entityDropItem(new ItemStack(DropItem.getEntityItem().itemID, parItemDropAmount, DropItem.getEntityItem().getItemDamage()), 0);
		}
		ItemDropList.clear();
	}

	//Checks if mobdrops are on, if not it will delete the mobdrops.
	if(parItemDropAmount  == -1)
	{
		for(EntityItem OldItem: Iterables.filter(OldentityItems, EntityItem.class))
		{
			for(EntityItem EntItem: Iterables.filter(parWorld.getEntitiesWithinAABBExcludingEntity(parPlayer, parAxisAlignedBB), EntityItem.class))
			{
				if(EntItem == OldItem)
				{
					EntItem.setDead();
}}}}}}

 

And if you'll excuse me now, I need to go complain to my cat about how horrible the minecraft code is written.

[Reika]

Excuse the fact that I am somewhat late but I needed to find the time to give the minecraft code a look.

In any case I wrote these few lines and they work splendidly, the only thing that they don't do is increase the chance for rare drops, which, unsurprisingly, are safely put away behind dumbly written methods and variables.

 

If you do want that, the best way I see is a wrapper class around EntityLivingBase, something which I believe you stated you didn't want so I avoided it.

The other possibility is creating a hashmap with the entities as keys and everytime an entity is encountered it doesn't know yet, spawn about a thousand of them, capture the drops, check which drop are below a certain amount and add those drops in a List<EntityItem> to the value in the hashmap.

A bit convoluted to say the least, although I don't think the mobs would show themselves so the temporary lag should be negligent.

 

Other than that, is just about regulates everything you could want.

 

public final class KillMobs {

public static void KillPresentMobs(World parWorld, EntityPlayer parPlayer, int parItemDropAmount, AxisAlignedBB parAxisAlignedBB, int parArmourDropChance)
{
	List<EntityItem> OldentityItems = parWorld.getEntitiesWithinAABBExcludingEntity(parPlayer, parAxisAlignedBB);

	Random rnd = new Random(); //Loops through current entities and selects the mobs.
	for(EntityLivingBase Ent: Iterables.filter(parWorld.getEntitiesWithinAABBExcludingEntity(parPlayer, parAxisAlignedBB), EntityLivingBase.class))
	{
		if(rnd.nextInt(100) <= parArmourDropChance)
		{	//drop armor if the conditions are right(random is below chance and selected slot has armor)
			Ent.entityDropItem(Ent.getCurrentItemOrArmor(rnd.nextInt(4)), 0);
		}
		//kills entities and captures their drops(it does not prevent them from falling)
		Ent.captureDrops = true;
		Ent.attackEntityFrom(DamageSource.causePlayerDamage(parPlayer), 100f);
		List<EntityItem> ItemDropList = Ent.capturedDrops;

		for(EntityItem DropItem: ItemDropList)
		{ //adds extras for the amount specified by parItemDropChance for each item dropped
			Ent.entityDropItem(new ItemStack(DropItem.getEntityItem().itemID, parItemDropAmount, DropItem.getEntityItem().getItemDamage()), 0);
		}
		ItemDropList.clear();
	}

	//Checks if mobdrops are on, if not it will delete the mobdrops.
	if(parItemDropAmount  == -1)
	{
		for(EntityItem OldItem: Iterables.filter(OldentityItems, EntityItem.class))
		{
			for(EntityItem EntItem: Iterables.filter(parWorld.getEntitiesWithinAABBExcludingEntity(parPlayer, parAxisAlignedBB), EntityItem.class))
			{
				if(EntItem == OldItem)
				{
					EntItem.setDead();
}}}}}}

 

And if you'll excuse me now, I need to go complain to my cat about how horrible the minecraft code is written.

 

While I greatly appreciate your efforts, this is a solution that, even if functional, will be nightmarish to debug and is likely slower than the reflection.