String → Identifier

[ExE-Boss]

@SoniEx2 has pretty much abandoned his PR, so I’m taking over. For a detailed description of what this does, see #227.

To do list:

• [ ] Get rid of String serialization.
• [ ] Redesign this whole thing from scratch. (Seriously, the current implementation is very bloated)
  • [ ] Get rid of IdentifierFactory (or whatever I called it) and replace it with the Storable interface.

Completed:

• [x] Implement StringIdentifier, ClassIdentifier and UUIDIdentifier (SoniEx2)
• [x] Implement NamespacedStringIdentifier and EnumIdentifier
  • IDs can’t be null. That throws a NullPointerException.
• [x] Switch Core to use Identifier everywhere
  • Currently kept an implicit String → StringIdentifier method in FactoryManager Should we keep it or not?
• [x] Write tests
• [x] Fix wrappers
  • [x] Test in Minecraft 1.7 and 1.8

---

Closes #227

[codecov-io]

Current coverage is 14.26% (diff: 28.48%)

Merging #238 into master will decrease coverage by 0.01%

@@             master       #238   diff @@
==========================================
  Files           393        398     +5   
  Lines         10984      11076    +92   
  Methods           0          0          
  Messages          0          0          
  Branches       1580       1585     +5   
==========================================
+ Hits           1569       1580    +11   
- Misses         9329       9405    +76   
- Partials         86         91     +5   

Powered by Codecov. Last update fc4146f...16cf3a9

[SoniEx2]

null IDs?!

You may disagree but I believe this thing should NPE everywhere.

[SoniEx2]

You need to learn how to use a damn immutable object.

And a constructor for such an immutable object.

[Victorious3]

Ive yet to understand why you'd need these. Interned strings are enough of an identifier to me...

You know, something like this:

interface Identifier {
    public static String of(String str) { return String.intern(str); }
    public static String of(UUID uuid) { return String.intern(uuid.toString()); }
    public static String of(Class<?> clazz) { return String.intern(clazz.getName()); }
}

Or if you don't like this because of the pitfall that you have to call Identifier.of() to make sure that it is an interned String...

class Identifier {
    private String interned;
    private Identifier(String id) { this.interned = String.intern(id); }

    @Override
    public boolean equals(Object o) {
        // Look at how simple equality becomes
        if (o instanceof Identifier) 
            return ((Identifier)o).interned == this.interned;
        return false;
    }

    @Override
    public int hashCode() { return interned.hashCode(); }

    public static Identifier of(String str) { return new Identifier(str); }
    public static Identifier of(UUID uuid) { return new Identifier(uuid.toString()); }
    public static Identifier of(Class<?> clazz) { return new Identifier(clazz.getName()); }
}

Downside of that is that you can't compare them with == but that's otherwise "normal" in java anyways. Forget about all of that "serializing back to specific Identifier type" crud. Calling Class.forName or UUID.fromString respectively should be enough to expect of someone to do, if really needed. Not once have I seen such a case so far. What you do here is excessive over-engineering.

[RX14]

@Victorious3 wouldn't interned strings technically work with ==? I wouldn't recommend it because it might be a bit flakey but if they are interned shouldn't they be the "same" string.

[Victorious3]

@RX14 Yes they do see my 0815 implementation. Hence why I said interned strings are all you need for identifers. And

Identifier.of("foo") == Identifier.of(new String(new char[] {'f', 'o', 'o'})));

Not if you go with the simple wrapper class approach tho. Which is more safe because you wont accidentally forget to call Identifier.of somewhere

[SoniEx2]

@Victorious3 The issue is Identifier.of("net.minecraft.block.Block") == Identifier.of(net.minecraft.block.Block.class). That should NEVER be able to happen.

BUT OTHER THAN THAT, THEY SHOULD ACT LIKE THE String CLASS: THEY SHOULD BE IMMUTABLE. Imagine you have an Identifier in 20 different places, including hashmap keys and stuff, then you accidentally call .load() on it. For one it'd break the hashmap contract. And it's just plain insane!

@ExE-Boss Could you at least TALK TO ME ABOUT IT before doing something completely messed up?!

[RX14]

@SoniEx2 How about you calm the fuck down and suggest improvements nicely instead of shouting at people. It won't make them take your input more seriously.

[SoniEx2]

@RX14 I'll happily shout at someone who's touching code I wrote without any idea of what the code should do.

[RX14]

@SoniEx2 Well, if you keep doing it you won't be commenting on this issue tracker any more. You talk nicely or don't talk at all.

[SoniEx2]

@RX14 Fine.

Part of the reason I made the PR was so I could put different kinds of identifiers in the same HashMap without having them conflict. Something I couldn't do with strings.

A NamespacedStringIdentifier should contain an inner static class called NamespacedStringData, and that class should be used as the identifier type. The equals and hashCode methods should be final, since otherwise equals wouldn't be symmetric. This is also why we can't/shouldn't have null IDs.

[SoniEx2]

Also isn't NOVA supposed to work in pre-1.7.10 versions of minecraft, aka blockID versions of minecraft?

[RX14]

Mapping different types of identifiers to strings in a non-conflicting way should be possible: just add a prefix.

[SoniEx2]

@RX14 That's one way to make it unmaintainable. How would you handle Identifier<Integer> as a string?

"i:1"?

"i:\u0000\u0001"?

"i:\u0001\u0000"?

"i:\u0100\u0000"?

etc?

[Victorious3]

@SoniEx2 Identifier.of("net.minecraft.block.Block") == Identifier.of(net.minecraft.block.Block.class) Sure. This is fine tho. Where's the problem with that?

