@1D_Inc there is no need to turn this thread into a UI debate, further posts like this can be split off into their own thread.
6 Likes
Before this thread got derailed, Smilebags posted these insightful comments. Letās try to keep the discussion on track:
11 Likes
Lots could be written on this, some things that are on the top of my list:
- Have newer developers take part in patch review (as part of regular developer duties).
- Do pair-patch review sessions (an hour or so with 2x developers over video-chat, reviewing a list of patches - weāve done this from time to time, currently not on a regular basis though).
- Have a way to tag a patch as needing design review first, in a way the module team can handle, eg: collect a list of all-patches with pending design review for a certain module.
Currently patches often end up in an undefined state because theyāre making opinionated changes which need some input on a design level before we even look at the code.
Note, these are topics that have been on our radar for a while.
34 Likes
So, when are you guys going to deal with it?
1 Like
First you criticize the developers for not participating:
Then, you criticize them for not immediately picking up your very important topic, which is well known by them and they have been looking into for quite a while.
Do you really believe you accomplish anything with this sort of attitude?
5 Likes
The problem is not my attitude. Donāt kill the messenger. The problem of the mishandling of the contributors and their contributions has been ongoing for several years and it needs urgently to be addressed properly, including how devs like Campbell himself deals with user contributionsā¦
1 Like
Just to throw my 2 cents in, my experience has been quite positive. I submitted a series of patches for the ocean modifier with 2.83, and subsequently. The responses in the tracker have been friendly and patient. If things got quiet, I simply nudged the submission to ask whether there was anything more needed - gently prompting the reviewer to take a look with a clear call for contributions.
It was certainly less abrasive a process than I expected and the review process was always something I appreciated rather than resented.
Iād also suggest that the interest level of the contributor and community is likely reflected in the activity on the ticket - the squeaky wheel(s) will get the grease.
9 Likes
I think the key there is the prodding, some new devs are shy about doing it. Donāt get me wrong ideally it wouldnāt be needed but if things were ideal we wouldnāt be 70 posts into this threadā¦
8 Likes
In this case the messenger arrived with an already known message and now tries to hold people accountable!
If you actually cared about the development, you would have seen that they have been looking quite sincerely into those sorts of problems since more than a year and have already improved a lot.
Just recently they changed the way meetings are held. They are now split more into the actual modules and every module has now an owner again. This will likely empower the module owners more and give them a greater responsibility. There are realistic chances that this might also improve the situation regarding code review or contributions from volunteers in general.
I understand that people might start this kind of thread, even though I donāt think it is necessary or changes anything. (Especially not in a situation like this, where a lot of changes are actually taking place.) At best it might raise the awareness a little. However, demanding an explanation of anyone is beyond reasonable and highly disrespectful!
4 Likes
Donāt get me wrong, I think that open discussions are essential to the dev workflow working and everyone understanding where everyone is coming from but is this topic still constructive?
Anecdotal but Iāve had a decent experience here. I guess YMMV, like any social situation.
-
Firstly there are some initiatives in this area.
The past 3 weeks Iāve been doing code-review with Julian, Hans Goudey has been getting involved in patch review too, Nathan Craddock (GSOC student) is helping us out by reviewing D9180.
We have weeks where we only do bug-fixing (preparing for release), during that time volunteers are still submitting patches, it tends to build up.
When we have meetings and talk about priorities, getting developers involved in patch review is raised as something that needs attention.
-
Secondly, the amount of patches being submitted are more than any single developer can reasonably handle both in terms of quantity and knowledge of the code-base required to review.
So this is something each module team needs to manage. So itās not like a single developer can just ādeal with itā, we need to find a balance that works for us.
To address your question more directly: "Why not have a dev-curfew on open patches?"
It could work in some limited way. While Iām not totally against I donāt think itās a good long term solution.
-
Ideally we would iterate with developers, not have them ignored until some sporadic review sprintā¦ when theyāve probably lost interest in the patch and moved onto other things.
-
Iād guess someā¦ even most? developers wouldnāt be up for extended periods of code review. Even though some developers are employed, there is a limit to how much you can push people to do things they donāt like.
-
Finally patches are not an end unto themselves. itās questionable that prioritizing patches above all other work for expended periods is even giving the best value to Blender users. While itās a shame that some patches are ignored, many of the patches that remain open are things developers have briefly checked over and not been convinced are a priority or even worth adding. For formal response extra testing and investigation is often needed, which results in many of these patches ending up unreviewed (speaking for myself at least).
Said differently, obvious improvements to an area where the module members all agree are worth adding - usually make it in. There are of course exceptions and things that slip through the cracks.
Having patches ignored can put off newer developers, which may be the greater long term loss.
While much more could be written on this, I think it covers the main points.
40 Likes
One of the proposals here in this thread is letting contributors know within a month, if the feature of their new feature patch(if properly developed from that point etc.) is wanted or not.
As it currently is, this decision is often never made or BF devs are disagreeing, which means that the contributor is left hanging. Or if one dev takes an interest and helps the code up to the standards and getting the patch committed, then another dev reverts the commit - and the feature is dead and so is the motivation of the contributorā¦
A solution could be to give the module teams an authority to green-light features under the condition that only if the patch is developed properly from that point, it will get committed. And other BF Devs should respect that decision.
Or in other words if any BF Dev can veto anything at any time, the communication and collaboration with the contributors can become impossible to an extend where new contributors just give up on contributing.
3 Likes
I know Iāve brought this up in similar threads in the past- but the thing that keeps me from contributing to the codebase is the very real possibility (and- given the immense number of ancient diffs that have never been touched, likelihood) that I could spend a lot of time and effort developing something only to have it flushed down the toilet without a glance. As long as there appears to be some kind of āsecret clubā you need to gain access to in order to get your patch reviewed, I wonāt bother. Iām just one potential contributor, but if I feel this way there are probably many others with similar views.
5 Likes
There is no secret club! If you want to contribute, get in touch with them to find out whether what you have in mind is welcome. If it is, discuss with them what you need to do to make the review process as smooth as possible, maybe discuss things that might be critical or whatever.
The secret club is located right here: https://blender.chat/channel/blender-coders
7 Likes
Some timely reply to patches in general would be good to improve on.
Iāll push back a little though. For patches that need to be looked over by multiple developers/designers. Itās not necessarily good use of time to raise their priority, postponing other plans - to meet an artificial deadline.
It may be good that there is some reply by a set time, even if the reply is that weāre not sure about the change (to let people know weāre aware of it and not ignoring them).
There is some onus on people writing patches to check if this is a change that is good use of their time too.
Side note, weāre not the only open-source project with some delays in patch review. I recently received an update to a patch I submitted to Python a decade ago.
6 Likes
Communication is the golden word all around.
I can imagine the core devs are rather busy, so communicating with (external) contributors falls through the cracks more often than desirable. Especially if itās a large patch, getting in the correct mindstate to review it can be quite time consuming. Leading to postponing it. Leading to forgetting about it.
There are 2 ways around this that actually work (IMHO).
- Making the core devs less busy by making blender less popular
. Or maybe the Blender foundation hiring more core devs. But growing too fast is dangerous as well, because it can lead to communcation breaking down between the core devs which is even worse. (Mythical Man-Month) - As a contributor, make it easier for the core devs by being extremely clear in your communication and documentation. Yes, that means a lot more work and time investment by the contributor. But I think thatās just the way it is with open source projects in general. You need to pitch your idea first (in the chat? over here?) get feedback. Only then start coding. Document your code so a toddler can understand it.
post scriptum:
Of course this is a utopia. In the real world you start some patch because something annoys you, or you need something. You hack together something that works for your own usecase and only then you start thinking of actually contributing it back. Also, even when doing everything right stuff will always fall through the cracks. Telling some contributor that his hard work will not be incorporated because reason is not a nice thing to do, especially if the contributor is (like most coders) likely to feel hurt and respond angrily. So if thereās tons of other work that needs to be done as well itās easier to get in the postponing cycle. Also as an external contributor itās hard to get all the documentation toddler-proof because you know the ins and outs of the problem your tackling, so itās hard to look at it with fresh eyes.
6 Likes
How about write-up on 'A Patchās Life"? Like write-up on a A Bugs Life.
That would make it so much more clear for a new contributor what will happen with his or her work after submitting a patch.
Iād probably rather extend the Contributing Code section on the wiki, since it covers a lot of ground already.
however the biggest issue currently is, the things that should happen and the things that actually happen are not the same, due to the people we need to do the code review being allocated elsewhere.
Which makes it really hard to give any guaranteed rule on if/when/how a review will happen for any specific patch.
The original suggestion here was basically when the module teams have their meetings to run through the newly submitted patches and greenlight the ideas they like and redlight the rest. I donāt know how many new patches each module gets pr. week, but I would guess it wouldnāt take more than 5 minuttes for each module member to give each new patch a vote. And then one module member would use/write a canned answer to the contributor - ex. if greenlight: āWe really like your idea, so if you do the work needed to meet these requirementsā¦ 1. 2. 3. etc. we will commit your patch to Blenderā. This would clean the plate for the devs on a weekly basis and motivate the contributor to go the extra mile.
a suggestion is pretty far from a rule though, you keep stabbing at this from different directions (which donāt get me wrong, i appreciate, i have had many conversation with ton about this very subject), but at itās core the people needing to do this do not have time allocated for it and unless something changes from the top down here, I donāt see it improving. Nor is it something we seemingly can force any change from the outside.
1 Like