Patch reviewing, the long waits involved now and the impact on development.

I think it should default to the user’s startup file. Maybe with the option to change it in the users preferences.

As for submitting patches, would it be possible for a module leader to just take a day or two right after the release to take a look at the patches related to their module, without looking at the code? Basically seeing if this is something that we want in blender?

Taking time immediately after a release to review patches makes sense in my opinion. But this shouldn’t be the first time that a reviewer gets in touch with that topic. As written in my preview post, the communication should start earlier and be accessible at least for everyone who has something relevant to say.

To be clear, I’ve brought up particular patches not to debate their merits but as examples of bad experiences that keep developers away. I’m far from an isolated case.

I’ve become convinced that many people at the BF think it is just a normal circumstance that new people pop up, create a patch or two, and are never heard from again. It will be rationalized as a normal thing that happens at other projects, that people change jobs, lose spare time, find the source too complex, etc. So the BF remains blissfully unaware that it is their behavior that is responsible for the turnover.

It is far better the cultivate and nurture existing relationships than to be always attracting, then losing, new people. I’m still chuckling that a possible solution to the problem of 253 contributors being removed from the “Credits” page is to just remove the credits page…

They should put that on the TODO list. Along with reset to defaults…

In all fairness, I think that’s partially just Campbell. I’ve seen (and made) a couple bug reports where his proposed fix was to remove the feature (can’t think of them off of the top of my head, sadly). Always makes me giggle. That said, I think it’s brought up as an option, but I don’t think (I hope) that it’s an actual consideration.

Yes, I’ve seen a few of those and always chuckle too. Not sure that I’ve ever noticed anything like that actually removed, so always chalk it up to the frustration of never-ending bug fixes. But always a funny eye-rolling moment.

No, I don’t think so either. My real concern is there will be no action at all. I haven’t noticed it mentioned in the months that its been a problem, there’s been a little discussion when I brought it up on the mailing list, but that interest will probably fade as everyone gets back to real work. I think a very likely scenario is that the credits page will remain incomplete for another year, then I’ll bring it up again, and then we’ll get more “we should just remove the page” musings once again. Repeat…

a few points to reply on.


There is some notion that features don’t exist in Blender, because we (existing developers) don’t have the time/resources/ability to add them.
This is sometimes the case (awesome-physics-integration is one example), but for smaller features, often its not the case.

This means if a new developer comes along and picks low-hanging-fruit, they often end up adding things which are not obvious improvements (useful in corner cases, or only useful in obscure cases).

So - if having your patch accepted in master is important, always talk with the person who will review your patch first. I can’t stress that enough (check module owner list).
Note that this does happen, I mail new devs and talk with them about areas they might be interested in working on, and see what fits well with current design.
If you like to work on features existing devs don’t want, this is a risk you take and YMMV.

Wanting to work on low-hanging-fruit is fine, and a good way to start. thats why I setup the quick-hacks project.
https://developer.blender.org/project/view/34/

Suggestions for more quick hacks are always welcome.


@Harley - regarding your massive defaults patch. IMHO doing this as a patch is just not very good. Its too risky and you do too much work with a chance we don’t accept.

In this particular case, the patch was never rejected, but it was never prioritized either, and I take some blame here. Though the fact Ton suggested to remove the feature didn’t make me motivated to spend time on this either.

For these kinds of bigger changes, I would propose to do them in steps.

  • Make a branch, get commit access (just ask)
  • agree on conventions headers & naming.
  • implement for eg- toolsettings
  • get review and acceptance from developers
  • merge into master
  • rinse-and-repeat with each RNA file.

Regarding credits, from the outside - I can see this looks bad, We credit patch submitters, and at some point we stop.
From my perspective - I had the data available, so I whipped up a script to digest it and spit-out-credits. But now I don’t and such a script is further complicated by having to support 2 different patch-tracking systems.

While its not impossible to support this, its now a fairly large (?) task, and takes time away from development. I don’t say not to support, just that someone needs to investigate extracting this data from Phabricator, and right now its not a priority for me.


