Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to 1.12 #316

Merged
merged 6 commits into from Sep 10, 2017
Merged

Update to 1.12 #316

merged 6 commits into from Sep 10, 2017

Conversation

SquidDev
Copy link
Contributor

@SquidDev SquidDev commented Jun 12, 2017

Parrots
Crafting

The big change this Minecraft update is that the recipe system got a bit of a rewrite. Most of the existing recipe code can be reused, but there are a couple of quirks:

  • The main recipes are now defined in JSON (in assets/computercraft/recipes).
  • Every recipe now requires a unique ID. This is a pain for the turtle and pocket upgrade imposter recipes, though can be worked around.
  • Recipes are "unlocked" (so shown in the recipe book) via advancements (previously achievements). I've implemented advancements for the basic recipes, but I don't think it is as easy for turtle/pocket upgrades - something to investigate in the future perhaps.
  • The forge registry system got a rewrite, so blocks/items are now registered via events. I've written some thoughts on this in a couple of comments further down.

If people want to have a play, you can download it here.


The build is failing as Minecraft now depends on Java 8 (woot!). The Travis file will need to be changed to include:

jdk:
  - oraclejdk8

@KnightMiner
Copy link

Already? You are quick.

I think with how long it will likely take for Forge to stablize, it would definitely still be worth getting 1.11 relase out first, and having separate branches for each of these versions is still useful.

@SquidDev
Copy link
Contributor Author

SquidDev commented Jun 12, 2017

@KnightMiner It isn't a very complex port - there are a couple of mapping and signature changes, and recipes are a little different. I'm sure there will be other changes required as Forge updates and removes old code.

I agree that we should get a stable 1.11 version out too (and a 1.9.4/1.10.2 version for that matter). All in all, I'd like to see a separate branch for 1.10.*, 1.11.* and 1.12.* - they are sufficiently similar that it it shouldn't be too hard to juggle all three.

@Wojbie
Copy link
Contributor

Wojbie commented Jun 12, 2017

It does raise a point. Should there be a complicated list of advancements for CC or just unlock it all from one of them and don't bother with it?

@SquidDev
Copy link
Contributor Author

SquidDev commented Jun 12, 2017

@Wojbie I'd prefer some form of hierarchy - I feel the advancement system should guide the player through the mod. For instance, something like:

  • Gets Redstone
    • Unlocks Computer, Advanced Computer
      • Unlocks peripherals
      • Unlocks pocket computers (see below)
      • Unlocks turtle for the given family.
        • Unlocks turtle upgrade recipes for the given family. I'm not sure how feasible this is with the current system.
  • Gets golden apple
    • Unlocks pocket computer
      • Unlocks pocket computer upgrades. As above, I'm not sure how feasible this is with the current system.

You could also have some "actual" advancements - such as one for using the printer, or dying a turtle. I'd like to imagine that this could encourage people to use more of the more than turtles. I'm probably being a little optimistic though :).

@vico93
Copy link

vico93 commented Jun 12, 2017

Impressive quick update @SquidDev , thank you so much!

@vico93
Copy link

vico93 commented Jun 13, 2017

About the integration between Minecraft's new Recipe System and Forge, have a look at this @SquidDev https://gist.github.com/LexManos/2a11d4f7aa9d680d861dae4faf9dcfa6

@SquidDev SquidDev force-pushed the feature/minecraft-1.12.2 branch 3 times, most recently from c2d841f to a20ce91 Compare June 17, 2017 20:52
@SquidDev
Copy link
Contributor Author

So the Forge team are currently rewriting the registry system to be more event oriented. Instead of registering items & blocks in PreInit, they are now done in individual registry events. Furthermore, there will be potential for registry reloading in the future (so only loading certain mods, reloading after config changes, etc...).

I feel this is a good time to rewrite the current pocket and turtle upgrade registries to make use of these changes. There are a couple of improvements this offers:

  • As pocket and turtle upgrades depend on the item registry, we can ensure we load upgrades after items. This is especially important if registry reloading is ever implemented.
  • IMO using the event cycle is more elegant than the current system, as it allows you to use @Optional.Method and @SubscribeEvent rather than doing Loader.isModLoaded.
  • Provides more power - people can query the registry, listen to missing mappings events, etc... This could also be seen as a disadvantage as people can technically overwrite the registry, though of course we can always guard against that.

I'd quite like to convert the bundled redstone, media, peripheral and turtle permission provides to use a similar event based system, but I'd be interested in people's opinion on that.

