search

squeek502 / AppleCore

An API for modifying the food and hunger mechanics of Minecraft
The Unlicense
55 stars 24 forks source link

The 1.13/1.14/1.15/etc Port #142

Open squeek502 opened 5 years ago

squeek502 commented 5 years ago

Figured I'd make an issue to coordinate/decide what all needs to happen for porting to the latest Forge version(s).

Things to decide:

Things to do:

cc @primetoxinz @GirafiStudios @Cazsius

squeek502 commented 5 years ago

Tried to get started with this, turns out probably the hardest part will be updating the build.gradle script for ASMHelper, as ForgeGradle and Gradle are much different now. Plus I completely forgot anything I knew back when I originally wrote that monstrosity, so it feels like I'm back to square one.

Maybe should look into a less hacky solution for including something like ASMHelper in a per-project manner.

EDIT: https://github.com/johnrengelman/shadow ?

primetoxinz commented 5 years ago

Frankly, you might not need ASMHelper. I'm not sure what the benefits of it were, but the new coremodding system in Forge is based in a sandboxed javascript loader. If you are in need of assistance in porting I can help.

primetoxinz commented 5 years ago

Also, a 1.13 port is basically worthless. Forge has already moved on and it was not really stable during 1.13

squeek502 commented 5 years ago

new coremodding system in Forge is based in a sandboxed javascript loader.

Whoops, didn't know that! Any docs for it? I was misled into thinking core mods hadn't changed by looking at a build.gradle of a 1.14 core mod and seeing FMLCorePlugin still in the manifest.

Looks like you're right that ASMHelper is worthless, or it would at least need a complete rewrite at this point.

If you are in need of assistance in porting I can help.

Pointing me in the right direction when I get stuck will help a lot. I haven't kept up with Forge changes at all and docs never seem to be easy to find.

primetoxinz commented 5 years ago

Unfortunately the docs on it are basically nonexistent. The main repo for it has a scattered example, but it doesn't really explain anything: https://github.com/MinecraftForge/CoreMods I managed to scrounge up the information from a bunch of mods that managed it somehow. The basic requirements are as follows

~~- META-INF/coremods.json with a map of modid -> javascript file This unfortunately means you can only have one .js file per~~

I used this as an example, it has a very simple coremod javascript file: https://github.com/TridentMC/FastBamboo Mine is now signficantly more advanced than that example and might be more useful: https://gitlab.com/BetterWithMods/1.13/betterwithmods/blob/dev/projects/Core/src/main/resources/javascript/betterwithmods_core.js

If this doesn't really fit your style, it might be possible to register your own system for loading coremods but I imagine no one surrounding forge would be willing to give advice in that effort.

squeek502 commented 5 years ago

Thanks for all the info @primetoxinz

Here's the branch I'm working in: https://github.com/squeek502/AppleCore/tree/1.14.3

squeek502 commented 4 years ago

Ok, I think I finally have a solid path forward for this. Rewriting the transformers from scratch using Javascript sounds like a complete nightmare, so I think it's worth porting ASMHelper to Javascript so that the transformer code can be ported closer to 1:1. I've started porting ASMHelper here, and have set it up so that it's possible to port the ASMHelper tests as well to make sure the functionality stays the same. Here's the toMethodDescriptor test file for example:

var ASMAPI = Java.type("net.minecraftforge.coremod.api.ASMAPI");

function initializeCoreMod() {
    ASMAPI.loadFile('../../main/javascript/asmhelper.js');
    ASMAPI.loadFile('jankytest.js');

    assertEquals("(Linternal/class/name;Lclass/descriptor;)Lclass/name;", ASMHelper.toMethodDescriptor("class.name", "internal/class/name", "Lclass/descriptor;"));
    assertEquals("(FZ)V", ASMHelper.toMethodDescriptor("V", "F", "Z"));
}

See https://github.com/squeek502/ASMHelper/commit/1d4a4e816c8c9cb197ba5ac43374e840d5260788 for some more context on how it's set up.

So the next step is getting ASMHelper ported fully to Javascript. After that, porting the AppleCore transformers to Javascript should be much easier.

squeek502 commented 4 years ago

Stalled due to https://github.com/MinecraftForge/CoreMods/issues/26

James103 commented 4 years ago

@squeek502 Are you also porting to 1.15.2?

squeek502 commented 4 years ago

Yeah, would probably just skip to 1.15 at this point. Still effectively blocked by https://github.com/MinecraftForge/CoreMods/issues/26 though.

James103 commented 4 years ago

Is it possible to port this to Fabric, or will it forever be Forge-only? A Fabric port may or may not work around the mentioned bug.

squeek502 commented 4 years ago

Fabric port would definitely not have the same problem.

