Fix CatalogTypes by ImMorpheus · Pull Request #1792 · SpongePowered/Sponge

ImMorpheus · 2018-02-10T17:41:43Z

SpongeAPI | SpongeCommon

Missing translations:

EntityTypes: (Missing entries in the lang file)
entity.MinecartSpawner.name
entity.MinecartTNT.name
entity.ThrownEnderpearl.name
entity.EyeOfEnderSignal.name
entity.FireworksRocketEntity.name
entity.MinecartCommandBlock.name
entity.MinecartFurnace.name
entity.LlamaSpit.name
entity.ThrownExpBottle.name
entity.ItemFrame.name
entity.SpectralArrow.name
entity.EvocationFangs.name
entity.ShulkerBullet.name
entity.MinecartRideable.name
entity.AreaEffectCloud.name
entity.LeashKnot.name
entity.WitherSkull.name
entity.EnderCrystal.name

ImMorpheus · 2018-02-10T17:45:46Z

~wip

gabizou · 2018-07-01T21:21:08Z

Does this fix the spam from the catalog type tests?

ImMorpheus · 2018-07-01T21:22:05Z

All inconsistencies with mojang's translations should be fixed.

There are still a few things I don't like, I'll see if I can come up with something better. Suggestions are welcomed.

EDIT:

Does this fix the spam from the catalog type tests?

Yes.

After this PR:

See edit

This also adds a test to make sure name and id are not the same and another one to find inconsistencies with mojang's translation

ImMorpheus · 2018-07-01T21:36:57Z

Also, the old test had a few problems. If the translation was equal to the id, then it'd be classified as a missing translation. However minecraft is using the id as a fallback for a missing translation, thus that's not something we should fix.

Here's the situation before this PR with the new test:

See edit

ST-DDT · 2018-07-01T21:48:34Z


