[Bf-committers] Code cleanup woes

Campbell Barton ideasman42 at gmail.com
Tue Jan 29 03:40:12 CET 2013


On Tue, Jan 29, 2013 at 12:26 PM, bjornmose at gmx.net <bjornmose at gmx.net> wrote:
> Am 27.01.2013 14:24, schrieb Ton Roosendaal:
>> Hi Campbell,
>>
>> There's enough room within Blender to have different ideas about coding practices, that's why we have (and should reconfirm) module ownership.
>>
>> For code someone actively maintains, and if agreed on with the co-maintainers, a cleanup of code is valid and shouldn't be an issue for the rest of the team. It's still a decent act to keep an eye at people doing branches, and communicate work well before you do.
>>
>> That means that for all the orphaned (unmaintained) parts of code, work should be limited to actual fixes or features benfiting users.
> This may be a mayor point why modules are orphaned (unmaintained) .. may
> be the owner got tired  of fixing 'fixes'.
> I have got the soft body module raped 3 times .. my concept on UV
> painting not to mention (sorry Brecht but I need to say it, being a
> painting artist with real brushes, charcoal ,pastel and all .. had an
> idea how it should work
> .. well I guess in future no one really cares what a charcoal or brush
> feels like and how it looks like on the canvas ..  )
>
> The python API changes did ruin some research code I did. That made 'das
> Fass ist übergelaufen'  a German idiom
> that has some more even nastier forms.
>   blender trucks on .. keep on trucking .. 16 tons and what do you get

Jens, I think its best if we restrict the discussion to code-cleanup,
Other people `improving` your code is a whole other topic.

If you have problems with improvements/features other devs do to your
code you should raise the issue when you notice it.

Python API changes are also unrelated, I don't know about the
specifics in your case but a while ago we did agree to do large
changes to the Python API that we knew would break scripts.

