-
Notifications
You must be signed in to change notification settings - Fork 24
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
The 1.13/1.14/1.15/etc Port #142
Comments
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. |
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. |
Also, a 1.13 port is basically worthless. Forge has already moved on and it was not really stable during 1.13 |
Whoops, didn't know that! Any docs for it? I was misled into thinking core mods hadn't changed by looking at a Looks like you're right that ASMHelper is worthless, or it would at least need a complete rewrite at this point.
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. |
Unfortunately the docs on it are basically nonexistent.
I used this as an example, it has a very simple coremod javascript file: https://github.com/TridentMC/FastBamboo 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. |
Thanks for all the info @primetoxinz Here's the branch I'm working in: https://github.com/squeek502/AppleCore/tree/1.14.3 |
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 squeek502/ASMHelper@1d4a4e8 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. |
Stalled due to MinecraftForge/CoreMods#26 |
@squeek502 Are you also porting to 1.15.2? |
Yeah, would probably just skip to 1.15 at this point. Still effectively blocked by MinecraftForge/CoreMods#26 though. |
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. |
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. |
That's sad, I was really looking forward to add lots of mechanics to Iguana Tweaks Reborn thanks to this |
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! |
Looking good, thanks for doing this @Parker8283. Some notes:
|
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. |
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. |
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. |
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 :) |
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! |
Thanks for the update, hope it can get merged. |
I'm attempting to move the forge PR forward. 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. |
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. |
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. |
I have a fair bit of knowledge with the attribute system, I'll poke around and see what is appropriate to be an attribute. 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. |
Don't forget to take into account that Scaling Feast by @yeelp does the following:
|
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 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 :) |
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. |
@Parker8283 For the tick period stuff, Rolling the
|
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.
That's about everything I can find for AppleCore events Finding the individual |
Use case I desperately want available again: Hunger in Peace. |
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:
IEdible
/IEdibleBlock
still necessary?ItemFoodProxy
still necessary?cc @primetoxinz @GirafiStudios @Cazsius
The text was updated successfully, but these errors were encountered: