MinecraftForge / FML

(Archive Only: Merged into Forge proper) The Forge Mod Loader - an opensource replacement mod loader for minecraft
Other
432 stars 201 forks source link

Impossible to override vanilla item / block #370

Open Clashsoft opened 10 years ago

Clashsoft commented 10 years ago

With the current FMLControlledNamespacedRegistry, it is impossible to override a vanilla item or block since it automatically adds the mod id to the mapping name. The swap method is not accessible because it has a default modifier, and calling it via Reflection crashes because the transactionalAvailabilityMap is null.

jeffreykog commented 10 years ago

You can pass the modid "minecraft" in as additional argument in GameRegistry.registerBlock. Read the javadoc!

Clashsoft commented 10 years ago

With this

GameRegistry.registerItem(brewingStand, "brewing_stand", "minecraft");

the output says "Adding duplicate key 'BrewingAPI:brewing_stand' to registry". The brewing stand doesnt change at all (I override almost everything, TileEntity, Container, GUI, placer item).

Clashsoft commented 10 years ago

Also, calling FMLControlledNamespacedRegistry.add without passing the modId from GameRegistry.registerItem(item, name, modId) still uses the current mod id.

ArcanoxDragon commented 10 years ago

I think you'll have to remove the old entry; not sure about the new system though.

Clashsoft commented 10 years ago

There's no way to remove an entry from the ID map, at least no "hacky" way.

cpw commented 10 years ago

Using "minecraft" should work.

ArcanoxDragon commented 10 years ago

Reflection?

cpw commented 10 years ago

OK. This isn't going to work at the minute. I thought item replacement would work, but it doesn't. I need special code because you want to preserve/detect your custom replacements in save games - the system doesn't support that behaviour at present.

I'll see what I can do over the next couple of days. Don't try and reflective hack it - you'll just make a nicely broken save for anyone using the hackery.

cpw commented 10 years ago

Just out of interest, what are you trying to do? Replace a brewing stand with an alternative block implementation? or what?

Clashsoft commented 10 years ago

I consider Reflection as a "hacky" way.

brewingStand2Item = new ItemBrewingStand2(); // extends ItemReed; adds a line to the tooltip for testing overrideItem(brewingStand2Item, "brewing_stand");

public static void overrideItem(Item item, String name) { if (!name.startsWith("minecraft:")) { name = "minecraft:" + name; } GameRegistry.registerItem(item, name, "minecraft"); }

Since it doesnt add a line to the tooltip and I am 100% the code for that is correct, I can assume that registerItem doesnt work.

And yes, alternate implementation to store potion data completely in NBT tags.

EDIT: Just read your comment

cpw commented 10 years ago

I think the right solution here is aliasing. Allow registering an alias for one thing to another. I had toyed with it before but I wasnt sure I needed it. This proves it is probably needed. That way, your stand is your code, and its in your mod namespace, and you alias the minecraft one to your code.. It also helps with existing world problems..

Clashsoft commented 10 years ago

Well, the FMLControlledNamespacedRegistry extends RegistrySimple, which uses a Map to store it's data. And when adding a key-value pair to a map where the key already exists, it would simply override the value. So you could simply put(name, block) again on the <String,Block> map and put(block, id) on the int map

cpw commented 10 years ago

Yes, but that means the change becomes invisible to the idtracker side, which isn't what I want at all..

zachanima commented 10 years ago

Has there been any progress on this? I see the issue is still open, and the addAlias method in the GameRegistry class is still a stub.

I would like to replace vanilla blocks with replacement blocks in a recommended way, such that they still work for custom recipes from other mods dependent on the vanilla block.

The solution suggested here is not yet implemented, or at least doesn't seem to be.

The documentation recommends reflection (or failing that, Access Transformers); I would rather use a simpler approach such as aliasing, though.

ShetiPhian commented 10 years ago

This seems to be mostly working in forge-1.7.10-10.13.1.1210-new After correcting my dumb mistake (was replacing the block but not the item) the only issue I've currently having is with registerBlockIcons not being called.

cpw commented 10 years ago

Interesting.. I wonder why.

Clashsoft commented 10 years ago

Is it possible that you are registering it too late? As far as I know, you have to do it in the preInit state.

cpw commented 10 years ago

Well, the substitute shouldn't be normally registered, for identity reasons. It probably needs the register code to change slightly to scan the substitutions..

Clashsoft commented 10 years ago

Also, you can take a look at how I do it without the alias system in my Clashsoft Lib: https://github.com/Clashsoft/Clashsoft-Lib/blob/master/src/main/java/clashsoft/cslib/minecraft/block/CSBlocks.java#L135: https://github.com/Clashsoft/Clashsoft-Lib/blob/master/src/main/java/clashsoft/cslib/minecraft/item/CSItems.java#L325 I believe it does the replacement a bit more safely, even though it involves some really critical and hacky ways that I'm sure @cpw doesn't like at all :trollface:

ShetiPhian commented 10 years ago

addSubstitutionAlias is being used in preInit after I register my other blocks. For now, so I can test, I'm registering the icons with another block. I haven't ran into any other problems yet.

Clashsoft commented 10 years ago

As for the "normal" way to do it with aliases, I have two questions:

  1. If I register block Dirt2 as "dirt_2" and then alias "dirt" -> "dirt_2", wouldn't that occupy a new block ID when registering "dirt_2"?
  2. To successfully use aliases, one will have to add the alias before Blocks.class and Items.class init. Which state would that be?