+    @Override
+    public Translation getTranslation() {
+        return translation;


Add this here.

ST-DDT · 2018-07-01T21:50:43Z

+
+    @Override
+    public Translation getTranslation() {
+        EnumDyeColor enumdyecolor = ItemBanner.getBaseColor(new ItemStack((Item) (IMixinItem) this));


Cache the translation?

ST-DDT · 2018-07-01T21:51:01Z

+
+    @Override
+    public Translation getTranslation() {
+        return new SpongeTranslation(PotionUtils.getPotionFromItem(new ItemStack((Item) (IMixinItem) this)).getNamePrefixed("lingering_potion.effect."));


Cache the translation?

ST-DDT · 2018-07-01T21:51:39Z

+
+    @Override
+    public Translation getTranslation() {
+        return new SpongeTranslation(PotionUtils.getPotionFromItem(new ItemStack((Item) (IMixinItem) this)).getNamePrefixed("potion.effect."));


ST-DDT · 2018-07-01T22:05:56Z

+    public SpongeItemStatistic(String statId, String itemName, ITextComponent statName, Item item) {
+        super(statId, itemName, statName, item);
+        String args = new ItemStack(item).getDisplayName();
+        translation = new SpongeTranslation(statId.substring(0, statId.length() - 1), args);


ST-DDT · 2018-07-01T22:09:55Z

+public abstract class MixinItemTippedArrow extends MixinItem implements ItemType {
+
+    @Override
+    public Translation getTranslation() {


Cache this.

ST-DDT · 2018-07-01T22:10:09Z


    public Translation itemstack$getTranslation() {
-        return new SpongeTranslation(shadow$getItem().getUnlocalizedName((net.minecraft.item.ItemStack) (Object) this) + ".name");
+        String id;


Cache this.

ST-DDT · 2018-07-01T22:10:14Z

+public abstract class MixinItemSplashPotion extends MixinItem implements ItemType {
+
+    @Override
+    public Translation getTranslation() {


Cache this.

ST-DDT · 2018-07-01T22:10:19Z

+public abstract class MixinItemSkull extends MixinItem implements ItemType {
+
+    @Override
+    public Translation getTranslation() {


Cache this.

ST-DDT · 2018-07-01T22:12:15Z

+
+    @Override
+    public Translation getTranslation() {
+        return translation;


ST-DDT · 2018-07-01T22:18:37Z

Nice improvements.

ImMorpheus · 2018-07-02T14:17:56Z

    [Test worker/INFO]: FML platform manager could not load class cpw.mods.fml.relauncher.CoreModManager. Proceeding without FML support.
    [Test worker/ERROR]: MixinBootstrap.doInit() called during a tweak constructor!
    [Test worker/INFO]: Compatibility level set to JAVA_8
    [Test worker/INFO]: Loading tweak class name org.spongepowered.asm.mixin.EnvironmentStateTweaker
    [Test worker/INFO]: Calling tweak class org.spongepowered.asm.mixin.EnvironmentStateTweaker
    [Test worker/ERROR]: Classloader restrictions [PACKAGE_CLASSLOADER_EXCLUSION] encountered loading mixins.common.test.json:MixinBlock, name: org.spongepowered.common.launch.mixin.MixinBlock
    [Test worker/ERROR]: Classloader restrictions [PACKAGE_CLASSLOADER_EXCLUSION] encountered loading mixins.common.test.json:MixinEnumDifficulty, name: org.spongepowered.common.launch.mixin.MixinEnumDifficulty
    [Test worker/ERROR]: Classloader restrictions [PACKAGE_CLASSLOADER_EXCLUSION] encountered loading mixins.common.test.json:MixinGameType, name: org.spongepowered.common.launch.mixin.MixinGameType
    [Test worker/ERROR]: Classloader restrictions [PACKAGE_CLASSLOADER_EXCLUSION] encountered loading mixins.common.test.json:MixinItem, name: org.spongepowered.common.launch.mixin.MixinItem
    [Test worker/ERROR]: Classloader restrictions [PACKAGE_CLASSLOADER_EXCLUSION] encountered loading mixins.common.test.json:MixinSpongeItemStackSnapshot, name: org.spongepowered.common.launch.mixin.MixinSpongeItemStackSnapshot
    [Test worker/ERROR]: Classloader restrictions [PACKAGE_CLASSLOADER_EXCLUSION] encountered loading mixins.common.test.json:MixinStatBase, name: org.spongepowered.common.launch.mixin.MixinStatBase
    [Test worker/INFO]: Launching wrapped minecraft {org.spongepowered.common.test.TestMain}

I don't know what's going on here.

Everything else works as expected.

ImMorpheus · 2018-07-02T14:19:40Z

~qa

gabizou

Some changes, clarifications.

gabizou · 2018-07-02T14:46:07Z

-                LOG.warn("Catalog Type: {} Id : {} has broken Method: {} ({}): {}", this.name, this.catalogId, this.methodName,
-                        this.implementationClass, t);
-                return;
+            testResult(checkNotNull(this.method.invoke(this.catalogType), "return value"));


How is this changing how it used to work previously?

Previously, everything inside ignoredFailureswould be ignored (it would just log "x has broken method: etc.") but it was a workaround (see the TODO above). I've just removed the if and unwrapped the exception.

gabizou · 2018-07-02T14:58:04Z

+
+            if (object instanceof IStringSerializable || object instanceof Fish || object instanceof CookedFish) {
+                if (verbose) {
+                    LOG.info("This is probably a blockstate. Mojang doesn't directly provide translations for blockstates. Ignoring {}", translation.getId());


Why is fish considered untranslatable but you're saying it's probably a block state?

AFAICT there are translations for fish!?

item.fish.cod.raw.name=Raw Fish item.fish.salmon.raw.name=Raw Salmon item.fish.pufferfish.raw.name=Pufferfish item.fish.clownfish.raw.name=Clownfish item.fish.cod.cooked.name=Cooked Fish item.fish.salmon.cooked.name=Cooked Salmon

Indeed. However that's net.minecraft.item.ItemFishFood.

This check should avoid net.minecraft.item.ItemFishFood.FishType

ST-DDT · 2018-07-02T17:05:09Z

    @Shadow private String unlocalizedName;

    @Shadow public abstract int getItemStackLimit();
    @Shadow public abstract String getUnlocalizedName();


AFAICT this is no longer used.

ST-DDT · 2018-07-02T17:10:11Z

+            ItemStack is = ItemStackUtil.fromNative(new net.minecraft.item.ItemStack((Item) (IMixinItem) this));
+            Optional<GameProfile> owner = is.get(Keys.REPRESENTED_PLAYER);
+
+            if (owner.isPresent()) {


This cannot be present, since you just created the item or am I missing something?

ST-DDT · 2018-07-02T17:15:40Z

    @Override
    public Translation getTranslation() {
-        return new SpongeTranslation(this.statId);
+        return this.translation;


Instead of statId.equals("stat.mineBlock.") you could use StatBase#statId.startsWith("stat.mineBlock.").

and instead of item you could use StatCrafting#Item

ST-DDT · 2018-07-02T17:17:43Z

+ */
+package org.spongepowered.common.interfaces;
+
+public interface MojangTranslatable {


Need javadocs that explain the purpose of this interface.

ST-DDT · 2018-07-02T17:26:20Z

+
+            if (object instanceof IStringSerializable || object instanceof Fish || object instanceof CookedFish) {
+                if (verbose) {
+                    LOG.info("This is probably a blockstate. Mojang doesn't directly provide translations for blockstates. Ignoring {}", translation.getId());


AFAICT there are translations for fish!?

item.fish.cod.raw.name=Raw Fish item.fish.salmon.raw.name=Raw Salmon item.fish.pufferfish.raw.name=Pufferfish item.fish.clownfish.raw.name=Clownfish item.fish.cod.cooked.name=Cooked Fish item.fish.salmon.cooked.name=Cooked Salmon

gabizou · 2018-07-15T07:32:17Z

Sounds like it's almost ready, so with these changes the warnings are pretty much gone from all the catalog type tests? (I know I may have introduced one or two printouts with the recent PhaseTracker changes, but that I'll get to once I figure out how to beat a testing environment check without making those checks fail in production).

ImMorpheus · 2018-07-15T16:21:23Z

so with these changes the warnings are pretty much gone from all the catalog type tests?

Correct, but as you can see from the travis log https://travis-ci.org/SpongePowered/SpongeCommon/jobs/403996687, it's still complaining about NotePitch having the same id and name. I think it's safe to ignore them (unless someone comes up with different names).

Cybermaxke · 2018-07-17T16:09:04Z

Ids shouldn't be uppercase, I noticed C_SHARP1 for one of the NotePitches in the log. With the CatalogKey PR, all catalog types should also get a plugin id in the future, so maybe this is the time?
minecraft:id

ImMorpheus · 2018-07-17T17:21:58Z

That's probably a breaking change. Is it something I can do in this PR ? Input wanted

gabizou · 2018-07-17T17:34:33Z

That's probably a breaking change. Is it something I can do in this PR ? Input wanted

I'd say it's a fix, but best to check with some plugin developers for input. @clienthax for your note block music plugin, do the catalog id's changing from previously unprefixed to correctly prefixed note pitches change anything for you? (first example I could think of at this moment).

ST-DDT · 2018-07-17T17:43:00Z

There is a solution to it though. The associated registry could still return the right entries for old ids, so after the entries get loaded and saved once, you have completed the note pitch migration. Or does the config loader only use the ids for the mapping. IIRC they use GameRegistry#get(type, id).

ImMorpheus · 2018-07-17T18:40:33Z

The thing is, NotePitch is not the only one having unprefixed ids.
Prefix everything and things like #1286 would arise. I could fix NotePitch and leave everything else to the catalogue key PR. Probably adding other tests to ensure we wont miss anything

ImMorpheus · 2018-07-18T17:20:37Z

org.spongepowered.common.registry.CatalogTypeMethodTest > testCatalogMethodImpl[87755 Catalog Type: Fish | Id : minecraft:raw.cod | Method: Fish#getCookedFish() | (net.minecraft.item.ItemFishFood$FishType)] STANDARD_OUT
    [Test worker/WARN]: id is not lowercase. Catalog Type: Fish | Id : cooked.COD | (net.minecraft.item.ItemFishFood$FishType)

org.spongepowered.common.registry.CatalogTypeMethodTest > testCatalogMethodImpl[87763 Catalog Type: Fish | Id : minecraft:raw.salmon | Method: Fish#getCookedFish() | (net.minecraft.item.ItemFishFood$FishType)] STANDARD_OUT
    [Test worker/WARN]: id is not lowercase. Catalog Type: Fish | Id : cooked.SALMON | (net.minecraft.item.ItemFishFood$FishType)

Something is not right.

EDIT: Ok, something is horribly wrong. Looks like the catalogtest was not testing every catalog type.
Thanks to @SimonFlash for making me realise that with his FireworkShape PR.

ImMorpheus · 2018-08-22T20:01:14Z

Updated to reflect the CatalogKey PR.

ImMorpheus requested review from Deamon5550, gabizou, kashike and stephan-gh as code owners February 10, 2018 17:41

ImMorpheus mentioned this pull request Feb 10, 2018

Fix CatalogTypes SpongePowered/SpongeAPI#1777

Closed

limbo-app added the status: wip Work in progress label Feb 10, 2018

ImMorpheus force-pushed the fix/catalogtypes branch from fb84287 to fb1cab5 Compare June 27, 2018 13:15

ST-DDT suggested changes Jul 1, 2018

View reviewed changes

limbo-app added status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc and removed status: wip Work in progress labels Jul 2, 2018

gabizou requested changes Jul 2, 2018

View reviewed changes

ST-DDT suggested changes Jul 2, 2018

View reviewed changes

ImMorpheus force-pushed the fix/catalogtypes branch from 5ab9b63 to 4187f41 Compare July 14, 2018 19:29

ImMorpheus mentioned this pull request Jul 17, 2018

Add item groups (aka creative tabs) #2000

Closed

ImMorpheus force-pushed the fix/catalogtypes branch 4 times, most recently from 87beb6d to bbe521d Compare August 22, 2018 19:59

limbo-app added status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc and removed status: wip Work in progress labels Aug 22, 2018

ImMorpheus added the type: bug Something isn't working label Aug 29, 2018

ImMorpheus force-pushed the fix/catalogtypes branch 3 times, most recently from 165b6f4 to 2d2b3eb Compare September 19, 2018 16:50

ImMorpheus added the branch: bleeding label Sep 19, 2018

ImMorpheus mentioned this pull request Jan 26, 2019

Consistent catalog builders. #2174

Merged

ImMorpheus changed the base branch from bleeding to 1.13 February 2, 2019 16:01

ImMorpheus force-pushed the fix/catalogtypes branch 4 times, most recently from 2ba45aa to 3828654 Compare February 2, 2019 16:56

ImMorpheus requested a review from Cybermaxke February 2, 2019 17:09

ImMorpheus added version: 1.16 (u) API: 8 and removed branch: bleeding labels Feb 2, 2019

ImMorpheus added 7 commits February 2, 2019 18:16

Add support for formatted translations

47eaafe

Remove SpongeCatalogType.Translatable

21009a3

id -> key and api changes

4c97929

Rework catalog test

a6530fd

Fix warnings

4369edf

Cleanup + id -> key

0d303c3

Fix license

3828654

Zidane closed this Dec 2, 2019

Uh oh!

Conversation

ImMorpheus commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ImMorpheus commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabizou commented Jul 1, 2018

Uh oh!

ImMorpheus commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ImMorpheus commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ST-DDT Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ST-DDT commented Jul 1, 2018

Uh oh!

ImMorpheus commented Jul 2, 2018

Uh oh!

ImMorpheus commented Jul 2, 2018

Uh oh!

gabizou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabizou commented Jul 15, 2018

Uh oh!

ImMorpheus commented Jul 15, 2018

Uh oh!

ImMorpheus commented Feb 10, 2018 •

edited

Loading

ImMorpheus commented Feb 10, 2018 •

edited

Loading

ImMorpheus commented Jul 1, 2018 •

edited

Loading

ImMorpheus commented Jul 1, 2018 •

edited

Loading

ST-DDT Jul 1, 2018 •

edited

Loading

Cybermaxke commented Jul 17, 2018 •

edited

Loading

ST-DDT commented Jul 17, 2018 •

edited

Loading

ImMorpheus commented Jul 18, 2018 •

edited

Loading

ImMorpheus commented Aug 22, 2018 •

edited

Loading