I'm not very involved in Minecraft modding anymore, and at this point am basically just updating my existing mods to newer versions, nothing more. I'm all for creating a Fabric version, but someone else would have to take the reins on that.

Insane96 commented 4 years ago

Still effectively blocked by MinecraftForge/CoreMods#26 though.

That's sad, I was really looking forward to add lots of mechanics to Iguana Tweaks Reborn thanks to this

Parker8283 commented 4 years ago

Hello friends, long time no see. A long time ago in a galaxy far far away, I made MinecraftForge/MinecraftForge#1758, which was the PR to lift the AppleCore changes up to Forge. However, I borked the PR and said I would fix it "tomorrow". Well, I guess tomorrow meant 5 years later, but now we are here! I am once again attempting to lift the core parts of the AppleCore API into Forge so that everyone can enjoy it and we can avoid the whole ASM problem altogether!

Currently, I have this branch that is my working branch. As of today, I think I have lifted the core parts and updated them to work better in the newer MC versions. I would love the feedback of the people who have helped to work on this mod and probably understand it better than I do. Just to make sure I don't miss anything or make some catastrophic errors. My goal is to make the PR to Forge by the end of this week. If anyone could spare any time and leave some comments for me, I would greatly appreciate it. I'm also in the SlimeKnights Discord if you wanted to do some closer-to-real-time chatter, do so in #progs-other-mods. Or you can DM me :)

Either way, just wanted to leave this here. Don't mean to pull the rug out from underneath this project, but I think our goal back in the day when squeek, prog, and I were updating Hunger Overhaul for 1.7 was to upstream these changes. Just never got around to it. Now with the new JS system, seems like a good time to try again!

squeek502 commented 4 years ago

Looking good, thanks for doing this @Parker8283.

Some notes:

Parker8283 commented 4 years ago

Hey squeek, thanks for the feedback!

Just pushed my update to my branch. Mostly documentation, test mods, and small fixes. Hopefully a PR will be made soon! Thanks again for your help.

squeek502 commented 4 years ago

Sounds good.

From what I remember, it only affected the eating/drinking animations for some reason, but I could be wrong about that. Or, perhaps the bug affects all items but AppleCore intentionally only affects the eating/drinking animations. I'd have to look at the code again to understand what exactly the patches are doing. Will get back to you on that. I agree that it would be better as a separate PR, though.

squeek502 commented 4 years ago

Ok, details on the animation stuff:

So this could be a fix applied to items in general, and it seems like it almost certainly affects 1.16.x still. I personally would consider it a Forge bug.

Parker8283 commented 4 years ago

Awesome, thanks for this research. I will look into this and make a PR to Forge to fix that.

In other news, MinecraftForge/MinecraftForge#7266 has been made! This should be the bulk of what AppleCore was. Thanks for your help squeek! Now starts the review process...hopefully they don't tear too much into it :)

Parker8283 commented 2 years ago

Over one year later, just wanted to update here and say that I haven't forgotten about this. Just made MinecraftForge/MinecraftForge#8405, which is the 1.18 version of the original PR (it's been so long that a new minecraft version came out in the meantime!), and I will do my best to stay active on this PR on GitHub and in the Forge Discord (seems to be the way to do things now) so that it can finally be merged. I am still hoping for MinecraftForge/MinecraftForge#7266 to be pulled as well for 1.16, but since that is now the LTS version, it would be after the new 1.18 PR is pulled. Hopefully we can get that done soon!

squeek502 commented 2 years ago

Thanks for the update, hope it can get merged.

Shadows-of-Fire commented 2 years ago

I'm attempting to move the forge PR forward. It will still have to go through Lex's approval once it is deemed assignable, and that may be a hurdle on it's own.

I'm asking anyone who has an interest in the state of the system go review the PR for anything that could be done differently or better, as you guys in this thread are likely more knowledgeable about what you want out of the system than I am. Specifically the 1.18 variant https://github.com/MinecraftForge/MinecraftForge/pull/8405 - this has to get merged first before the 1.16 version can be considered.

Parker8283 commented 2 years ago

Hey folks, minor hiccup on the Forge PR. The Forge team has requested some core design changes, so I will have to look into that. See this and above comments on the PR to see what I mean. Since this will require some non-trivial changes, I'm hoping I can get the input of some people that use the current API as it stands on what the best path forwards is for Forge's requests. I just want to make sure that my eventual changes don't suddenly make some use case impossible or really cumbersome in the "new" API. If you have input or ideas or just want to link how you use this so I can better understand what all is needed from this system, please leave it here, I would greatly appreciate it. I'll try to find some time to go over everything within the next week or so and hopefully we can get this back on track relatively quickly.

squeek502 commented 2 years ago

In terms of why AppleCore exists, here's the primary use case it was created to support:

And secondarily:

