Jump to content
  • Home
  • Files
  • Docs
Topics
  • All Content

  • This Topic
  • This Forum

  • Advanced Search
  • Existing user? Sign In  

    Sign In



    • Not recommended on shared computers


    • Forgot your password?

  • Sign Up
  • All Activity
  • Home
  • Non-Forge
  • Site News (non-forge)
  • Helping out with Forge PRs
The update for 1.13 is being worked on - please be patient. (Updated 02/19/19)
Sign in to follow this  
Followers 0
mezz

Helping out with Forge PRs

Started by mezz, November 25, 2016

2 posts in this topic

mezz    3

mezz

mezz    3

  • Tree Puncher
  • mezz
  • Moderators
  • 3
  • 9 posts
  • Report post
Posted November 25, 2016

Hi, I'm mezz.

 

I'm the author of JEI, and I have maintained Forestry the last couple years.

 

I have also made several contributions to Forge using Pull Requests including:


  •  
  • Automatically wrapping tooltips that would go off the screen.
     
  • Added an event to cancel the potion gui shift
     
  • Added support for key modifiers (Ctrl, Alt, Shift) to key bindings
     
  • Added the Fluid Capability (co-authored with RWTema)
     
  • Made dispensers with buckets work with modded fluids
     
  • Improved the "missing mods" error screen
     
  • Fixed mipmapping when  modded textures are the wrong dimensions
     
  • Fixed the very laggy mipmap slider
     

...and a bunch of other stuff.

 

 

I will be helping to clean up and streamline Forge Pull Requests.

 

The primary goal is to get more PRs up to standard and keep them from sitting for a long time in a bad state with no response or update. I'll be working with other modders to review them in a more comprehensive way, and then pass them to Lex for final review.

 

That means that PR issues will have to get through me first. That doesn't mean it has to be a more difficult process though. In working towards the primary goal, I want to help everyone write effective PRs that are likely to be accepted.

 

Below is an explanation of what it takes to write a good Forge Pull Request.

 

 

What is a Pull Request (PR) to Forge?

 

Mods are built on top of Forge, but there are some things that Forge doesn't support, and that limits what mods can do. 

When modders run into something like that, they can make a change to Forge to support it, and submit that change as a Pull Request on Github.

 

However to make a good Forge Pull Request, you have to understand a bit about what the Forge project is, its goals, and its limitations.

 

What Exactly is Forge?

 

At a high level, Forge is a mod compatibility layer on top of Minecraft. 

Early mods edited Minecraft's code directly (like coremods do now), but ran into conflicts with each other when they edited the same things. They also ran into issues when one mod changed behavior in ways that the other mods could not anticipate (like coremods do now), causing mysterious issues and lots of headaches. 

 

By using something like Forge, mods can centralize common changes and avoid conflicts. 

Forge also includes supporting structures for common mod features like Capabilities, Registries, Fluid handling, the Ore Dictionary, and others that allow mods to work together better.

 

When writing a good Forge Pull Request, you also have to know what Forge is at a lower level. 

There are two main types of code in Forge: Minecraft patches, and Forge code.

 

Patches

 

Patches are applied as direct changes to Minecraft's source code, and aim to be as minimal as possible. 

Every time Minecraft code changes, all the Forge patches need to be looked over carefully and applied correctly to the new code. 

This means that large patches that change lots of things are difficult to maintain, so Forge aims to avoid those and keep patches as small as possible. 

In addition to making sure the code makes sense, reviews for patches will focus on minimizing the size.

 

There are many strategies to make small patches, and reviews will often point out better methods to do things. 

Forge patches often insert a single line that fires an event or a code hook, which affects the code after it if the event meets some condition. 

This allows most of the code to exist outside of the patch, which keeps the patch small and simple.

 

For more detailed information about creating patches, see the wiki.

 

Forge Code

 

Aside from the patches, Forge code is just normal java code. It can be event code, compatibility features, or anything else that's not directly editing Minecraft code.

When Minecraft updates, Forge code has to update just like everything else. However, it's much easier because it is not directly entangled in the Minecraft code.

 

Because this code stands on its own, there is no size restriction like there is with the patches.

 

In addition to making sure the code makes sense, reviews for this code will focus on making the code clean, with proper formatting and java documentation.

 

Explain Yourself

 

All Pull Requests need to answer the question: why is this necessary? 

Any code added to Forge needs to be maintained, and more code means more potential for bugs, so solid justification is needed for adding code.

 

A common Pull Request issue is offering no explanation, or giving cryptic examples for how the Pull Request might theoretically be used.

This only delays the Pull Request process. 

A clear explanation for the general case is good, but also give a concrete example of how your mod needs this Pull Request.

 

Sometimes there is better way to do what you wanted, or a way to do it without a Pull Request at all. Code changes can not be accepted until those possibilities have been completely ruled out.

 

Show that it Works

 

The code you submit to Forge should work perfectly, and it's up to you to convince the reviewers that it does. 

 

One of the best ways to do that is to add an example mod or junit test to Forge that makes use of your new code and shows it working. 

 

To set up and run a Forge Environment with the example mods,

.

 

Breaking Changes in Forge

 

Forge can't make changes that break the mods that depend on it. 

This means that Pull Requests have to ensure that they do not break binary compatibility with previous Forge versions. 

A change that breaks binary compatibility is called a Breaking Change.

 

There are some exceptions to this, Forge accepts Breaking Changes at the beginning of new Minecraft versions, where Minecraft itself already causes Breaking Changes for modders. 

Sometimes an emergency breaking change is required outside of that time window, but it is rare and can cause dependency headaches for everyone in the modded Minecraft community.

 

Outside of those exceptional times Pull Requests with breaking changes are not accepted, they must be adapted to support the old behavior or wait for the next Minecraft version.

 

Be Patient, Civil, and Empathetic

 

When submitting Pull Requests you will often have to survive code review and make several changes before it is the best Pull Request possible. 

Keep in mind that code review is not judgement against you. Bugs in your code are not personal. Nobody is perfect, and that's why we're working together.

 

Negativity will not help, threatening to give up on your Pull Request and write a coremod instead will just make people upset and make the modded ecosystem worse. 

It's important that while working together you assume the best intentions of the people who are reviewing your Pull Request and not take things personally. 

 

Review

 

If you do your best to understand the slow and perfectionistic nature of the Pull Request process, we will do our best to understand your point of view as well.

 

After your Pull Request has been reviewed and cleaned up to the best of everyone's ability, it will be marked for a final review by Lex, who has the final say on what is included in the project or not.

 

Thanks for contributing, and I'll see you in code review!

  • Like 1

Share this post


Link to post
Share on other sites

LexManos    1470

LexManos

LexManos    1470

  • Reality Controller
  • LexManos
  • Forge Code God
  • 1470
  • 8369 posts
  • Report post
Posted November 25, 2016

Everyone welcome a new Forge team member. He is in charge of all Pull Requests and Issues on the Forge GitHub now.

Him and I will be talking through everything that is needed to be done in a PR.

I will no longer be processing things until he tags me in them.

So if you have concerns about your PR speak to him.

This is an attempt to make the process a smoother and "friendlier" process. Lets see how it goes!

 

So ya, say hello!

  • Like 1

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  
Followers 0
Go To Topic Listing Site News (non-forge)

  • Recently Browsing

    No registered users viewing this page.

  • Posts

    • Floydman
      Solved: [1.13.2] Duplicate key error

      By Floydman · Posted 17 minutes ago

      I just found the fix: For whatever reason, in runClient.launch and runServer.launch the mapEntry MOD_CLASSES includes the main class multiple times. Removing all but the first one resolves the issue.
    • oridos
      Having trouble launching the client test through eclipse

      By oridos · Posted 18 minutes ago

      The abusive staff are silencing my posts for no reason, so if you see this and have the solution, just message me.   I get this error when I try to run my mod through eclipse after doing all the initial setup
    • fieldbox
      Unresolved reference: minecraftforge

      By fieldbox · Posted 44 minutes ago

      I'm using Forgelin and created an object, annotated it with Mod, which didn't work. I then tried to import net.minecraftforge.fml.common.Mod, but that throws an 'unresolved reference' error. I'm in IDEA and, if it helps, Forge and Minecraft are not in my External Libraries. Sorry if Forgelin isn't directly supported, I didn't know where else to ask. Oh yeah, and genIntellijRuns throws a NullPointerException! 
    • DaemonUmbra
      cant make modded server

      By DaemonUmbra · Posted 56 minutes ago

      Many sites pretend to be "the forge website", please give a URL so I can be sure.
    • LordTSI
      cant make modded server

      By LordTSI · Posted 1 hour ago

      java version "1.8.0_201"
      Java(TM) SE Runtime Environment (build 1.8.0_201-b09)
      Java HotSpot(TM) 64-Bit Server VM (build 25.201-b09, mixed mode) i got it from the forge website
  • Topics

    • Floydman
      1
      Solved: [1.13.2] Duplicate key error

      By Floydman
      Started Thursday at 03:45 AM

    • oridos
      7
      Having trouble launching the client test through eclipse

      By oridos
      Started 7 hours ago

    • fieldbox
      0
      Unresolved reference: minecraftforge

      By fieldbox
      Started 44 minutes ago

    • LordTSI
      3
      cant make modded server

      By LordTSI
      Started 6 hours ago

    • MrMarnic
      7
      No Values in ForgeRegistries.ENTITIES 1.13.2

      By MrMarnic
      Started Wednesday at 09:21 PM

  • Who's Online (See full list)

    • DaemonUmbra
    • ImTitoYoghurt
    • fieldbox
    • DavidM
    • thedarkcolour
    • Choonster
    • Floydman
    • oridos
  • All Activity
  • Home
  • Non-Forge
  • Site News (non-forge)
  • Helping out with Forge PRs
  • Theme
  • Contact Us

Copyright © 2017 ForgeDevelopment LLC Powered by Invision Community