Different types of identifies should not exist. Interned strings are enough to cover the purpose of "identifying" things. We aren't using integer based ids anywhere.

[SoniEx2]

@Victorious3 One's a class, the other is a string. The Identifier class is a wrapper class to allow type-safe usage of identifier types in objects. (We could just use Object instead, which would've avoided this whole argument, but that's not type-safe.)

In other words, just like "net.minecraft.block.Block" != net.minecraft.block.Block.class, Identifier.of("net.minecraft.block.Block") shouldn't be equal to Identifier.of(net.minecraft.block.Block.class).

[Victorious3]

@SoniEx2 Typesafe identifiers? Please? Are you even listening to yourself? The purpose of an identifier is to identify any given object in a specific context. If its about identifying Blocks you'll identify blocks and nothing else. It doesn't matter if an item and a block, or an entity share the same id because they are stored in different places and you are using different methods to access them. An identifier doesn't have to be unique. It CAN be unique, but thats by all means no requirement.

[RX14]

I can imagine identifiers being unique aiding debuggability. This implementation is a bit overblown though.

Also @Victorious3 if I tell @SoniEx2 off for tone I have to tell you to keep it technical too.

[SoniEx2]

Old versions of Minecraft use ints instead of strings. To support both in a highly compatible way, you use Identifier class, with StringIdentifier/NamespacedStringIdentifier and IntegerIdentifier.

[Victorious3]

@RX14 Well then use UUIDs everywhere and call your blocks "d6356985-e8bd-45ab-b16c-8a589af95cc9" instead of stone, gravel or whatever. This PR is overcomlicating a non issue.

[SoniEx2]

Fine. Replace all instances of "Identifier" with "Object". That'll solve all your issues.

[Victorious3]

@SoniEx2 You mean String. Like it was before.

[SoniEx2]

@Victorious3 I mean Object, for its higher flexibility.

[Victorious3]

No, because you can't serialize "Object" so its useless as an identifier.

[SoniEx2]

And string is useless for older Minecraft versions.

[Victorious3]

@SoniEx2 This is Nova CORE it doesn't CARE about "Minecraft Versions"

[SoniEx2]

And the Minecraft wrappers are directly integrated into NOVA Core (kinda why they're in the same repo instead of being in their own repos).

[Victorious3]

@SoniEx2 If you manage to name one problem that this overbloated PR supposedly solves, then go for it. You are shooting with cannons at ants. I'm for simplicity and ease of use and things like NamespacedStringIdentifier certainly don`t fall into that category.

[SoniEx2]

Please make Mojang remove Minecraft's ResourceLocation. It's bloated.

[Victorious3]

@SoniEx2 Besides this being completely off point, ResourceLocation is NOT an identifier. It serves the purpose of being able to locate files within the jar or resource packs which use completely different file paths.

[SoniEx2]

@Victorious3 It is actually NO different from a NamespacedStringIdentifier. It identifies block names, item names, etc.

[Victorious3]

@SoniEx2 You are impossible to talk to. Closed until further notice from @ExE-Boss

[RX14]

@Victorious3 While I share your opinion, that doesn't mean I won't remove you from the project or issue tracker if you continue to be rude.

[SoniEx2]

You just need to talk to some java pros about it. Maybe even @calclavia (altho I'm not sure how "pro" they are).

[ExE-Boss]

On the NOVA discord server, I’ve discussed this with RX14 and we decided the following:

I would continue working on my version of the Identifier API and once all the versions of the Identifier API are fully implemented, we will compare them and decide which implementation is the best and end up using that.

[SoniEx2]

I need something I can use with HashMaps and stuff. I cannot use this implementation, and strings are unsuitable due to their conflicting nature.

[Victorious3]

@SoniEx2 You dont throw two different types of Objects into the same map. Just dont do it. Then you are fine.

[SoniEx2]

I can throw String and Class and UUID all into the same Map just fine. They don't conflict with eachother.

I want it to also work for Identifiers.

It has its uses.

(Also, part of the reason I made the PR was so mods could extend Identifier and create their own Identifiers for their own purposes. Like using Identifier for a programming language/compiler. That is where being able to mix Identifiers together in a Map is a really useful feature to have. I agree that @ExE-Boss 's version is bloated. It's also unsuitable for my use-case. Unlike my version.)

[Victorious3]

I can throw String and Class and UUID all into the same Map just fine. They don't conflict with eachother.

And then its a Map<Object, Object> hell no.

Like using Identifier for a programming language/compiler.

Mhm. Sounds like something I'd do every day.

[loordgek]

@Victorious3 ResourceLocation IS used as a identifier https://github.com/MinecraftForge/MinecraftForge/blob/1.11.x/src/main/java/net/minecraftforge/event/AttachCapabilitiesEvent.java#L74 https://github.com/MinecraftForge/MinecraftForge/blob/1.11.x/src/main/java/net/minecraftforge/fml/common/registry/IForgeRegistryEntry.java#L50

[Victorious3]

@loordgek And so might any other object that implements hashcode and equals. Yet you wouldn't call them identifiers.

[SoniEx2]

I miss @calclavia, the only professional/sane java dev we had on this POS.

[loordgek]

@SoniEx2 can you stop crying ranting plz

[calclavia]

@ExE-Boss What's the NOVA discord server? I'd like to join just to see what you guys are up to.

[ExE-Boss]

@calclavia The server is https://discord.gg/Sh63S4H

[SoniEx2]

@loordgek Never. If you don't like it, why do you keep doing things that cause it?

[RX14]

@SoniEx2 goodbye