Before AppleCore existed (in 1.6.4), Hunger Overhaul overrode FoodStats and unconditionally set the player's FoodStats to HO's custom implementation on player spawn--a technique that only works for one mod at a time. With AppleCore in 1.7.10, both mods were able to go through AppleCore's API instead, and were therefore compatible without needing any special casing.

Shadows-of-Fire commented 2 years ago

I have a fair bit of knowledge with the attribute system, I'll poke around and see what is appropriate to be an attribute. I think each exhaustion action will need to be an event still, but things like max exhaustion, hunger, sat, can be attributes just from memory.

Not everything can be widened to LivingEntity, but what can be will need to be. Since lex un-assigned it, it means there was some back-channel comms about that being necessary.

James103 commented 2 years ago

Don't forget to take into account that Scaling Feast by @yeelp does the following:

Parker8283 commented 2 years ago

Hey friends, haven't forgotten about this! I'm making pretty good progress on updating the Forge PR to meet the Forge Team's requests. One thing that was brought up on the PR was the tick period events, and questioning their necessity. You can read the comment here. It seems like these events have existed since the beginning of AppleCore, so I'm not sure who needed their use, and if just keeping the tick period the same and scaling the actual changes done would be acceptable. I'm on the fence personally on that, since I can see the value from having a mod that wanted to make more frequent changes over really big less-frequent changes, would probably give the player a bit more context. Downside, that's three more events that get fired pretty often, and those aren't cheap (although more realistically, they would be moved to entity attributes, which are definitely cheaper). Just looking for some perspective and/or context for the original versions so I can make sure I understand everything before making a decision.

On top of that, another request was to roll things like AllowExhaustion and Exhausted into one event if possible. There are a few places where we have an Allow* event, very closely followed by the actual event we are allowing. To me, I can see those potentially being rolled into one and not losing functionality, but I just want to double-check here before I do that, to make sure I'm not missing something.

I think those are the only two things left to resolve for me before I update the PR and hope it's good to go now. Hopefully we get this upstreamed soon! A note that because 1.19 seems to be not too far off, that will become the new main target probably, and then it can get backported to 1.18. It would then seem that 1.16 will be left behind. Such is the endless march of new Minecraft versions :)

Shadows-of-Fire commented 2 years ago

You could make the tick period an attribute that can be modified instead of an event, maintaining both performance and functionality.

The allow events can probably be safely rolled into the real event imo.

squeek502 commented 2 years ago

@Parker8283 For the tick period stuff, GetRegenTickPeriod is used in Hunger Overhaul. Unsure if it could be converted to something that doesn't allow for modification of the tick period. The others were likely just for the sake of completion, without any specific use-case in mind.

Rolling the Allow* stuff into the real event seems potentially okay, but hard to know exactly:

yeelp commented 2 years ago

Don't forget to take into account that Scaling Feast by @yeelp does the following:

  • Modifies the player's max hunger via a new item and a new attribute (modifier)
  • Adds an enchantment, potion effect, and attribute that changes the amount of exhaustion players receive
  • Adds a block that restores a percentage of the player's max hunger
  • Adds an enchantment that makes food more nourishing to the wearer
  • Adds a curse that changes the amount of exhaustion needed to lose a food point
  • ... and more (see the mod repo and its wiki)

I'm a little late to the party (read: very). A lot of Scaling Feast was me trying to use the AppleCore API in unique ways. I've gone ahead and documented which AppleCore events I use, and for what purposes. Hopefully that helps outlining use cases. Most of these event listeners are in features and handlers if you want to see the use cases.

FoodEvent: FoodStatsAddition: Allowing a hunger overflow system, and a dynamic starvation system.

HealthRegenEvent: AllowRegen: extending the health regen to a dynamic hunger bar size AllowSaturatedRegen: Same as above.

StarvationEvent: Starve: implementing a dynamic starvation system, and preventing starvation with a hunger Absorption effect.

ExhaustionEvent: ExhaustionAddition: implementing a dynamic starvation system, modifying exhaustion rate based on attribute value. Exhausted: For calculations involving a hunger version of the Absorption effect. GetExhaustionCap: This is a big one. Minecraft hard caps maximum exhaustion. Not sure why. This was a request by me to allow this cap to be variable. Check here for details. I can see it being reasonable to just remove the cap entirely instead of keeping the event, since it feels very arbitrary to have the cap in the first place.

HungerEvent: GetMaxHunger: Setting max hunger.

That's about everything I can find for AppleCore events Finding the individual AppleCoreAPI calls is a little more involved but is also used quite heavily. Pretty much everything mentioned except the HealthRegenEvents are needed for Scaling Feast.

Sinhika commented 1 year ago

Use case I desperately want available again: Hunger in Peace. I play on peaceful mode for reasons, but I really want food to matter.