Jump to content
  • Advertisement
Sign in to follow this  
aregee

Trying to reduce the massive amount of variables in my struct

This topic is 1467 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I am adding music formats to my audio player.  This time I am adding support for the ancient Amiga module format that was used in a lot of music trackers under lots of various names.  While I have not added every effect used by the "format" yet, it is playing my old music surprisingly well.

 

My question is about my structs.

 

I have organised the module (the song) in structs in a similar way to this:

typedef struct AmigaModule {
    uint8_t *ModuleName;
    uint8_t SampleCount;
    ModuleSample *SampleList;
    uint8_t SongPositions;
    uint8_t RestartPosition;
    uint8_t PatternTable[128];
    uint8_t MaxPattern;
    uint8_t ChannelCount;
    ModuleChannel *ChannelList;
} AmigaModule;

Where ModuleSample is a struct for each sample, and ModuleChannel is a struct for each channel in the song, with more structs to describe the pattern data for each channel, but this is not really interesting for my question.

 

ModuleChannel that has grown massively big with lots of arbitrary variables for each effect that tracks state of the song while it is playing, and I have made an effort in refactoring my code to a better structure.  Now I have removed all the state variables in its own struct to track states.

 

It looks something like this:

typedef enum ModuleChannelEffect {
    EFFECT_NONE,
    EFFECT_ARPEGGIO,
    EFFECT_PORTAMENTO,
    EFFECT_VIBRATO,
    EFFECT_TREMOLO,
    EFFECT_VOLUME_SLIDE,
    EFFECT_RETRIGGER_SAMPLE
} ModuleChannelEffect;

typedef enum ModuleChannelEffectSpecifier {
    EFFECT_SPECIFIER_NONE,
    EFFECT_SPECIFIER_PORTAMENTO_TO_NOTE,
    EFFECT_SPECIFIER_PORTAMENTO_UP,
    EFFECT_SPECIFIER_PORTAMENTO_DOWN,
    EFFECT_SPECIFIER_VOLUME_SLIDE_UP,
    EFFECT_SPECIFIER_VOLUME_SLIDE_DOWN
    //And more coming later...
} ModuleChannelEffectSpecifier;

typedef struct ModuleEffect {
    ModuleChannelEffect primaryEffect;
    ModuleChannelEffectSpecifier primaryEffectSpecifier;
    uint16_t parameter1;
    uint16_t parameter2;
    uint16_t parameter3;
} ModuleEffect;

And here is my issue:

See the three lines 'parameter1', 'parameter2', 'parameter3'?

 

In code, it looks like this:

currentChannel->effect.parameter1 = periodsTable[translatedPeriod];
currentChannel->effect.parameter2 = periodsTable[arpNoteIndex1];
currentChannel->effect.parameter3 = periodsTable[arpNoteIndex2];

And when I use those values:

currentChannel->period = currentChannel->effect.parameter1;

I like two things:

 

1. Code that is self-explanatory.

2. Generalisation, as long as it is not over-generalisation.

 

My problem is that 'parameter1' doesn't tell me anything, really.  In the portamento effect, is 'parameter1' the speed or the note I am sliding towards?  I could name the values for what they are, but then I wouldn't solve the problem with too many variables inside the struct again, because I would need to make lots of variables I don't always need to use.

 

Then it came to me that I could do something like this:

#define ArpeggioValue1 parameter1
#define ArpeggioValue2 parameter2
#define ArpeggioValue3 parameter3

Now I can reference the variable names in a more sensible and descriptive way:

currentChannel->effect.ArpeggioValue1 = periodsTable[translatedPeriod];
currentChannel->effect.ArpeggioValue2 = periodsTable[arpNoteIndex1];
currentChannel->effect.ArpeggioValue3 = periodsTable[arpNoteIndex2];
And:
 
currentChannel->period = currentChannel->effect.ArpeggioValue1;

My question is whether this is an acceptable coding practise, is there a better way of doing this?  Do you have any other suggestion?  The point here is really that I want to learn to get better.  What I am doing now is working, but how would you go about it?  Make massive objects to handle this?  Some other thing?  What is the "best" way?

 

Share this post


Link to post
Share on other sites
Advertisement

Have you thought about using unions, possibly anonymous unions?

Depends on which compiler you are using but I could imagine something like this:

