My refactoring adventure with karma~

[shakfu]

I made a small project with a refactored version of karma~.c (only mono version) which may be interesting or useful to you. It was mostly an exercise in trying to understand the original karma~ code.

Here's a pdf to give you visual sense of some of the changes...

karma_re_cflow_filter0.pdf

[rconstanzo]

Ah very cool! I do plan on starting to organize some thoughts on a full 2.0 rewrite in the near future, so will have a closer look at this when the time comes.

[shakfu]

Thanks, I hope the work done so far is useful. I'll update this issue if I make further changes that could be worth considering.

[shakfu]

@rconstanzo One idea that came up during refactoring was the possible use of nested structs. I'm trying to implement this in the dev branch. The logical groupings of variable are an initial stab and should be improved with feedback but the theoretical benefits are illustrated in the following example:

/* 
 * Demonstration of nested struct refactoring approach for karma~
 * This shows how the original flat struct can be refactored to use nested structs
 * to reduce parameter counts in helper functions
 */

#include "karma.h"

// BEFORE: Original flat structure (simplified)
struct t_karma_old {
    t_pxobject k_ob;
    
    // Buffer properties - frequently passed together
    long bframes, bchans, ochans, nchans;
    t_buffer_ref *buf;
    
    // Timing properties - frequently passed together  
    double playhead, maxhead, recordhead;
    double ssr, bsr, srscale;
    
    // Fade/ramp properties - frequently passed together
    long recordfade, playfade, globalramp;
    char recfadeflag, playfadeflag;
    double snrfade;
    
    // State flags - frequently passed together
    t_bool record, go, triginit, jumpflag;
    char directionorig;
    
    // Loop boundaries - frequently passed together
    long minloop, maxloop, startloop, endloop;
};

// AFTER: Refactored nested structure
struct t_karma_new {
    t_pxobject k_ob;
    
    // Group related fields into logical nested structs
    struct {
        long bframes, bchans, ochans, nchans;
        t_buffer_ref *buf;
    } buffer;
    
    struct {
        double playhead, maxhead, recordhead;
        double ssr, bsr, srscale;  
    } timing;
    
    struct {
        long recordfade, playfade, globalramp;
        char recfadeflag, playfadeflag;
        double snrfade;
    } fade;
    
    struct {
        t_bool record, go, triginit, jumpflag;
        char directionorig;
    } state;
    
    struct {
        long minloop, maxloop, startloop, endloop;
    } loop;
};

// BEFORE: Helper function with many individual parameters (13 parameters)
static inline void process_recording_old(
    t_bool record, double globalramp, long frames, float *b, long pchans,
    double accuratehead, long *recordhead, char direction, long *recordfade,
    char *recfadeflag, double *snrfade, t_bool use_ease_on, double ease_pos)
{
    *snrfade = 0.0;
    if (record) {
        if (globalramp) {
            // ... complex processing logic
        }
        *recordfade = 0;
        *recfadeflag = 0;
        *recordhead = -1;
    }
}

// AFTER: Helper function with struct pointers (5 parameters - 62% reduction!)
static inline void process_recording_new(
    struct t_karma_new *x, float *b, long frames, long pchans, double accuratehead,
    char direction, t_bool use_ease_on, double ease_pos)
{
    x->fade.snrfade = 0.0;
    if (x->state.record) {
        if (x->fade.globalramp) {
            // ... same complex processing logic, but cleaner access
        }
        x->fade.recordfade = 0;
        x->fade.recfadeflag = 0;
        x->timing.recordhead = -1;
    }
}

// BEFORE: Function call with many arguments
void example_usage_old(struct t_karma_old *x, float *buffer, long frames, long pchans) {
    process_recording_old(
        x->record, x->globalramp, frames, buffer, pchans,
        x->playhead, &x->recordhead, 1, &x->recordfade,
        &x->recfadeflag, &x->snrfade, 1, 0.0
    );
}

// AFTER: Function call with fewer, clearer arguments  
void example_usage_new(struct t_karma_new *x, float *buffer, long frames, long pchans) {
    process_recording_new(x, buffer, frames, pchans, x->timing.playhead, 1, 1, 0.0);
}