cpw commented 10 years ago

@clashsoft your replacement crap is by definition less safe than the CORRECT way to do it. The registry will be locked down to this, once I have sufficient testing done, so BE WARNED..

Clashsoft commented 10 years ago

I know this is not the 100% correct way to do it, that's probably why these methods change in almost every update. But still, am I correct with my assumption in my second question? Because if it puts any block in Blocks.class into the field without any registered aliases, it uses the vanilla block until someone manually updates the field, right?

cpw commented 10 years ago

NO. not at all. Blocks and Items are both handled by FML automatically. If you register a substitution for anything that's in that class, your alternative will be present there, WHEN THE SERVER IS RUNNING.

Clashsoft commented 10 years ago

So, how do I actually register an alias now? FMLControlledNamespacedRegistry.addAlias(String, String) is package private, and GameRegistry.addAlias(String, String, GameRegistry.Type) is a no-op.

ShetiPhian commented 10 years ago

You need to use the "new" branch of forge http://files.minecraftforge.net/minecraftforge/new

Cybermaxke commented 9 years ago

I tried to override the default bow, but they dissappear just from the game. So I cannot give them anymore or see them in creative.

LexManos commented 9 years ago

Code Only.

Cybermaxke commented 9 years ago

Using this method:

GameRegistry.addSubstitutionAlias(String, Type, Item);

I have currently replaced the entries in the registry and fields in the Items class to make it working. But this method removes the items from the game. You can find my code here.

Clashsoft commented 9 years ago

So we can conclude that the Alias System works syntactically, but not semantically. Here is a list of things that should be improved.

  1. A substitution should not be forced to be a subtype of the value it substitutes. That just leads to a ton of duplicate code and general confusion.
  2. registerIcons is not called on substitutes. What happens if we don't have a 'normal' block or item that calls the method on our substitutes?
  3. Why exactly does GameRegistry.addSubstitutionAlias(String, Type, Item) has a type argument if you could simply add two methods, one for blocks and one for items?
  4. The amount of unnecessary calls in this method is too damn high. Variables are your friend. And the compiler likes them too.
if (getPersistentSubstitutions().containsKey(nameToReplace) || getPersistentSubstitutions().containsValue(toReplace))                                                         
{                                                                                                                                                                             
    FMLLog.severe("The substitution of %s has already occured. You cannot duplicate substitutions", nameToReplace);                                                           
    throw new ExistingSubstitutionException(nameToReplace, toReplace);                                                                                                        
}                                                                                                                                                                             
I replacement = superType.cast(toReplace);                                                                                                                                    
I original = getRaw(nameToReplace);                                                                                                                                           
if (original == null)                                                                                                                                                         
{                                                                                                                                                                             
    throw new NullPointerException("The replacement target is not present. This won't work");                                                                                 
}                                                                                                                                                                             
if (!original.getClass().isAssignableFrom(replacement.getClass()))                                                                                                            
{                                                                                                                                                                             
    FMLLog.severe("The substitute %s for %s (type %s) is type incompatible. This won't work", replacement.getClass().getName(), nameToReplace, original.getClass().getName());
    throw new IncompatibleSubstitutionException(nameToReplace, replacement, original);                                                                                        
}                                                                                                                                                                             
int existingId = getId(replacement);                                                                                                                                          
if (existingId != -1)                                                                                                                                                         
{                                                                                                                                                                             
    FMLLog.severe("The substitute %s for %s is registered into the game independently. This won't work", replacement.getClass().getName(), nameToReplace);                    
    throw new IllegalArgumentException("The object substitution is already registered. This won't work");                                                                     
}                                                                                                                                                                             
getPersistentSubstitutions().put(nameToReplace, replacement);                                                                                                                 
donington commented 9 years ago

I know this hasn't been looked at in a while, but I would like to add that as of 1.8 substituting fire is now impossible with the system as-is.

The issue is that in order to substitute a Block properly, the corresponding Item needs to be overridden as well. As of 1.8 minecraft:fire no longer has an associated Item to override, rendering the alias impossible to complete. Only substituting the fire block alone causes fire to not spawn properly.

Please see https://github.com/donington/finitefire/releases/tag/1.8-1.0 for the code I used to come to this conclusion.

hea3ven commented 9 years ago

Currently, addSubstitutionAlias doesn't seem to be completely working even for basic blocks. I'm using this code, and the block can't be placed in the world because the block state to id mapping still has the original block in it. From my debugging I believe that the problem is that GameData.injectSnapshot is not taking the substitution aliases into account, but I don't know enough of the registries to be sure.

usafphoenix commented 9 years ago

is FMLControlledNamespacedRegistry what's responsible for block and item ids now? I appear to be getting item ID collisions with several mods installed, and i'm not sure how to fix this in 1.7.10. in 1.6.4 i could just edit block and item ids in config files.

LexManos commented 9 years ago

ID Collisions should not happen, For basic debugging use the forums and post logs. Do not hijack github issues that have NOTHING to do with your issue.

usafphoenix commented 9 years ago

not trying to hijack anything, just scowering the internet to find a solution to a problem, and asking simple questions. But which is now resolved. Got some quality help from a stranger who was both knowledgable and gracious, and it's all fixed now. I'm going to go slaughter some endermen. peace.