@julianeisel I feel like your posts were meant for the module page improvement thread 
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.
1 Like
Yes, that’s what I was trying to say. But you restate it clearer.
1 Like
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.
11 Likes
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.
1 Like
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)
1 Like
@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.
12 Likes
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?
4 Likes
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.
3 Likes
Still unreviewed. Is the module dead? If it is, maybe a BF dev could inform this contributor that the module is RIP.
5 Likes
Which doesn’t mean much. And it will continue to grow. Yes, there’s a problem keeping up with reviewing. But even if there wasn’t the number woudl keep to grow because not all contributors mark a patch as ‘abandoned’ when they decide the patch was not the right solution, or too much hassle or whatever myriad other reason a patch will not get used.
A lot of patches are experiments, never even really meant to be included in that incarnation.
Yes, it would be nicer if people marked abandoned patches as such. Herding cats is not easy.
7 Likes
What do I see:
- Curfew: Dead month for new features.
- Lots of old patches: 1200 over 9 years. Taking a square distribution… 200 patches in the last year? that is, a 10 patchs per week. For the whole blender… Wow, that’s a problem!
- My patch is not accepted for a long time, or not considered: why is the inspector to blame. And can this inspector really do it? Have you made a mega patch that rewrites half of the program? …
- My patch was accepted by one of the verifiers, but not by the other: Mh…
- My patch took a very long time to update to master: Hmm…
- My patch: … Stop. Who else is working in this area? Did you agree on this beforehand? Did your patch stop merging properly because someone was doing more consistent work elsewhere? Was your work pre-agreed, or did someone who already did it have to redo their plans because of you?
- The problem is that there are few specialists to check: Yes. And I don’t think that a quantitative increase in specialists will help. More complex review steps could help. For example, so that at the beginning the review would be carried out by those who are not a high-level specialist and can weed out beginners, like me a year ago (my first patch is still not accepted, in it I taught c ++, blender api, tracked changes in the api I was using added and removed a bunch of features depending on the change of vision, …) and if I could not waste the time of one of the two owner of the module with this patch, it would probably be so great
6 Likes
Another approach, not mentioned here, is introducing a QC & QA team, to not only do the testing of patches, but also do the work to make patches align with both the coding and design standards as well as pruning bad ideas. So, basically, would this QA & QC team be the first to deal with contributor patches, no matter what module the contribution would belong to. Having QA & QC team would also ensure the quality of BF dev patches. Actually, thinking about it, with a huge code base in Blender, it is quite surprising that there isn’t a QA & QC team.
5 Likes
Blender foundation Need to grow team…