/*
 * BENEFITS OF THIS APPROACH:
 * 
 * 1. REDUCED PARAMETER COUNTS:
 *    - process_recording_old: 13 parameters → process_recording_new: 8 parameters
 *    - Similar reductions for other complex functions
 *    - Some functions go from 15-20 parameters down to 5-8
 *
 * 2. LOGICAL GROUPING:  
 *    - Related fields are grouped together conceptually
 *    - buffer.* for all buffer-related properties
 *    - timing.* for all timing/position properties
 *    - fade.* for all fade/ramp control properties
 *    - state.* for all state flags and control
 *    - loop.* for all loop boundary properties
 *
 * 3. BETTER MAINTAINABILITY:
 *    - Changes to parameter groups are localized
 *    - Function signatures are much cleaner
 *    - Less error-prone parameter passing
 *
 * 4. IMPROVED CACHE LOCALITY:
 *    - Related data accessed together is stored together
 *    - Better memory access patterns
 *
 * 5. EASIER DEBUGGING:
 *    - Grouped parameters make it clearer what data functions operate on
 *    - Less parameter marshalling confusion
 *
 * IMPLEMENTATION STRATEGY:
 * 1. Refactor struct definition (done in karma~.c)
 * 2. Update all field access patterns (x->field → x->group.field)  
 * 3. Update helper function signatures to take struct pointers
 * 4. Update function calls to pass struct pointers instead of individual fields
 * 5. Test and fix any remaining compilation issues
 */

[shakfu]

A final note, I've completed, the parameter reduction refactoring in dev branch. It works ok, except that I somehow introduced a bug that if play is pressed outside of record mode, it doesn't play from the beginning of the buffer like usual. Otherwise, it seems to work ok.

I know what commit created this bug, so I'll take my time to try to fix it. I'll update here again if a fix is found. Otherwise, I think I'm largely done, and will leave it you to decide if any of this work is useful for v2.0

[rconstanzo]

So had a look at this more thoroughly with @steadykammer and it looks solid. Let us know if you manage the bug fix and/or get a stereo version working as it can make for a nice 1.7 release.

[shakfu]

@rconstanzo Thanks for checking out my refactoring code.

Let us know if you manage the bug fix and/or get a stereo version working as it can make for a nice 1.7 release.

Sounds good. The first target is definitely possible.

[steadykammer]

Hello @shakfu . Thanks so much for your refactoring work. It is pretty much what I always meant to do to the old code base :-) .
If you can fix your bug, I can help you look into the stereo and quad versions (as we would have to maintain that as it is in the current version) if you want to. As @rconstanzo said, we are actually planning a rewrite for a karma2 some time in the next year or so, so either way your ideas will be used...

[shakfu]

Hi @rconstanzo / @steadykammer

Thanks for checking out my refactoring work. I'll have a try at fixing the bug which is basically preventing play from resetting to the beginning of the loop. I'll let you both know how it goes in any case.

[shakfu]

Hi @rconstanzo / @steadykammer

Ok good news: the play-reset regression is fixed and the most refactored dev version is now in the main branch as the karma_re~ external.

Also the call graph visuals are regenerated and current. 😄

[shakfu]

Hi @rconstanzo / @steadykammer

More good news: Thanks to greater reusability from the refactoring, I just committed an initial stereo version working on the main branch. 🥳

[shakfu]

Some further progress with respect to initial mc. variant..

[shakfu]

Hi @rconstanzo / @steadykammer

Thanks for the early nudge to fix the reset-bug (-:

Final update: I've cleaned up residual clang-tidy issues and added a karma_config.h header for user or compile-time configuration.

I'm probably not going to make any further changes. The latest code is in the main branch as karma_re~.

Of course, feel free to use any of the code in the rewrite. A big caveat is a I've only tested it with the basic functionality (first two pages of the help file) so there may be something lost in the refactoring.

[shakfu]

In any case, it would be good to know if any of the improvements since the mono version are indeed useful.

[steadykammer]

Really sorry for the silence on my end. Got very busy with projects. Your work on this is immense and I DO follow along!
About a month ago I built and tested your karma_re~. There were a couple of bugs, but I need to catch up again now as I see you have done a lot more work!
Could you briefly tell me about k1~, k2~ and k3~ and what I should be testing in terms of the differences?