typedef struct ModuleEffect {
    ModuleChannelEffect primaryEffect;
    ModuleChannelEffectSpecifier primaryEffectSpecifier;
    union {
        uint16_t note;
        uint16_t speed;
        uint16_t depth;
        uint16_t something_else;
        uint16_t and_another;
    };
    // ...
} ModuleEffect;


Also I don't think the original format with parameter1, 2, 3 is too bad. After all that's how the file format is laid out. How many hours are you going to spend editing these in code.

Edited by Madhed

Share this post


Link to post
Share on other sites

You have two options, first is to create one scruct for each effect, so you can properly name them.

 

The second is, as Madhed suggested, use anonymous unions, but be very careful as you should only use one of them. You could also use named unions, them you will be explicit about which variable you are changing (coding wise, this will be the same as the first option, but it will be more memory efficient, but prone to override errors).

Edited by KnolanCross

Share this post


Link to post
Share on other sites

Ah brilliant!  Two very good answers. I never had a look into unions before, I had even forgotten that they exists.  This is exactly what I need.

 

I also like the idea of creating one struct for each effect, but how do I combine them in a "polymorphic" way?  Using unions again?

 

 

Also I don't think the original format with parameter1, 2, 3 is too bad. After all that's how the file format is laid out. How many hours are you going to spend editing these in code.

 

I am not sure I understand the question, but if you mean how many hours I am going to spend making the module player, I have spent about 3 days and it is almost done.  It does play most of what I throw at it, with exception of a few files that the file size does not quite match the position where the loader thinks it should be, and where certain data is illegal or weird values.  But that is normal.  I just need to investigate the cases that I find, and I know there are bugs in a lot of the editors that were used to make these songs, plus some people added custom markers to sync parts of their demos with the music, etc.  I have written everything from scratch, so it is my own code that I am refactoring.  Why I would do that when there are tons of other players out there?  Just because it is fun. ;)  I am listening to the music that it is playing right as we speak.  :)

Share this post


Link to post
Share on other sites

Actually it is Objective-C, but most of the code I am writing in this part is C.  I have not set the project file to support C++.

 

Edit:

I am also considering moving fully to C++ instead of using Objective-C, especially now that Apple is moving towards Swift.  Nothing wrong with that, but Objective-C is at least semi-portable.  Swift is far away from that, and I have a feeling that is the point...

Edited by aregee

Share this post


Link to post
Share on other sites

Reading about unions makes me a bit uncertain in regards to the undefined behaviour issues, but it seems that for my use, where I am always reading the last value I sat, I should be good.

Share this post


Link to post
Share on other sites
Not sure offhand if Objective-C has a more idiomatic solution to this, but here's what I might do in C:

typedef enum {
    PARAMETER_PACK_TYPE_FOO,
    PARAMETER_PACK_TYPE_BAR,
} EParameterPackTypes;

typedef struct {
    EParameterPackTypes PackType;
} ParameterPack;

typedef struct {
    EParameterPackTypes PackType;
    int Something;
    unsigned SomethingElse;
} ParameterPackFoo;

typedef struct {
    EParameterPackTypes PackType;
    float Stuff;
} ParameterPackBar;

typedef struct {
    int CommonFieldOne;
    float CommonFieldTwo;
    ParameterPack * AdditionalParams;
} ThingWithParameters;
Then you can allocate a ParameterPackWhatever to hold the data you need. Since all ParameterPack* structs begin with the enum to tell you their type, you can assert that the package of parameters is correct for the effect (or whatever else).

This is as close to polymorphic objects as C gets (sans lots of horrible magic).

Share this post


Link to post
Share on other sites

Reading about unions makes me a bit uncertain in regards to the undefined behaviour issues, but it seems that for my use, where I am always reading the last value I sat, I should be good.

 

There is no "undefined" behavior, is just that you can run into non intuitive errors.

 

For instance, if you use something like this:

struct example_st {
    union {
        struct {
            int volume;
            int pitch;
        };
        struct{
            int atenuation;
            int something;
        };
    };
};

Then you code:

    struct example_st ex;

    ex.volume = 1;
    ex.pitch = 10;

If at some other point of the code you access ex.atenuation, it will have the value of 1. If you change the atenuation value, it will affect the value of volume as well. Also notice that at this example, structs where perfectly aligned, if they were not (specially if there are some floating point variable there), you could run into some really strange values.

Share this post


Link to post
Share on other sites
IIRC it is undefined behavior to write to a union field as one type and then read from it as another.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!