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

1 Like

Same here. One of my PRs is awaiting Review since august 13th :frowning:
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 :frowning:

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:

  • you should give many informations, like (at least some of these depending on the context):

    • precise description of the issue (look at what they use internally, for instance #UE-44637),

      • best start would be to link to a previously made bug report on AnswerHub (or existing issue on the public Epic Game tracker)

      • steps to reproduce

      • perhaps even a minimum test case project

    • an explanation on what was fix/how it works

    • what non-reg tests where done/are there any risks involved

  • the fact is after a few one on the same subsystem you become known as somehow reliable (sort of) so this become quicker/easier

1 Like

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

Considering the number of active pull requests just keeps rising, most of them are never reviewed or accepted. It’s sad.

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 :slight_smile:

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).

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)

My somewhat related blog post may be of interest: http://headcrash.industries/stories/why-your-idea-might-not-make-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.

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 :slight_smile:

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.

Thanks for answering this clearly once again Max.
Keep up the good work!

Gerke moved on from Epic a few months ago, he’s not there anymore.