Regarding removing features - sometimes features are added without very good review (or as part of a larger change, and we don’t property review implications of every detail), in those cases there are features we may not be able to support well, there are even features that work-by-accident (ran into one yesterday - loop-cut-on-a-single-edge *). in those cases I think it can be reasonable to remove the feature - but each case needs to be reviewed on its own merits.


At this point, I could stop doing any development, I could just reply to tracker tasks, check bug reports - review and comment on poorly written patches, maintain infrastructure and fix docs all day long. Not joking, but I didn’t sign up for that, I work on Blender because I enjoy it, I accept a certain amount of less-interesting tasks - because its needed, but I really don’t want it to become a focus, if thats the case - I rather do something else with my time.

Thats speaking for myself, of course BF needs to handle this so there are enough devs assigned to patch review, fixes, admin duties - but also keep existing employees interested enough they don’t quit and go work on something else, its a tricky balance.


Thanks for the tips. Much appreciated!

To be fair though…

I initially started with a patch that changed the defaults for only 40 RNA properties (Properties, World, and Object panels), then made another ticket for a patch for a further 72 defaults, then later a third one was added for another 100. You then asked me to put all the defaults into a single “DNA_defaults.h” file, which I did. Then Brecht asked that the defaults instead be in each DNA file where the relevant structs are defined, so I did that too. This did make for a large patch but when I asked if it would help the review to break it up into smaller pieces Brecht said to not bother and said that “the way it’s implemented seems fine”.

So yes, it was done in stages and had input from you and Brecht throughout.

When it hadn’t been reviewed for a few months I sent a reminder to the mailing list. Your answer was “I talked with Ton about this and he suggest to remove the ‘Restore to Defaults’ button” and that was basically the end of it. So no, I don’t think the result would be any different if I had used a different procedure.

Ok, well this is really bad on a few levels.

I think the key issue here is.

  • a feature was added half-working, and nobody owns the problem (responsible to make it work).
  • active devs can’t agree on right course of action.

Personally I don’t feel strongly enough about this feature to sink a lot of time into it, since you already did, its also not fair to leave the patch undecided.

Id suggest to kick devs to come to conclusion.

Either…

  • remove the feature.
  • support it properly, make a target for next release (2.73) and assign someone to update your patch and apply it.

I really don’t like having features in an unknown state - but also don’t like to make it my personal project to go around kicking devs about this stuff (its draining and just ends up in arguments from both users and devs).

My point in bringing this particular thing up wasn’t to rehash the topic but to keep with the original topic of this thread: an example of how the patch review process can impact on development and on potential developers. It was a souring process.

I would have felt much better about the entire thing had someone actually taken the half-hour (or less) in the past two years to actually remove the “Reset to Default” item as Ton requested. And if anyone had taken the 30 seconds or so needed to remove this task that still remains on the “Development Simple To Dos” page.

And as for the credits page, I hate to go on about it and look petty but I propose the following mental exercise:

Imagine that the incomplete Credits list came out in June, as it did. However, the now-missing 253 contributors were shown, while the others were omitted. This would mean that it would not include Ton, Campbell, Sergey, Bastien, Antony, Brecht, Joshua, Thomas, Dalai, etc.

If this were the case I highly doubt that it would be considered an unimportant problem. I’d wager that it would be have been fixed before that release.

Am i the only one who wants the bisect-modifier???

No, I think it is an interesting thing that would have some possible uses that could span more than just model output.