I hope this all makes sense (and isn't an absurd idea). I'd really like to hear what other's think (especially @dan200 obviously).


Just to provide a code example of how I'm envisioning this working:

interface ITurtleUpgrade extends IForgeRegistryEntry<ITurtleUpgrade>
{ /* Same as before minus getUpgradeID() */ }

class TurtleTool implements ITurtleUpgrade extends IForgeRegistryEntry.Impl<ITurtleUpgrade>
{
    public TurtleTool( ResourceLocation id, int legacyID, String adjective, Item item )
    {
        setRegistryName(id);
        // Same as before
    }
    // Same as before minus getUpgradeID()
}

class CCTurtleProxyCommon
{
    @SubscribeEvent
    public static void registerUpgrades(RegistryEvent.Register<ITurtleUpgrade> event)
    {
        event.getRegistry().register(new TurtleTool());
    }
}

@SquidDev SquidDev force-pushed the feature/minecraft-1.12.2 branch 2 times, most recently from 846edd5 to c65f2b4 Compare June 24, 2017 08:58
@SquidDev
Copy link
Contributor Author

SquidDev commented Jun 24, 2017

I've just updated the PR to add support for Forge's new registry system. There are a couple of issues here, so any suggestions are welcome:

  • GameRegistry.registerTileEntityWithAlternatives is gone. This means people will have to update to 1.10.2/1.11.2 before updating to 1.12, otherwise tile entities will vanish.

  • It isn't entirely clear where you're meant to register IItemColor any more. I'm still doing it in the init stage, but it is possible you should do it in event handlers.

  • Turtle and pocket upgrades should probably be registered after items but before recipes. It currently isn't an issue as upgrades either reference vanilla items (which don't change) or lazily load the item. This will probably end up being an issue down the line (or for another mod).

    The simple (but not the nicest) solution would just be to register upgrades as item registration finishes (so a very low priority). This is rather ugly, but would work. If item reloading is ever implemented, we'd have to find a way to clear the upgrade registry first.

    An alternative solution would be, as mentioned above, to create a custom registry and use that. However, this has some of it's own problems:

    • It's rather over-kill: ForgeRegistry does integer id <-> ResourceLocation handling, callbacks, etc... Sadly there isn't a way to register your own registry class either, so you're pretty much stuck with it.
    • Little control over registry event order. Currently registries are fired in alphabetical order which works for us (computercraft is before minecraft:recipes), but really isn't ideal.

@vico93
Copy link

vico93 commented Jun 28, 2017

I've got this error trying to run the latest CC build for 1.12 in https://cc.crzd.me/. I'm using only Forge b2385 and Iron Chests:

https://gist.github.com/vpontin/4c188edc4dacfe6684a11de50c8d61e7

@KnightMiner
Copy link

  • I think requiring to update to 1.11 first is acceptable as long as a 1.10 or 1.11 build is put out soon. Forge's officially policy is that they only support the last update for world transfer anyways, and I seem to recall dan mentioning that he does not expect you to be able to make large version jumps anyways.
  • Pretty sure nothing changed with IItemColor, that was always a vanilla method so init is fine since preInit does not work.
  • I know Lex currently advises registering TileEntities in the block event, so upgrades in the item event should be clean enough. I think the best solution to it firing multiple times would be to simply replace the old one with the new one if registered twice. If a string/resourcelocation to upgrade map is used, this has the additional benefit of allow addons to override the default upgrades if they wish (such as replacing the diamond tool upgrades with another tool material while keeping function) making it consistent with the Forge registries.

@SquidDev SquidDev force-pushed the feature/minecraft-1.12.2 branch 2 times, most recently from 83375fb to 9762229 Compare June 29, 2017 09:21
@SquidDev SquidDev changed the title [WIP] Update to 1.12 Update to 1.12 Jun 29, 2017
@chanbakjsd
Copy link

SquidDev, you can check this Travis blog post here: https://blog.travis-ci.com/2013-11-26-test-your-java-libraries-on-java-8/ . Add the jdk into .travis.yml and Java 8 build should work.

@SquidDev
Copy link
Contributor Author

@lutherwenxu: I'm aware that you can change travis's Java version, it's just I'm pretty sure it needs to be changed on the master branch (and so needs to be done by Dan).

@asweigart
Copy link

I am +1 on SquidDev's idea for separate branches for 1.10 to 1.12.

On advancements: ComputerCraft has always been more of a programming education tool than game playing (hence why the recipes are so cheap), so I don't think requiring advancements to unlock recipes is a good idea. People use OpenComputers if they want a gameplay challenge.

@KnightMiner
Copy link

Advancements are not so much a requirement to unlock recipes, the recipes always work whether they are visible in the book or not. The point is to help discover recipes, so I personally thing they are a great idea.

Basically, obtaining redstone would notify the user of computers, crafting that would notify them of pocket and turtles. Crafting it and obtaining gold would notify them of the relevant advanced computers, etc.

 - Convert most recipes to JSON
 - Add JSON factories for impostor and turtle recipes.
 - Several mappings changes
 - Migrate to Forge's new registry system
Default methods, everywhere.
Arrow types, switch on strings.
Lambdas!
Here comes Java 8.
This doesn't provide the ability to unlock the upgrade impostor recipes,
but I'm not sure that is currently feasible.
@redfast00
Copy link

Hi, I don't know if this is the right place to post this, but I'll do it anyway: multicolor print seems to be broken in the latest build (printing works, but when printing over an existing page in another color, the old text gets wiped.) (I think) it used to be able to print in multiple colors by cycling an already printed page trough the printer again with a different dye. I don't know if this bug is because of the 1.12 port or due to ComputerCraft in general since I only play on 1.12. Thanks a lot!

@dan200 dan200 merged commit 61ff91f into dan200:master Sep 10, 2017
@SquidDev SquidDev deleted the feature/minecraft-1.12.2 branch September 10, 2017 19:56
SquidDev added a commit to SquidDev-CC/ComputerCraft that referenced this pull request Sep 16, 2017
…aft-1.12.2"

This reverts commit 61ff91f, reversing
changes made to bde9ac9.
SquidDev added a commit to SquidDev-CC/ComputerCraft that referenced this pull request Sep 16, 2017
…aft-1.12.2"

This reverts commit 61ff91f, reversing
changes made to bde9ac9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants