[1.12.1] @ObjectHolder versus IForgeRegistry.getValue()

[jabelar]

Is there any technical reason that the object holder injection is better than using the getValue() method of the registry itself and storing that in a public field? Are they equivalent? Or is there a performance or logical reason that object holder is preferred method? 

 

I'm asking because I'm porting some older mods and in most I set the registry name within the block or item constructor so if I use injection now I have to go through all my blocks and items and dig up the registry names and copy those into the object holder annotations. Seems easier for me to simply to use the getValue() and retrieve the registry name instead rather than hard-coding it into annotations. Just being lazy, but it made me think about how the injection works and mostly I just want to understand whether it is equivalent -- I plan to mostly use object holder in the future.

[SuprizePlayz]

Both options aren't bad, but, as you said before, it is faster to code with getValue() method than annotations like @GameRegistry.ObjectHolder.

I would prefer the getValue() way.

Edited September 12, 2017 by SuprizePlayz

[Draco18s]

Lookup performance wise, ObjectHolder is probably faster. After that, it's going to be the same.

 

Personally I'd recommend ObjectHolder.

[LexManos]

s/probably/DEFINITLY/

Static field access vs a map lookup everytime you're accessing the value? That's a DEFINITE performance increase.

Beyond that, for your own stuff it should be simple as hell to use @ObjectHolder

@ObjectHolder("my_mod")
	public class MyBlocks {
	  public static final name1 = null;
	  public static final name2 = null;
	  public static final name3 = null;
	  public static final name4 = null;
	}

[Draco18s]

37 minutes ago, LexManos said:

s/probably/DEFINITLY/

Static field access vs a map lookup everytime you're accessing the value?

No one said "every time" Lex:

3 hours ago, jabelar said:

using the getValue() method of the registry itself and storing that in a public field?

 

[LexManos]

Eah missed that part. But point still stands as most people who use getValue don't cache it. And you have issues with making sure you rebuild that cache when things change. There is a event for it. But it's more work then just using @ObjectHolder which is what they brought up.

[Draco18s]

6 minutes ago, LexManos said:

But it's more work then just using @ObjectHolder which is what they brought up.

Sure. Which is why I recommended it.

[jabelar]

Thanks guys. As discussed I understood that lookup would be somewhat of a performance hit, although frankly the vanilla code uses lookups a lot. But yeah I meant store it for access as well.

 

Lex, regarding your statement about "your own stuff should be simple as hell", the problem with your example is the names of the fields. They either need to match the registry name (which is kinda weird because they use the underscore style naming) or you have to further add additional object holder annotations to match it all up. Furthermore, for my own stuff there is stuff I'm porting in which case knowing the registry name actually takes a bit of work -- I have to go into each of my classes and copy the text out and put it into the annotation.

 

Anyway, I just wanted to make sure they were functionally equivalent in case I was missing something fundamental about the object holder concept. Thanks!

[LexManos]

Yes the field names need to match up. But that shouldn't be an issue.

You issue seems to be that you don't want to lookup the registry names.

There isn't anything we can do to fix that.

Even get calls would need to have the name...

[jabelar]

The issue arises because of my existing codebase, where all the unlocalized/registry names were tucked within each class and so now collecting them all in the registration class is painful.

 

From an older version of my mod I had unlocalized name / registry name set in my constructors and so when I create a public static field I would simply call new MyCustomItem1() and so forth and to register them I would pull up the unlocalized name.

 

So in my registration (which previously was in a proxy) it would look like this:

 

 public static final customItem1 = new MyCustomItem1();

 public static final customItem2 = new MyCustomItem2();

...

 public static final customItem50 = new MyCustomItem50();

 

and I would register them simply by the old methods where I could do:

 

  GameRegistry.registerItem(customItem1, customItem1.getUnlocalizedName());
   GameRegistry.registerItem(customItem2, customItem2.getUnlocalizedName());

....
   GameRegistry.registerItem(customItem50, customItem50.getUnlocalizedName());
 

 

 So to convert to @ObjectHolder I have to go to dozens of item classes, dozens of block classes, sound instances sprinkled throughout my code and such and find the actual unlocalized name string and copy it over into annotations and it would be easier to continue to use a method similar to the above.

 

The problem is that maintaining mods across versions becomes a fulltime job if you've got even a few significant code bases. Not a big deal but it adds up.

 

I appreciate all the efforts to keep improving Forge, but it is a bit disheartening every time a new approach is used. Converting IEEPs to capabilities, achievements to advancements, recipes to JSON etc. Depending on what your mod focuses on it can actually be a significant rewrite.

 

So I'm just being lazy and whining. It is taking a few hours per mod, but I am converting over to Object Holder method...

Edited September 13, 2017 by jabelar

[Draco18s]

6 hours ago, jabelar said:

  GameRegistry.registerItem(customItem1, customItem1.getUnlocalizedName());

   GameRegistry.registerItem(customItem2, customItem2.getUnlocalizedName());

....
   GameRegistry.registerItem(customItem50, customItem50.getUnlocalizedName());

OH FOR FUCK'S SAKE

 

DO NOT USE getUnlocalizedName() HERE. THIS IS WHY WE HAVE REGISTRY NAMES AND getRegistryName()!

 

Not to mention that in 1.12, GameRegistry.register(...) is private (use the RegistryEvent.Register<Item> event) and GameRegistry.registerItem() has been removed as it was deprecated back in 1.10.2 in favor of GameRegistry.register(...)!

 

Also, I see no point in using ObjectHolder for your own items. None what so ever. You're already creating them, just stuff them into the static field yourself. ObjectHolder exists to get other mod's items without needing some kind of API or using getValue().

Edited September 13, 2017 by Draco18s

[LexManos]

You should also never statically construct your blocks/items.

So really you need to go through and cleanup your code.

Yes, it's work, but you should do it.

 

@Draco18sThere IS reason to use @ObjectHolder for your own items, that his current method doesn't support. OVERRIDES. If anyone comes along and says "Screw you I want to override the functionality of your item" then you just silently detect and use it. This way doesn't support that.

[Draco18s]

Ah, thanks for that @LexManos

[Cadiboo]

On 9/13/2017 at 5:44 AM, LexManos said:

And you have issues with making sure you rebuild that cache when things change. There is a event for it.

Sorry for the necro, whats the event? I can't find it anywhere. The fields I'm using for caching are instance fields so I can't use the @ObjectHolder annotation.

 

Edit: its the RegistryEvent.Register<> event

Edited October 14, 2018 by Cadiboo