>> For the parts where I'm involved, and for general bigger refactor projects involving multiple modules, I'll always insist on a clear benefit for using Blender first.
>>
>> -Ton-
>>
>> ------------------------------------------------------------------------
>> Ton Roosendaal  Blender Foundationton at blender.org     www.blender.org
>> Blender Institute   Entrepotdok 57A  1018AD Amsterdam   The Netherlands
>>
>> On 27 Jan, 2013, at 13:24, Campbell Barton wrote:
>>
>>> Of course this is not a "fix", but as our API's change, it makes sense
>>> (sometimes) point to use the improved API's in existing code.
>>>
>>>
>>> It came to my attention that quite a bit of our code was doing
>>> (calloc/memcpy/free) just to resize an existing array.
>>>
>>> With re-calloc you can do...
>>> ---
>>> temp = (AviIndexEntry *) MEM_recallocN(movie->entries, (frame_num + 1)
>>> * entry_size);
>>> ---
>>>
>>> Compared to old code.
>>> ---
>>>      temp = (AviIndexEntry *) MEM_mallocN ((frame_num+1) *
>>>          (movie->header->Streams+1) * sizeof(AviIndexEntry),"newidxentry");
>>>      if (movie->entries != NULL) {
>>>              memcpy (temp, movie->entries, movie->index_entries *
>>> (movie->header->Streams+1)
>>>                  * sizeof(AviIndexEntry));
>>>              MEM_freeN (movie->entries);
>>>      }
>>> ---
>>> When looking into a bug report in areas of code like this you may want
>>> to double check the sizeof(), offsets, alloc-size etc is correct (off
>>> by one cause uninitialized memory of writing out of memory bounds), so
>>> even if its working its more code to wade through and check from time
>>> to time.
>>>
>>> Not to say going over and replacing all code is something we should
>>> do, but with an API that makes some common expression more concise - I
>>> believe it can be good to do so with care (and testing of course,
>>> which I did, just not adding from zero).
>>> Devs use existing code as a reference, so if we use our API's in a
>>> clear way I think it helps us in the long term with GSOC, patches -
>>> new modules.
>>>
>>> IMHO we should try to have our own code be an example of what we would
>>> like others to develop in blender.
>>>
>>> In practice its a compromise, some refactors/cleanups are just too
>>> disruptive, error-prone or time consuming.
>>>
>>> But I disagree with Ton's comment: "New code should be totally and
>>> undeniably better, for users. Not for coders."
>>>
>>> Coders have to read/debug blenders existing code a _lot_. - Reducing
>>> confusion is important and reduces errors we make when we do end up
>>> making functional changes to the code (really! - quite a few bugs in
>>> the tracker I found have been caused by devs who improve blender but
>>> miss checking for non obvious side-effects or just having trouble with
>>> confusingly written code).
>>>
>>> In the case of our particle code, I agree that we can just leave it
>>> and not try do more cleanup - just maintain until its re-written.
>>>
>>> On Sun, Jan 27, 2013 at 8:29 PM, Ton Roosendaal<ton at blender.org>  wrote:
>>>> Hi,
>>>>
>>>> You miss the point Jason. It's not about being stupid or not thinking enough.
>>>> It shouldn't even have come up in a coders mind to "fix" it.
>>>>
>>>> -Ton-
>>>>
>>>> ------------------------------------------------------------------------
>>>> Ton Roosendaal  Blender Foundationton at blender.org     www.blender.org
>>>> Blender Institute   Entrepotdok 57A  1018AD Amsterdam   The Netherlands
>>>>
>>>> On 26 Jan, 2013, at 22:02, Jason Wilkins wrote:
>>>>
>>>>> I think this would not have broken if MEM_reallocN was exactly like
>>>>> the standard library realloc.  The slight differences probably mislead
>>>>> whoever was making this change.
>>>>>
>>>>> On Sat, Jan 26, 2013 at 6:42 AM, Ton Roosendaal<ton at blender.org>  wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Here's another nice illustration of code cleanup that caused bugs;
>>>>>> http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-blender&revision=54058
>>>>>>
>>>>>> The idea of "replace calloc + memcpy with recalloc" sounds nice, but it breaks the case when memcpy is of length zero. (zero hairs, use 'add' brush in particle mode).
>>>>>>
>>>>>> May I quote Joelonsoftware here?
>>>>>>
>>>>>> "Old code has been used. It has been tested. Lots of bugs have been found, and they've been fixed. There's nothing wrong with it. It doesn't acquire bugs just by sitting around on your hard drive. Au contraire, baby!"
>>>>>>
>>>>>> Great fun read:
>>>>>> http://www.joelonsoftware.com/articles/fog0000000069.html
>>>>>>
>>>>>> Of course we should never be afraid to recode parts of Blender, but it shouldn't be with the naive assumption that the old bad messy code wasn't functional. New code should be totally and undeniably better, for users. Not for coders. :)
>>>>>>
>>>>>> We can moan and complain about our current bad particle code a lot, but it has offered a lot of good for Blender for many years. The challenge is not to make a new system with clean design, but to make a new system that's far superior.
>>>>>>
>>>>>> -Ton-
>>>>>>
>>>>>> ------------------------------------------------------------------------
>>>>>> Ton Roosendaal  Blender Foundationton at blender.org     www.blender.org
>>>>>> Blender Institute   Entrepotdok 57A  1018AD Amsterdam   The Netherlands
>>>>>>
>>>>>> _______________________________________________
>>>>>> Bf-committers mailing list
>>>>>> Bf-committers at blender.org
>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>>> _______________________________________________
>>>>> Bf-committers mailing list
>>>>> Bf-committers at blender.org
>>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>> _______________________________________________
>>>> Bf-committers mailing list
>>>> Bf-committers at blender.org
>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>
>>> --
>>> - Campbell
>>> _______________________________________________
>>> Bf-committers mailing list
>>> Bf-committers at blender.org
>>> http://lists.blender.org/mailman/listinfo/bf-committers
>> _______________________________________________
>> Bf-committers mailing list
>> Bf-committers at blender.org
>> http://lists.blender.org/mailman/listinfo/bf-committers
>>
>
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers



-- 
- Campbell


More information about the Bf-committers mailing list