Re: [Bf-committers] The Story Of My Attempts At Contributing Code To Blender
Since the last email, I have reached out to Bastien and Dalai on blender.chat where we discussed my patch[1], the discussion then moved to the Phabricator page[2] where I was finally given specifics of the issue Bastien had with my patch and the steps to reproduce it. It turns out there was another bug I had missed that affected deleting collections, but with a reproducible case to test against I was able to quickly fix the bug. However Dalai feels that there are more pressing matters than going forward with my original solution and that the mental energy to consider reviewing such a different approach [from Bastien's newer solution] is not something the core module can afford at the moment. Given this stance, I have abandoned my patch so that no more time will be lost, but I will be looking into creating some unit tests to help prevent bugs in any future work on layer collections. From my perspective, had these issues been brought to my attention when Bastien initially found them, and my patch been reviewed then, they likely could have been fixed in less than a week by me, and there would have been plenty of time for testing before release, plus Bastien would have been free to deal with more important things and we wouldn't need to be worrying about performance now. In situations like this it would help strengthen community developers if they are included in the process before an alternate commit is made rather than after. Ryan [1] https://blender.chat/channel/blender-coders?msg=vXPnBupiTtkCu8DzJ - The link only works if you're logged into blender.chat, if you aren't logged in you'll have to go to the blender-coders channel and scroll back. The conversation starts at 2:33 AM EDT, September 7, 2021, with the majority taking place between 3:26-4:08 AM EDT. [2] https://developer.blender.org/D9599 On 2021-08-17 06:00 AM, bf-committers-requ...@blender.org wrote: Date: Tue, 17 Aug 2021 07:47:25 + From: lumpeng...@posteo.de To: bf-blender developers Subject: Re: [Bf-committers] The Story Of My Attempts At Contributing Code To Blender Message-ID: <8ebe1f8c4485491f25d4cb50e8953...@posteo.de> Content-Type: text/plain; charset=UTF-8; format=flowed Hi Ryan and Dalai, I would like to chime in regarding the "thousands of collections" use case: When importing CAD geometry it is very common to have a hierarchy of collections in the hundreds or thousands of collections. Since this hierarchy is useful for organisational purposes it is often preferable to keep this hierarchy intact instead of spending time on re-organizing the scene. A performance boost for scenes with lots of collections would therefore be very welcome. Best Regards Johannes (Lumpengnom) Am 17.08.2021 06:41 schrieb Ryan Inch via Bf-committers: Hi Dalai, Thank you for looking into this and getting back to me with more specific details. I think there may be a slight misunderstanding with how my patch is designed. If by first matching user you mean the first layer collection in the tree, of a collection which has been linked to multiple layer collections, then my patch doesn't do this, it doesn't just take the first layer collection it comes across. My patch walks through the layer collection tree and the collection tree at the same time, when there are no structural changes it should always keep the trees perfectly in sync, when there are structural changes it takes only the invalid layer collections (layer collections that have been either significantly moved, or removed) and then remaps or removes only those invalid layer collections (the rest are relinked as when there are no structural changes) by order so that the correct layer collection should always end up at the correct new position in the tree. As far as I am aware there is no way for the order to change in a move, so it should always remap the layer collections successfully. Note: While figuring out a simple example to illustrate this, I did find and fix a small bug where some layer collections got missed, and I found that ASan had issues with the order I was deleting collections, so I fixed that too. I guess this is why patch review is a thing. ;) I've updated my diff with the fixes, but I would be very interested in hearing more about this problem that the studio ran into and that my patch didn't fix, and if it works now with the update. If Bastien would like to chime in here, that would be great. It's too bad he didn't mention it at the time, as it only took a couple days to fix, but better late than never. Note2: When I updated my diff I found that I had missed a change unrelated to the syncing algorithm, so my performance metrics were slightly off. The new metrics are: 296 microseconds to resync 1000 collections when configured so that each of the 1000 has 1 child. (2x faster than Bastien's) 294 microseconds to resync 1000 collections when configured as a flat list with all of the collections under the scene
Re: [Bf-committers] The Story Of My Attempts At Contributing Code To Blender
Hi Ryan and Dalai, I would like to chime in regarding the "thousands of collections" use case: When importing CAD geometry it is very common to have a hierarchy of collections in the hundreds or thousands of collections. Since this hierarchy is useful for organisational purposes it is often preferable to keep this hierarchy intact instead of spending time on re-organizing the scene. A performance boost for scenes with lots of collections would therefore be very welcome. Best Regards Johannes (Lumpengnom) Am 17.08.2021 06:41 schrieb Ryan Inch via Bf-committers: Hi Dalai, Thank you for looking into this and getting back to me with more specific details. I think there may be a slight misunderstanding with how my patch is designed. If by first matching user you mean the first layer collection in the tree, of a collection which has been linked to multiple layer collections, then my patch doesn't do this, it doesn't just take the first layer collection it comes across. My patch walks through the layer collection tree and the collection tree at the same time, when there are no structural changes it should always keep the trees perfectly in sync, when there are structural changes it takes only the invalid layer collections (layer collections that have been either significantly moved, or removed) and then remaps or removes only those invalid layer collections (the rest are relinked as when there are no structural changes) by order so that the correct layer collection should always end up at the correct new position in the tree. As far as I am aware there is no way for the order to change in a move, so it should always remap the layer collections successfully. Note: While figuring out a simple example to illustrate this, I did find and fix a small bug where some layer collections got missed, and I found that ASan had issues with the order I was deleting collections, so I fixed that too. I guess this is why patch review is a thing. ;) I've updated my diff with the fixes, but I would be very interested in hearing more about this problem that the studio ran into and that my patch didn't fix, and if it works now with the update. If Bastien would like to chime in here, that would be great. It's too bad he didn't mention it at the time, as it only took a couple days to fix, but better late than never. Note2: When I updated my diff I found that I had missed a change unrelated to the syncing algorithm, so my performance metrics were slightly off. The new metrics are: 296 microseconds to resync 1000 collections when configured so that each of the 1000 has 1 child. (2x faster than Bastien's) 294 microseconds to resync 1000 collections when configured as a flat list with all of the collections under the scene collection. (75x faster than Bastien's) When I looked over Bastien's patch a couple performance improvements did come to mind, and I wouldn't necessarily mind working on them with him, however, I think that the overall method of mine has more inherent performance gains, and shouldn't be abandoned without a fair trial. I too hope that communication can be improved so that situations like this don't arise in the future, and I'm here if you want any feedback on new communication guidelines, or whatever. Without the missed communication, I think there could have been a highly performant solution merged into master months ago. Speaking of missed communication, I noticed that you mentioned in blender.chat a desire to have unit tests included with the patch and that there is currently a lack of unit tests in core areas. Now that I think about it, I agree it would be a good idea to have unit tests for layer collections (although I'm not very familiar with unit tests and would need some direction to create my own), but unfortunately, there was never any mention of them on any of my patches so I didn't consider them before. Here is a simple test to show that my patch remaps the equivalent layer collection and not just the first that it comes across: 1. Create a set of collections like so: Collection - Collection 1 Collection 2 - Collection 1 (linked) 2. Turn off the exclude checkbox for Collection 1 under Collection 2 3. Move the excluded collection to before/above Collection. 4. See that the moved collection is still excluded. Note3: This didn't work before I updated my patch with my bug fix, but works now that I've found and fixed it. I know that it'll be somewhat of hassle looking into my patch again, but I think it's got the potential to be a really good contribution, and I hope you'll still consider it. I also hope Bastien will contribute his thoughts because I'd like to work with everyone on this. As a final note, I'm pushing a bit strongly for my patch because I think it will provide the greatest benefit to Blender (a 100%-7400% performance increase, in some situations, is no small thing), but if there is some inherent, insurmountable, flaw in the underlying design,
Re: [Bf-committers] The Story Of My Attempts At Contributing Code To Blender
Hi Dalai, Thank you for looking into this and getting back to me with more specific details. I think there may be a slight misunderstanding with how my patch is designed. If by first matching user you mean the first layer collection in the tree, of a collection which has been linked to multiple layer collections, then my patch doesn't do this, it doesn't just take the first layer collection it comes across. My patch walks through the layer collection tree and the collection tree at the same time, when there are no structural changes it should always keep the trees perfectly in sync, when there are structural changes it takes only the invalid layer collections (layer collections that have been either significantly moved, or removed) and then remaps or removes only those invalid layer collections (the rest are relinked as when there are no structural changes) by order so that the correct layer collection should always end up at the correct new position in the tree. As far as I am aware there is no way for the order to change in a move, so it should always remap the layer collections successfully. Note: While figuring out a simple example to illustrate this, I did find and fix a small bug where some layer collections got missed, and I found that ASan had issues with the order I was deleting collections, so I fixed that too. I guess this is why patch review is a thing. ;) I've updated my diff with the fixes, but I would be very interested in hearing more about this problem that the studio ran into and that my patch didn't fix, and if it works now with the update. If Bastien would like to chime in here, that would be great. It's too bad he didn't mention it at the time, as it only took a couple days to fix, but better late than never. Note2: When I updated my diff I found that I had missed a change unrelated to the syncing algorithm, so my performance metrics were slightly off. The new metrics are: 296 microseconds to resync 1000 collections when configured so that each of the 1000 has 1 child. (2x faster than Bastien's) 294 microseconds to resync 1000 collections when configured as a flat list with all of the collections under the scene collection. (75x faster than Bastien's) When I looked over Bastien's patch a couple performance improvements did come to mind, and I wouldn't necessarily mind working on them with him, however, I think that the overall method of mine has more inherent performance gains, and shouldn't be abandoned without a fair trial. I too hope that communication can be improved so that situations like this don't arise in the future, and I'm here if you want any feedback on new communication guidelines, or whatever. Without the missed communication, I think there could have been a highly performant solution merged into master months ago. Speaking of missed communication, I noticed that you mentioned in blender.chat a desire to have unit tests included with the patch and that there is currently a lack of unit tests in core areas. Now that I think about it, I agree it would be a good idea to have unit tests for layer collections (although I'm not very familiar with unit tests and would need some direction to create my own), but unfortunately, there was never any mention of them on any of my patches so I didn't consider them before. Here is a simple test to show that my patch remaps the equivalent layer collection and not just the first that it comes across: 1. Create a set of collections like so: Collection - Collection 1 Collection 2 - Collection 1 (linked) 2. Turn off the exclude checkbox for Collection 1 under Collection 2 3. Move the excluded collection to before/above Collection. 4. See that the moved collection is still excluded. Note3: This didn't work before I updated my patch with my bug fix, but works now that I've found and fixed it. I know that it'll be somewhat of hassle looking into my patch again, but I think it's got the potential to be a really good contribution, and I hope you'll still consider it. I also hope Bastien will contribute his thoughts because I'd like to work with everyone on this. As a final note, I'm pushing a bit strongly for my patch because I think it will provide the greatest benefit to Blender (a 100%-7400% performance increase, in some situations, is no small thing), but if there is some inherent, insurmountable, flaw in the underlying design, then I will of course be willing to work with Bastien on his patch to provide the much needed performance improvements, because while the collection system may not have been designed with 1000s of collections in mind, it's a valid use case and one that I think is only impeded by some implementation details. All the best, Ryan On 2021-08-13 06:00 AM, bf-committers-requ...@blender.org wrote: Date: Thu, 12 Aug 2021 17:42:22 +0200 From: Dalai Felinto To: bf-blender developers Cc: Ryan Inch Subject: Re:
Re: [Bf-committers] The Story Of My Attempts At Contributing Code To Blender
Hi Ryan, More details about your specific patch (D9599). The solution you found has some merits, however it is incomplete and doesn't work as well as the one that we now have in master. More specifically it doesn't account for cases when the collection is linked inside more than one collection (it would just get the first matching user of said collection). This problem wasn't considered a priority until a related issue came up (overrides resync in some production files). By then Bastien tried your patch which didn't fix the problem. He then proceed to create his own solution, which is what ended up being merged. The use case of 9,000 collections is not one that was taken into consideration for the solution. It simply doesn't represent the type of scene the system is built for. That said this can be improved a bit. So if you are interested on help this further please reach out to Bastien, he has some ideas on how the code can iterate a bit faster on the collections. He is also available if you need more clarifications on the shortcomes of your patch. If you need more clarifications let me know. All in all I hope we (the development team) can improve the communication even further to prevent situations like this one. -Dalai- Dalai Felinto - da...@blender.org - www.blender.org Blender Development Coordinator Buikslotermeerplein 161, 1025 ET Amsterdam, the Netherlands Op wo 11 aug. 2021 om 10:25 schreef Dalai Felinto : > Hi Ryan, > Thanks for your email. > > I will talk to Bastien to understand better the decision process for this > particular patch. See what caused your patch to be abandoned, and if there > is room for further performance improvements. I will get back to you here. > > Furthermore, you brought a few points that the Blender project has to > improve: > > * Communication of the modules agendas. > * Module to be more open to contributors. > * Patch review process (e.g., clear process or assigning, set expectations > upfront, better communication). > > As a rule of thumb any developer who is driven to collaborate to Blender > overall agenda and shows the dedication and competence should be welcomed > into the modules. Even for modules where the bar is a bit higher, such as > the core module [1]. > > All in all this comes in in a good time. In a few weeks Thomas Dinges will > start helping to coordinate the online development community. And this is a > very clear use case of what he can look at to help for improvements. > > Meanwhile as a general rule if a module is not working well this can be > escalated to the bf-admins [2]. As a last resource if someone think that > the bf-admins are not addressing things properly or in a fashionable time, > it can escalate further to Ton. > > [1] - > https://code.blender.org/2021/02/module-teams-for-core-blender-development/ > [2] - https://wiki.blender.org/wiki/Modules#Blender > > Regards, > -Dalai- > > Dalai Felinto - da...@blender.org - www.blender.org > Blender Development Coordinator > Buikslotermeerplein 161, 1025 ET Amsterdam, the Netherlands > > > Op wo 11 aug. 2021 om 01:17 schreef Ryan Inch via Bf-committers < > bf-committers@blender.org>: > >> To the core Blender developers. >> Hello again, Collection Manager dev here. >> Let me start off by saying that I care a lot about Blender, it's been a >> huge influence on my life and an inspiring story of the little DCC app >> that could, and I want nothing but the best for it. So it highly saddens >> me that I feel I need to write this email. >> >> I've been a part of the Blender community and writing add-ons for it for >> eight years and a user of Blender for even longer. I love the power it >> gives me to create, and have always tried to give back with what I have >> to give (Unfortunately, money is not one of those ways, so I have >> contributed code, feedback, and support instead). >> >> In 2019 I separated the Collection Manager out of a larger add-on and >> submitted it to be included in Blender as a bundled add-on on the advice >> of Brendon Murphy (meta-androcto). Soon after my submission, I was >> contacted by Paul Kotelevets (1D_Inc) who expressed the short comings of >> the current collections system for complex scene setup and precise >> modeling that requires multiple reference images. Together, along with >> others who gave feedback, we have advanced the usability of Blender for >> complex setups and increased the capabilities of the new collection >> system. When I started this project I assumed that it would be highly >> welcomed, because collections were one of the pillars of the 2.8 >> project, however, it was mostly ignored. Still, I continued on with the >> project because it was useful to me and was essential to others for >> their jobs. >> >> In the year and a half I've worked on the project, I've become pretty >> familiar with the
Re: [Bf-committers] The Story Of My Attempts At Contributing Code To Blender
Hi Ryan, Thanks for your email. I will talk to Bastien to understand better the decision process for this particular patch. See what caused your patch to be abandoned, and if there is room for further performance improvements. I will get back to you here. Furthermore, you brought a few points that the Blender project has to improve: * Communication of the modules agendas. * Module to be more open to contributors. * Patch review process (e.g., clear process or assigning, set expectations upfront, better communication). As a rule of thumb any developer who is driven to collaborate to Blender overall agenda and shows the dedication and competence should be welcomed into the modules. Even for modules where the bar is a bit higher, such as the core module [1]. All in all this comes in in a good time. In a few weeks Thomas Dinges will start helping to coordinate the online development community. And this is a very clear use case of what he can look at to help for improvements. Meanwhile as a general rule if a module is not working well this can be escalated to the bf-admins [2]. As a last resource if someone think that the bf-admins are not addressing things properly or in a fashionable time, it can escalate further to Ton. [1] - https://code.blender.org/2021/02/module-teams-for-core-blender-development/ [2] - https://wiki.blender.org/wiki/Modules#Blender Regards, -Dalai- Dalai Felinto - da...@blender.org - www.blender.org Blender Development Coordinator Buikslotermeerplein 161, 1025 ET Amsterdam, the Netherlands Op wo 11 aug. 2021 om 01:17 schreef Ryan Inch via Bf-committers < bf-committers@blender.org>: > To the core Blender developers. > Hello again, Collection Manager dev here. > Let me start off by saying that I care a lot about Blender, it's been a > huge influence on my life and an inspiring story of the little DCC app > that could, and I want nothing but the best for it. So it highly saddens > me that I feel I need to write this email. > > I've been a part of the Blender community and writing add-ons for it for > eight years and a user of Blender for even longer. I love the power it > gives me to create, and have always tried to give back with what I have > to give (Unfortunately, money is not one of those ways, so I have > contributed code, feedback, and support instead). > > In 2019 I separated the Collection Manager out of a larger add-on and > submitted it to be included in Blender as a bundled add-on on the advice > of Brendon Murphy (meta-androcto). Soon after my submission, I was > contacted by Paul Kotelevets (1D_Inc) who expressed the short comings of > the current collections system for complex scene setup and precise > modeling that requires multiple reference images. Together, along with > others who gave feedback, we have advanced the usability of Blender for > complex setups and increased the capabilities of the new collection > system. When I started this project I assumed that it would be highly > welcomed, because collections were one of the pillars of the 2.8 > project, however, it was mostly ignored. Still, I continued on with the > project because it was useful to me and was essential to others for > their jobs. > > In the year and a half I've worked on the project, I've become pretty > familiar with the python side of collections and layer collections, but > during that time I ran into problems that I thought would be better > solved on the C side of Blender, so I spent ~6 months familiarizing > myself with layer collections and attempting to solve some of the > problems I'd run into, and then ultimately trying to stabilize layer > collections as a first step to further improvements. I submitted three > patches during this time, one of which went nowhere (and rightly so), > one of which was a draft and that I used to ask for help when stuck (to > which was replied: Thanks for the patch, but it's up to you to present a > working code, otherwise it's likely a loss of time for everybody...), > and then a final one, which did contain working code. > > I submitted the final patch on November 18th 2020 and assigned Bastien > Montagne, Brecht Van Lommel, and Dalai Felinto as reviewers; two months > later I updated the patch to the latest master and linked a good test > file I had made; ~2 weeks after that, Bastien responded that the patch > hadn't been forgotten and that he'd just had a lot on his plate; a few > days after this Hans Goudey (HooglyBoogly) reviewed the patch and > provided some really good comments (he didn't have to do this and I'm > really grateful to him), which I then addressed a couple weeks later (I > had been busy and things didn't seem urgent); three months later I > updated the patch to the latest master again; and then almost two months > after that it was abandoned by Bastien who had apparently just committed > his own solution. > > As you might imagine, this was fairly
[Bf-committers] The Story Of My Attempts At Contributing Code To Blender
To the core Blender developers. Hello again, Collection Manager dev here. Let me start off by saying that I care a lot about Blender, it's been a huge influence on my life and an inspiring story of the little DCC app that could, and I want nothing but the best for it. So it highly saddens me that I feel I need to write this email. I've been a part of the Blender community and writing add-ons for it for eight years and a user of Blender for even longer. I love the power it gives me to create, and have always tried to give back with what I have to give (Unfortunately, money is not one of those ways, so I have contributed code, feedback, and support instead). In 2019 I separated the Collection Manager out of a larger add-on and submitted it to be included in Blender as a bundled add-on on the advice of Brendon Murphy (meta-androcto). Soon after my submission, I was contacted by Paul Kotelevets (1D_Inc) who expressed the short comings of the current collections system for complex scene setup and precise modeling that requires multiple reference images. Together, along with others who gave feedback, we have advanced the usability of Blender for complex setups and increased the capabilities of the new collection system. When I started this project I assumed that it would be highly welcomed, because collections were one of the pillars of the 2.8 project, however, it was mostly ignored. Still, I continued on with the project because it was useful to me and was essential to others for their jobs. In the year and a half I've worked on the project, I've become pretty familiar with the python side of collections and layer collections, but during that time I ran into problems that I thought would be better solved on the C side of Blender, so I spent ~6 months familiarizing myself with layer collections and attempting to solve some of the problems I'd run into, and then ultimately trying to stabilize layer collections as a first step to further improvements. I submitted three patches during this time, one of which went nowhere (and rightly so), one of which was a draft and that I used to ask for help when stuck (to which was replied: Thanks for the patch, but it's up to you to present a working code, otherwise it's likely a loss of time for everybody...), and then a final one, which did contain working code. I submitted the final patch on November 18th 2020 and assigned Bastien Montagne, Brecht Van Lommel, and Dalai Felinto as reviewers; two months later I updated the patch to the latest master and linked a good test file I had made; ~2 weeks after that, Bastien responded that the patch hadn't been forgotten and that he'd just had a lot on his plate; a few days after this Hans Goudey (HooglyBoogly) reviewed the patch and provided some really good comments (he didn't have to do this and I'm really grateful to him), which I then addressed a couple weeks later (I had been busy and things didn't seem urgent); three months later I updated the patch to the latest master again; and then almost two months after that it was abandoned by Bastien who had apparently just committed his own solution. As you might imagine, this was fairly distressing to me as I had put a lot of time and effort into creating and then maintaining it. I had been content to be patient and not push for my patch, because I thought it was of a low priority and I knew from various sources of Blender communication that Bastien had been very busy and I didn't want to add to the stress he was under. So, after the patch was closed, I looked into the one that replaced it and at Bastien's weekly reports to try and figure out what had happened. On inspection of Bastien's weekly reports, I found that he had been working on his own solution for a month or so, but I hadn't connected the dots because I thought he was working on stuff for library overrides and that if he had needed something to preserve layer collections he would have used the already working code that had been sitting in the patch tracker for six months (or if a different direction was required, told me and brought me onboard with developing the new patch). And when I looked at the patch that replaced mine, I found a comment by Brecht saying it was "Great to see this tackled.", as if now that a "real" programmer was working on it, it was suddenly much more important and welcomed. There are two issues here, with the first being how everything was handled, the lack of communication and the disregard for the time I spent attempting to contribute. I have seen in official Blender communications that one of the goals of Blender is to onboard new developers, this is a perfect example of how not to do that. Campbell Barton once said something in IRC about GSoC students that I have always found inspiring: "Typically for students who don't know what they want they ask a lot of Q's... and never get involved. (sounds a bit
