In your case there might be an assumption that you have commit rights. Just send a note to Campbell asking for such and see what he says. You don’t get to just commit whatever you want - things are still reviewed the same way - but at least you could do so once accepted. It saves them the time to do so, which does take a few minutes per. A couple of the patches you referenced are without a “reviewer” set which makes them not appear on some searches - just needing “User Interface” as group reviewer.
6 Likes
It is certainly more difficult to achieve alignment within a larger team, and in this case an extremely large team. Alignment is the key issue that I can see here and that’s not a straightforward thing to solve. Some responsibility is on the proposing developer to ensure the general aim of their proposal is aligned with the direction the core developers wish to take the code, which might not be immediately clear to some new contributors.
That being said, once you get to the point of submitting code to be reviewed for merging, I would expect alignment to not be too much of an issue. Can PRs from external contributors be handled in the same way as between core developers? I don’t see why not. There is of course going to be more friction, which makes clear process and communication even more important. Having an action point from each review is a useful tool for core developers and community contributors alike.
7 Likes
One practical tip which could be used to resurface issues like this without such an absolutist approach as a curfew/deadline is to have a ‘stale-bot’ style automation which will notify relevant people of things that might be in limbo. The utility of this is simply to nudge the reviewer or developer to clarify the status and who is waiting on what.
10 Likes
Just to focus a little too much on these examples, I think they would be much easier to review if there were more context included. It’s helpful to assume that the reviewer might not have all the fresh knowledge of that specific area of code that you do, or at least it was a while since they looked at it.
The Ingredients of a Patch may be a bit over the top in some situations, but usually that sort of info would be extremely helpful. Personally when I see a patch without much of a description I think “Oof, not right now, I’ll come back to this when I have more headspace.” Because I need to fully immerse myself in this issue basically from nothing.
6 Likes
Try to imagine what he sees in email: a diff is being updated. To catch his attention, use @< name> maybe ? I used to wait for a week and then comment that I don’t have commit rights, please commit it.
I’ve almost always gotten a response within a day on my (petty) (non-UI) patches. Is it that UI diffs quickly go from code review to design discussions ?
Not really, a discussion about merging or closing old patches took place in chat. Also, using blocking reviewers (it doesn’t directly affect this post, but is good for reviewers), and having something that can indicate contributors without commit access was also discussed.
@Harleya @HooglyBoogly @ankitm
Yes, that’s right, the patch may have disadvantages (insufficient description, no reviewer, etc.) or just be lost, but these cases could be handled (moderated) somehow, giving some feedback so that the contributor can see what’s going on.
Yes, it would be nice to have a triaging pass over patches that could be done before the full-timers are even aware of them. Could address things like lack of context, needing more explanation, need for before/after captures. And only when it appears to contain all we expect of a patch would it get a status that would make it more visible. And, like bad bug reports, could be closed if the author did not address the concerns in a reasonable amount of time.
4 Likes
I’m curious how much of the review process is definable? And how much of the definable review processes are automated?
If, as a contributor I could run a lint on the entire codebase that gave a report of the items that are “wrong”, that gets me closer. This probably exists but I’m not aware of it.
Not exactly sure what you are asking. Anyone in the world can download the entire Blender codebase, and every code contributor does. So you can easily run any tool on it that you wish.
Sorry, I was specifically asking if Blender foundation has a recommended tool that has all “their settings”/ preferences with it.
I might be misunderstanding, but you probably won’t be fixing things that are flagged as “wrong” by some tool. First you’d get started by downloading and compiling the source. All the modules have bug lists, workboards, etc to coordinate contributions. And you’d follow code documentation and style guides to submit code that meets agreed formats that are reviewed and possibly accepted.
2 Likes
For general formatting we use clang-format which is tuned to our preferred codestyle, but things like non-functional changes , weird variables names, etc are rather hard to automatically test for.
Weird variable names (at least in C#) can be detected and refactored automatically by JetBrains and other tools.
It’s probably a pointless suggestion. Apologies for the noise.
Seriously, not noise at all. If you have any interest at all, I recommend downloading the source and getting compiling. The source is so huge that even very small contributions are appreciated. Fix grammar and spelling mistakes, improve tooltips, etc
While it’s easy to see if they used the right style for a variable, it be hard to tell if the name makes any sense. Most of the time the “weirdness” of a variable name is largely depended on context. It be real hard for automatic tools to see the difference between.
int sales_per_customer = Sales.count() / Customers.count();
and
int lazy_dodo_likes_kittens = Sales.count() / Customers.count();
1 Like
I think I could make another topic about this but if “good enough” reduces the volume that reviewers have to check then there are sane checks that can be produced from this.
Depending on code style:
- Arrays declarations contain a plural noun
- Each function declaration is prefixed with a three letter namespace identifier, then an underscore
- More stuff here…
Suffice to say that what I’m suggesting is getting quite a bit off topic here. If anyone’s interested I might be able to put together a toybox project to play around with, but that would be in its own topic.
Are you wondering if you should embark on such a thing? Absolutely. Download the source, and run any tools you want on it. And if you find anything that can be improved, do so, submit a patch and “sell” the change. If reviewers agree with you then it will improve blender and we all win.
Well, we hope he will not bring all that C4D UI problems into Blender, like William did.
It took a lot of time and efforts to clean up all those critical workflow issues after him, and some of them are still opened.
Williams designs were reviewed by the UI team, not liking them is not an excuse to make this personal. It’s also not very inviting for potential designers/contributors.
18 Likes
Do you mean the same UI/UX team, that brought broken vertices selection to a master for a year, and was protecting it for a half of a year, making fast modeling workflows literally impossible by default?
Or all those “borrowed designs” that forced people urgently to redesign them to make somehow viable?
It seems that such a team is in a safe zone from the decisions they make.
Breaking fingers to people making it personal, not we.
UI/UX is important. Especially in Blender, because it is used for workflows that other software can’t even handle.