Jump to content

  • Log In with Google      Sign In   
  • Create Account

Banner advertising on our site currently available from just $5!


1. Learn about the promo. 2. Sign up for GDNet+. 3. Set up your advert!


Trying to reduce the massive amount of variables in my struct


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
12 replies to this topic

#1 aregee   Members   -  Reputation: 1037

Like
1Likes
Like

Posted 16 July 2014 - 09:42 AM

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?

 



Sponsor:

#2 Madhed   Crossbones+   -  Reputation: 3469

Like
2Likes
Like

Posted 16 July 2014 - 10:20 AM

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, 16 July 2014 - 10:26 AM.


#3 KnolanCross   Members   -  Reputation: 1540

Like
0Likes
Like

Posted 16 July 2014 - 11:43 AM

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, 16 July 2014 - 11:46 AM.

Currently working on a scene editor for ORX (http://orx-project.org), using kivy (http://kivy.org).


#4 aregee   Members   -  Reputation: 1037

Like
0Likes
Like

Posted 16 July 2014 - 12:22 PM

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.  :)



#5 ApochPiQ   Moderators   -  Reputation: 17450

Like
0Likes
Like

Posted 16 July 2014 - 12:45 PM

Are you using C or C++?

#6 aregee   Members   -  Reputation: 1037

Like
0Likes
Like

Posted 16 July 2014 - 12:48 PM

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, 16 July 2014 - 12:51 PM.


#7 aregee   Members   -  Reputation: 1037

Like
0Likes
Like

Posted 16 July 2014 - 01:18 PM

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.



#8 ApochPiQ   Moderators   -  Reputation: 17450

Like
1Likes
Like

Posted 16 July 2014 - 03:15 PM

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).

#9 KnolanCross   Members   -  Reputation: 1540

Like
0Likes
Like

Posted 16 July 2014 - 03:17 PM

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.


Currently working on a scene editor for ORX (http://orx-project.org), using kivy (http://kivy.org).


#10 ApochPiQ   Moderators   -  Reputation: 17450

Like
0Likes
Like

Posted 16 July 2014 - 03:25 PM

IIRC it is undefined behavior to write to a union field as one type and then read from it as another.

#11 SeanMiddleditch   Crossbones+   -  Reputation: 9902

Like
0Likes
Like

Posted 16 July 2014 - 06:59 PM

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?

Just process each effect in its own loop. Chain them together. You can then pipeline them over multiple threads or split them into non-overlapping chunks and run the effect pipeline over each chunk in its own thread.

There's no need for effects to know about each other. If there are some pairs of effects that do need to work together, make that a separate effect all on its own and morph the input data to use that special combined effect instead of the individual effects.

#12 aregee   Members   -  Reputation: 1037

Like
0Likes
Like

Posted 16 July 2014 - 07:19 PM

IIRC it is undefined behavior to write to a union field as one type and then read from it as another.

 

That is exactly what I was thinking about, but I am safe here, since that is not what I will be doing anyway.  I read something about differences in how different fields are padded on different compilers, which probably can be tested with lots of voodoo magic, but that kind of defies the whole usefulness of writing one type and reading another.  One example I saw was to test for endianness, an example that is actually void due to the undefined behaviour.  Edit: It is actually not void, when I think about it, because the array in the example I saw was the same size as the field that was used to test for endianness.

 

KnolanKross, in your example, there won't be any problems, because you are using the same underlying size of the data types.  Had you used uint16_t mixed with uint8_t, it would probably work, but there you *might* stumble across a compiler that pads the field differently, which would lead to undefined behaviour.

 

ApochPiQ, it took a little while to wrap my head around your example, but to be sure I understood:

 

Allocate the ParameterPackWhatever-type I need, cast it to the type ParameterPack, and assign it to the AdditionalParams field in ThingsWithParameters struct.  Since the first field in all the ParameterPackWhatever-structs are EParameterPackTypes, I can now safely check the type before accessing (and casting) to the correct struct type and fetch the fields I need from there?

 

Thank you all!  This is exactly the kind of answers I was hoping for.  You have been a great help, and I have learnt a lot of new stuff!  biggrin.png


Edited by aregee, 16 July 2014 - 07:25 PM.


#13 aregee   Members   -  Reputation: 1037

Like
0Likes
Like

Posted 16 July 2014 - 07:46 PM

 

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?

Just process each effect in its own loop. Chain them together. You can then pipeline them over multiple threads or split them into non-overlapping chunks and run the effect pipeline over each chunk in its own thread.

There's no need for effects to know about each other. If there are some pairs of effects that do need to work together, make that a separate effect all on its own and morph the input data to use that special combined effect instead of the individual effects.

 

 

I have a feeling that this is an interesting idea, but I am not quite sure I understand exactly what you mean. To me it sounds like you are suggesting to process several effects at once.  In that case, the music format is pretty primitive.  Only one effect is run at a time.  If a new effect comes on a channel, it breaks the other effect.  It is essentially a state machine, with exception of a few that might be combined with some volume adjustments that I will make a secondary effect field for.  BUT -- you gave me an idea I will have a look into: maybe process each channel on separate threads, but that probably depends on how much data I am processing for each request to fill the playback buffer.  I have chosen to have a relatively small buffer to reduce latency, and I am only using 1% of the cpu at all times, opposite of my FLAC part of the audio player that lies around 6%.

 

I have used threading in my app, though.  I have long had a problem with the beginning of the song not playing occasionally.  It plays, though, but with no sound.  I had no idea what caused it, and I was fairly sure it was not a bug from my side.  (Aren't we all? ;) )  I was going to make a post about it here to ask for help, when I wrote about that it, it came to me: it seemed like the OS introduced a lag in the system because of my hard drives were spinning up or something, causing a buffer underrun.  AudioQueues has a bad habit of playing silently for a while after being subjected to a severe buffer underrun situation.  But I have an SSD disk...  Then it came to me that I also have three USB disks connected, and I now had a hunch. 

 

To solve the problem, I tried to let the current song continue to play, while I was loading the next in the background.  If it took more than half a second, I would also display an indeterminate progress bar until the task told the player that it was ready to play the next song.  That solved my problem.  The indeterminate progress bar thing was also a really good psychological help in that you would see that the app was indeed working on something and not just stalling for no reason.  I just cancelled that post.

 

OS X has a bad habit of stalling the system on disk access when you have connected USB drives that need to spin up, even when you are not accessing the drives at all.

 

Edit: pardon my messy post, but it is bed time now and I am just rambling now.  :)


Edited by aregee, 16 July 2014 - 07:47 PM.





Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS