This is better than having a decision made by someone who feels qualified without being qualified.
Nothing is better than damage.
That one-character spelling patch you referenced took me 45 minutes to commit in total - it was part of an optional component so I had to do a full build. So, as mentioned, this isnât about âwhy canât someone spend 5 seconds to click somethingâ but instead âwhy canât a volunteer find 45 minutes to do this particular thing versus all the other things they could be doing to improve blender.â
Does that include the ~10 minutes to run the tests? The number of times things got committed in a rush without testing going âthis shouldnât break anythingâ yet breaking some tests, is much further from zero than iâd like it to be.
I donât think the problem is âhired devs, who want to spend their time on their own workâ. Everybody wants to spend time on their own work, not only the hired devs. The problem is just that there are more people sending in patches than there are people available with enough overview of the complete codebase to be able commit them.
This is something all open source projects struggle with. In my experience blender does better than most. Itâs far from perfect, but still better than most.
Thatâs not to say we shouldnât keep searching for systems to help us manage this stuff better. I just donât think thereâs a silver bullet which can actually fix it completely.
One solution would be to ânot accept contributionsâ. But that would hardly be productive, because lots of progress blender makes does come from contributions, even if a lot of stuff falls through the cracks.
Another solution might be to hire someone specifically to just commit external contributions. It would need to be someone who gets intimately familiar with the whole codebase, then, because contributions are all over the place. Not impossible, but not easy either. And once someone gets to that level (s)he is probably more productive to blender as a whole doing something else.
Itâs a difficult problem to solve.
If you are closer to the codebase and deep code handling (math pole), you are farther from its actual application (art pole) and vice versa. This happens because you have limited time which allows you to be good in some specific area. Getting full stack is costly in development, getting wide range is costly in production.
I tried to depict such a dependency here.
Switching between tasks take efforts, getting deep into a task (obtaining a full context) take efforts.
In my opinion, prioritizing the tasks (task ranking) is also a separate management job that connects devs and industry requirements, so you have to be familiar with both of them at quite deep level, to distribute the amount of overall care properly.
But managing wide range require mastering wide range first.
In this position, you will have too much power and the ability to screw up a lot of things if you are not qualified in some areas (for example, when an animator familiar with maya is trying to design modeling workflows, confusing system design with features design or usability with low entry threshold), so this power is distributed among the modules.
Slower but safer?
Heh, indeed, wrong tab I guess Thanks for the note. Moved the comments.
Unfortunately, it is not that simple.
There are a couple of things a new hire could in fact do with the patch tracker, such as making sure the code compiles properly and fits the style guide. However, to actually commit the patch requires the guidance of someone who knows how the code works and how the new code will interact with existing code (ie. making sure the patch for starters is not a hack that could lead to a long game of whack-a-mole bugfixing). Please note the devs. look at the long-term as well, and while some patches look great from a userâs point of view, it could have broken design that could lead to a lot of dev. time on refactors that are preventable.
Committing blind is never recommended, and a hire who actually commits patches for the purpose of getting them in will do just that.
Yes, thatâs what I was trying to say. But you restate it clearer.
I think there are some improvements that could be imagined to the build/phab infrastructure, for instance the straight forward Quicktime
â QuickTime
rename, most devs will be perfectly capable of going âyeah this looks fineâ in well under a minute.
If there was functionality in our build infra to go âHey I sign off on this, if this builds, and passes all tests on all platforms, just go ahead and land this for meâ Iâd use it, thereâs no reason I should be spending 30-45 minutes on that, itâs something that could be the job of some automated process.
Isnât testing part of the review process? If you have accepted the patch, it means that it is correct, safe and can be committed. (If there is no additional comment.)
And running tests is definitely not for a contributor. You just canât rely on âsome kind ofâ contributor to do everything right. Iâve seen your recent discussions about how someone with commit rights did something wrong and no one noticed it for years.
Hah, I wish. âBeing reviewed by a humanâ is something completely different from âbeing bug-freeâ. Otherwise weâd be reviewing even more, and the bug tracker would be empty.
First of all, my comment was in the context of simple diffs, like the one I mentioned that corrected a spelling mistake, reviewing that vs reviewing a brand new cycles feature quite a different review process. One I can look at an go, âyeah this looks fineâ, the other deserves a much much closer look.
Second we run the tests every time the buildbot builds for a reason, itâs virtually impossible to make sure a single innocent looking change does not break any of our major supported platforms in unintended ways. Iâve had commits, that I had reviewed, I made sure all tests passed on my windows machine and they still caused a build or test error for either mac and/or Linux.
Iâm going to politely disagree there, I think it most definitely is, itâs your change, itâs your job to make sure your fix or feature didnât break anything. If Iâm reviewing your patch and it broke a test, it will be returned to you with a message saying âthis breaks test ABC, please fixâ
But you can rely on reviewers who are scrambling for time due to their existing workload to do âevery thing rightâ ? They are human, they will make mistakes.
Reviewers ought to spend their relative precious time looking at the guts of the diff, what is the diff doing, is it following our code style, are they using a bad algorithm, is it fitting in the overall blender architecture, etc. Spending 30-40 minutes doing a full build of blender and running the unit tests for a spelling change is a waste of time, robots can do it (Point is our current bots already can, they just lack the landing phase I envisioned)
@sybren RE: â D13609 Motion Path: Tweak the User Interface
What I have written is just my experience in two years. All are glad to see a little polishing of the interface, but do not have the ability to spend time on it, since there is always something more important.
We have already spent more time on these comments than it would take to make the proposed changes. So, IMHO, if such (not requiring a separate discussion) changes can be made at the same time with others, then why not.
And again, I just see that such patches can wait more than a year for at least some reaction from the developers, although they look straightforward.
In D13609 you wrote:
And the problem is that there is no other way to make such minor changes. Although no one is against such improvements.
Such a thing cannot go through the normal review process since developers are not interested in it as these are too insignificant.
The only possibility here is incorporation into a larger patch.
We have a team of developers who are already spread too thin over many different areas of Blender. The solution is not to make existing patches more complex than they have to be.
Also please do not read a patch sitting in the tracker as disinterest from developers. Itâs safe to assume that pretty much all developers are interested in improvements to Blender, big or small. Itâs more a matter of having the time and the mental space to do yet another context switch.
A patch appearing straight-forward does not mean it costs zero energy to context-switch to it, to read through the patch description, to figure out the writerâs mental model when they were writing it (necessary because often details are ommitted), then to decide whether the patch is actually doing anything desirable, and to check that it doesnât have any unintended/hidden side-effects.
This is exactly why I suggested including the changes in that patch rather than making a separate one. Itâs just, IMHO, easier for you. But of course you better know whatâs best for you.
And of course I will not add a separate patch for this because there are (and mine among them) more important fixes that are waiting for review for a long time.
I can understand this perfectly (itâs almost always the case in SW dev).
How about another idea: is there a way to maybe pass some of these reviews back onto the community?
Something like 3 community reviewers could equal 1 dev review. (depending on the case).
Pros:
- Faster development
- Less stress/workload on devs
- Maybe less cost on the long run (since devs can focus on their stuff)
Cons:
- A trust system needs to be put in place (ranks, exp, something something) that builds with experience
- Someone would have to do an initial filtering (what can be passed to the community)
- A form of reward? (titles, access to assets, access to courses for 1 month, whateverâŚ)
I donât know what the current system is, really, and this is probably a bit idealistic, but maybe worth for a conversation?
Thanks for the offer to help @cata_cg. Anyone is welcome to help in reviews. Especially when it comes to the âdetail levelâ, like code style, naming conventions, adequate code comments, etc. itâs not that hard to just join in and help out. Also when itâs about the local design of the code, i.e. the design of the patch itself, I think itâs fine to help out; just as an example, if you see that something is quadratic in complexity when it could also be linear, feel free to offer such feedback.
When it comes to bigger code design aspects, like whether the design of the patch actually matches the overall design of Blender, then of course it helps to have more Blender development experience/knowledge.
Still unreviewed. Is the module dead? If it is, maybe a BF dev could inform this contributor that the module is RIP.