Announcement

Collapse
No announcement yet.

How to get pull requests reviewed and accepted?

Collapse
X
  • Filter
  • Time
  • Show
Clear All
new posts

    How to get pull requests reviewed and accepted?

    Hi,

    I work for a medium sized developer, with 30+ coders - and we've made some good fixes to the engine. Ideally we would like to push these changes back to Epic so that other people can benefit from them, and also will prevent merge conflicts for us at a later date.

    I have submitted 2 pull requests for very simple fixes (as a starting point), but both are just sitting in "Waiting for Review" state - one of them since July.

    Is there any way to raise the profile on pull requests? Or perhaps I'm missing a step?

    https://github.com/EpicGames/UnrealEngine/pull/3799
    https://github.com/EpicGames/UnrealEngine/pull/4066

    Thanks,

    James

    #2
    Same here. One of my PRs is awaiting Review since august 13th
    AFAIK there's no way to speed up the review process. Epic simply picks the most interesting ones or critical bug fixes.
    One thing I noticed is that the review process slowed down massively around may 2017. Before that, it took 1-2 months to get a review. The reason is unknown, at least to me

    Comment


      #3
      Hi @James Chilvers, it seems that you've had one of them pass the review three days ago, congrats!

      I've had a ton of merge requests accepted, so here is my opinion on the way to do it:
      1. you should give many informations, like (at least some of these depending on the context):
        1. precise description of the issue (look at what they use internally, for instance #UE-44637),
          1. best start would be to link to a previously made bug report on AnswerHub (or existing issue on the public Epic Game tracker)
          2. steps to reproduce
          3. perhaps even a minimum test case project
        2. an explanation on what was fix/how it works
        3. what non-reg tests where done/are there any risks involved
      2. the fact is after a few one on the same subsystem you become known as somehow reliable (sort of) so this become quicker/easier
      UE4 Git LFS 2.x Source Control Plugin v2.5 for UE4.18 - (v1 integrated by default in Beta status since UE4.7)
      UE4 Plastic SCM Source Control Plugin (1.2.1 for UE4.19)
      PayPal me a beer to support my work

      Comment


        #4
        Originally posted by TriNityGER View Post
        Same here. One of my PRs is awaiting Review since august 13th
        AFAIK there's no way to speed up the review process. Epic simply picks the most interesting ones or critical bug fixes.
        One thing I noticed is that the review process slowed down massively around may 2017. Before that, it took 1-2 months to get a review. The reason is unknown, at least to me
        Probably Because Andrew Pascal Left https://twitter.com/UnrealAlexander everything seems to have been going downhill since he left, also the live streams aren't the same anymore without him

        Comment


          #5
          Considering the number of active pull requests just keeps rising, most of them are never reviewed or accepted. It's sad.
          Last edited by Zeblote; 11-15-2017, 10:33 AM.

          Comment


            #6
            Originally posted by James Chilvers View Post
            Hi,
            I work for a medium sized developer, with 30+ coders - and we've made some good fixes to the engine. Ideally we would like to push these changes back to Epic so that other people can benefit from them, and also will prevent merge conflicts for us at a later date.
            It's best if you just submit all of your changes where you think it makes sense. It's not just Epic looking at the PRs, but also other programmers that look through the PRs to find interesting stuff that makes sense to merge because it would improve performance or stability. If I find something interesting, I merge it into my branch of course. So even if Epic doesn't immediately accept the PRs, you are still helping other people a lot with them. Nothing is better than having an issue and finding a PR that just fixes it

            And yes, Epic is a bit (very) slow with PRs recently. They just don't have enough people looking over them. At some point they will probably dedicate more people to that again (I hope).

            Easy to use UMG Mini Map on the UE4 Marketplace.
            Forum thread: https://forums.unrealengine.com/show...-Plug-and-Play

            Comment


              #7
              Yes I am pretty sure they will put more priority into it again at some point, like they did in the past.

              Meanwhile, I've look at a few open PR, and let's be honest, we could/should largely improve the minimum quality (some titles just say "Fix a small bug" or the like)
              UE4 Git LFS 2.x Source Control Plugin v2.5 for UE4.18 - (v1 integrated by default in Beta status since UE4.7)
              UE4 Plastic SCM Source Control Plugin (1.2.1 for UE4.19)
              PayPal me a beer to support my work

              Comment


                #8
                My somewhat related blog post may be of interest: http://headcrash.industries/stories/...e-it-into-ue4/
                Gerke Max Preussner | Sr. Engine Programmer | Epic Games

                Follow me on Blog | Delicious | Github | Goodreads | Linkedin | Pinterest | SlideShare | Twitter.
                Chat with me as 'gmpreussner' in #unrealengine on FreeNode IRC, or on UnrealSlackers.

                Comment


                  #9
                  Originally posted by gmpreussner View Post
                  My somewhat related blog post may be of interest: http://headcrash.industries/stories/...e-it-into-ue4/
                  That's all fine, some PRs not being accepted is normal. What we're concerned about is PRs being ignored.
                  If you open the repo, there are currently 500 PRs (!!!) just sitting around in limbo: https://github.com/EpicGames/UnrealEngine/pulls

                  Some of them are years old. Noone knows why they weren't accepted, or whether they still work.
                  Some of them are bug fixes. Do those bugs even still exist?

                  Until recently someone was at least tagging all the PRs with "waiting for review" to give some illusion of activity, but even that has stopped with no explanation.
                  It would be really useful if you decide not to use a PR, to close it and maybe explain why it's a bad idea.
                  Last edited by Zeblote; 11-21-2017, 02:54 PM.

                  Comment


                    #10
                    Originally posted by gmpreussner View Post
                    My somewhat related blog post may be of interest: http://headcrash.industries/stories/...e-it-into-ue4/
                    Nice blog gmpreussner...
                    But that piece is from 2015 - You should write more often!


                    Question:

                    Ultimately who says yes / no to a PR.... Is it down to each specialist team?
                    Or is there a formal review process / centralized decision making system?


                    Observation:

                    Indies, like Enterprise customers want to see more Engine sign-posts and traffic lights...
                    Overall, the new Roadmap system and PR-backlog makes it harder for devs to plan etc.

                    Comment


                      #11
                      Originally posted by franktech View Post
                      Nice blog [..] You should write more often!
                      Thanks, I'm trying to get into a more regular blogging schedule again. I have eight articles in the pipeline right now, but had no time this year to finish any of them. Good thing the holidays are coming up


                      Ultimately who says yes / no to a PR.... Is it down to each specialist team?
                      Or is there a formal review process / centralized decision making system?
                      It comes down to the system owner, i.e. the person(s) who wrote the code for that system, or who last touched it or is most familiar with it. Some PRs are quick fixes and can be merged in a second. Others may look quick, but can have wider effects on other parts of the Engine that are not obvious. Then, sometimes, our developers have specific plans to refactor or rewrite the affected system, and they may not be sure how that fix fits in, if at all. Most of the time, however, the devs are too busy with other high-priority tasks. It basically comes down to deciding what we work on first. It may take only 10 minutes to merge a PR, but if you have a thousand ten minute tasks in the queue, even that can be too much and become deprioritized.

                      For example, I currently have a couple blockers and several critical work items in my queue. Unless the PR is considered a blocker or critical as well, I probably won't work on it, even if it is a major bug. The only reason I can respond to this thread right now is because I'm waiting for the compiler to finish. Most of the PRs are minor, some are major, and some are new features. Features usually have the lowest priority. The priority of work items depends on a number of factors, i.e. whether it is blocking one of our licensees, whether it is a substantial improvement for the majority of UE4 users, whether it affects our games, etc.

                      PR# 3799, for example, is probably super low priority since few developers use batched lines. It's a one line change, but someone will have to analyze the problem and decide whether that fix is, in fact, a good fix. I can't merge this fix myself, because I don't know much about batched line rendering. So it will likely have to be someone on the Rendering Team, and those guys are going on all cylinders working on improvements that are critical to the vast majority of UE4 users.

                      I hope this makes sense.

                      Gerke Max Preussner | Sr. Engine Programmer | Epic Games

                      Follow me on Blog | Delicious | Github | Goodreads | Linkedin | Pinterest | SlideShare | Twitter.
                      Chat with me as 'gmpreussner' in #unrealengine on FreeNode IRC, or on UnrealSlackers.

                      Comment


                        #12
                        Originally posted by gmpreussner View Post
                        I hope this makes sense.
                        Definitely... Thanks for sharing that insight Max! When you're on the outside looking in, as most Indies are...
                        The process can seem mysterious / contradictory or even frustrating etc. So that explanation helped a lot!

                        Comment


                          #13
                          Thanks for answering this clearly once again Max.
                          Keep up the good work!
                          UE4 Git LFS 2.x Source Control Plugin v2.5 for UE4.18 - (v1 integrated by default in Beta status since UE4.7)
                          UE4 Plastic SCM Source Control Plugin (1.2.1 for UE4.19)
                          PayPal me a beer to support my work

                          Comment

                          Working...
                          X