Hey @Harley,
I’m sorry to hear about what happend in the past. This is a typical negative example of how ridiculous the “post coding process” in UI-development can be :(.
I had a deeper look at your work, like your Blender UI Experiments site. You have a lot of really interesting proposals there, which we still don’t have in 2.7x, even though we should! Especially the points Modal Status, Scrollbars, Window Borders and Info Header Statistics, whereby the latest one is finally gathering pace (at least I hope so). Other points are broaching some different problems, which I personally would like to see solved a bit differently, but that are going into the right direction.
I also had a quick look at the Properties Panel Buttons thread, where I stumbled over the idea of making the Properties header left aligned. I think this would be a possible nice solution for my Properties tabs patch (someone already poked me about this idea, but I rejected it due to screen space). I will talk to the rest of the UI-team about this.

About your “Reset to Default” patch…
This is something that would be really worth adding IMHO (+1 from me). We should talk to the UI Mafia about making it a target for 2.73. I offer myself here for reviewing (@ideasman42, of course you can still have a second look on it). BTW, I much prefer the “DNA_defaults.h” version, as it keeps the code much cleaner and better organized.

To get back to the tread’s topic…
I think a bad reviewing process is a bitter pill to swallow for open source projects of our size. The main cause seems to come from the collision of many external contributors on one hand and only a few (payed) core developers on the other. This effect is boosted by the quality standards we have, but of course lowering quality standards isn’t really a solution. Would be interesting to see how other big OS communities handle the review process.
It seems to me, that this happens more on UI related tasks then on others, which isn’t a big suprise for me as everybody has a different opinion on how stuff should look/work and everybody has more insight into the changes (since UI changes are often visual), so things tend to get controversial.
In my experience, the best that can be done to get things rolling (to get patches reviewed in this case), is to talk with other devs/users, write to the mailing lists and be available on IRC. Often times it is needed to be a bit pushy, but don’t overdo it. I also found out, that it helps to set a proper version goal to the people involved.

Finally, I’d like to thank @Harley for the work he has done in the past! You were a victim of the bad organisation that ruled UI development. However, things have changed. You’re really welcome, more than ever. We need and appreciate your help! Feel free to poke me or the other UI guys on IRC :).

Cheers
Severin

Thanks for the kind words Severin!

I should say that I love Blender and I’m really not a complainer who is looking for either sympathy or a pat on the back. My general message is that it is important to encourage and cultivate new developers. They might be a pain since they are more likely to pick easy things (at first) and do them badly (at first), but it is within that group that we will find our good ones. We just don’t know how many potentially-good developers we’ve scared away.

On that “Header stats” task (now closed) Ton added a comment that “BTW: and I would prefer people to work on real issues” and went on to list a number of other things that he deemed more important than that one. But when even small changes cannot be accepted it makes it less likely that anyone will move on to more complex ones.

Cheers, Harley

You make the assertion we have double-standards here, which I really don’t think is the case.

Since it seems this is causing ill will between contributors, Proposing we drop this page until the patch contributors issue is resolved:
http://markmail.org/message/7q3tjkjquzc5uegr

That’s icing on the cake… Not only is there total uncertainty to get patches through, you don’t even get enough respect to not be insulted.

I think an experimental (user run) fork of Blender would be useful, where all stable patches are included would be interesting and alleviate some of the problems. That way real users could vote with their downloads.

We had Blender tuhopuu branch years ago which was basically a free-for-all (in fact this is how I got involved in Blender development, was great to be able to hack on stuff without so much red-tape :slight_smile: )

So +1 for a user-run-repo, a place to experiment with patches, try things out and gain experience, I’m kind of surprised this didn’t happen already. - Anyone can set this up using one of the many free git hosting services out there.

To be honest, there has been more movement on UI patches then there has been in the past, the panel grab widgets were recently changed to a more standard design for instance.

There’s still some significant challenges facing patch developers though that can’t readily be faulted on their character or their code, like what’s been pointed out about larger changes and functionality additions that are key to gaining new developers who are ready to try to show the world that it’s possible for a FOSS project to be innovative.

There really needs to be a development phase (Bcon1?) devoted to communicating with patch developers and helping them get their work into Master, much like how it is now with the focus on bugfixing late in the cycle. This will help Blender grow in the area of volunteer-based development which may pay big dividends in the future (and in turn more revenue for open movie projects and hired dev. work).

That works too, I rather just take the patch down until its resolved properly though, at this point, I regret having written the credits generator. It seemed like a nice idea at the time - but since then its caused more trouble IMHO then its helped, and I don’t especially care to keep it. (getting personal mails about missing credits and have to debug this stuff from 50k+ commits is really quite a hassle)

If a new dev comes in and makes changes we never asked for, are very subjective (and wouldn’t accept from experienced devs even), its tricky - in cases where its obviously a bad idea - then reject immediate is fine of course. But as you can see, its not always that simple.