Hello.
I'm having a problem with this computer program. Suffice it to say that I have narrowed down the problem to the exact statement that seems to be malfunctioning. Why elementary C++ statements don't work the same every time is a mystery to me. Honestly, I'm pretty fed up with C++. Too many times I've written a computer program perfectly and by the book, only to find that there's some unfathomable, and subsequently crippling, kink in the source code. If you know of a programming language that isn't as syntactically strict or so demanding of hours of troubleshooting, please let me know because that is what I need to be learning.
Anyways, this program starts by asking for the number of output frames to produce (typing 1 will suffice for troubleshooting purposes). It then asks for the value of # in the file name LCinit#.ppm. This part only exists because I have more than one LCinit file that can be used (I've been using LCinit2.ppm for troubleshooting. This file is attached as a text file). The program output includes the number of frames that you asked to be produced. These frames should, in theory, be replicas of LCinit#.ppm. The program also outputs a file called debug.txt, which is composed of rows, that display the following information regarding LCinit#.ppm pixel upload, in the following format: [current_pixel] ([red_value]@[file_location],[green_value]@[file_location],[blue_value]@[file_location],) [color_category]
The problem is (and you can verify this in debug.txt) the line of code, for(i=0;i<pw*ph;i++) in the function void getinitial(), stops before the 3rd iteration (i=2) even though pw*ph is equal to 32. Why this occurs, I can not even imagine.
If you can help me figure this out and correct the problem, I would greatly appreciate it.
your coding style is horrible... but you could at least indent it properly, I'm not going to analyze it
win7 x86, VS 2008 & 2010, C++/CLI, C#, .NET 3.5 & 4.0, VB.NET, VBA... WPF is comming
remeber to give feedback you think my response deserves recognition? perhaps you may want to click the Rate this post link/button and add to my reputation
private lessons are not an option so please don't ask for help in private, I won't replay
if you use Opera and you'd like to have the tab-button functionality for the texteditor take a look at my Opera Tab-UserScirpt; and if you know how to stop firefox from jumping to the next control when you hit tab let me know
Too many times I've written a computer program perfectly and by the book, only to find that there's some unfathomable, and subsequently crippling, kink in the source code.
Check the errata for the book that you are using, or simply get another book if you are too fed up with the errors in the one that you are using.
Originally Posted by hixidom
If you know of a programming language that isn't as syntactically strict or so demanding of hours of troubleshooting, please let me know because that is what I need to be learning.
I do not think that C++ is a particularly good programming language in such a case, but if the instructional material is the problem, then switching to a different programming language instead of switching instructional material is silly.
Originally Posted by hixidom
Anyways, this program starts by asking for the number of output frames to produce (typing 1 will suffice for troubleshooting purposes). It then asks for the value of # in the file name LCinit#.ppm. This part only exists because I have more than one LCinit file that can be used (I've been using LCinit2.ppm for troubleshooting.
For the purposes of the smallest and simplest compilable program that demonstrates the problem, I suggest that you directly embed the input as a string in the source code. Make it such that the program can be built and run, and produces output with no user intervention needed. Then post the expected output and actual output so we can compare them with what we get when we build and run your sample program.
On a more general note, I strongly suggest that you avoid the use of global variables, and give your variables names that are more descriptive. I also suggest that you place each statement on its own line. Your indentation could be more consistent, and you might want to adopt a more common indent style, at least when posting code to a public forum.
Last edited by laserlight; July 4th, 2009 at 03:20 PM.
C + C++ Compiler: MinGW port of GCC
Build + Version Control System: SCons + Bazaar
The char array "file" is only 10 characters long. The word "LCinit.ppm" is 10 characters long. However, you try to concatenate *another* string (temp) of nonzero length in there as well---so you're definitely going to overflow those array bounds. You should be using a std::string rather than a char array if you don't know the final string length with certainty beforehand.
In fact, use of std::strings should always be preferred to char arrays in C++ unless you have a demonstrated performance issue which can only be addressed by using the more basic solution.
only to find that there's some unfathomable, and subsequently crippling, kink in the source code.
This will happen less often if you rely more on STL containers and algorithms (including std::string) rather than on "C-style" coding, which can be error-prone. Also, global variables almost always make a program harder to maintain & debug.
Also, whitespace is your friend. You don't have enough of it, which will always make your logic less clear.
Last edited by Lindley; July 4th, 2009 at 01:56 PM.
First of all, thank you Memeloo for replying just to let me know that you're too much of an asshole to help me. That's good to know.
Laserlight, your suggestion is a good one, but I don't think it will help me in this case. The thing is, the purpose of the for() statement in question is to read and "download" the input file so, while including the input file in the actual code would eliminate the problem, it would defeat the goal of my program, which is to perform a function on a given .ppm file.
Lindley, you're absolutely right, I don't know why I made file[] so small. Fixing that didn't help the problem at hand, but it was still worth addressing. Thanks for pointing that out.
Now that it's formatted so that we can actually see the control flow at a glance, it becomes obvious that you must be hitting one of those three break statements, which thus exits the outer for loop. Is that your intention? Whenever you use a break or continue statement, you should document what control structure you're trying to break from or continue in.
First of all, thank you Memeloo for replying just to let me know that you're too much of an asshole to help me. That's good to know.
I saw your post before he had even replied and spent about 5 minutes trying to decipher your code before deciding not to bother, and I'm sure I wasn't the only one. If you have any plans at all to work with other programmers or regularly ask for help, you need to learn to write code that's easier to read. Use indentation. Put spaces between variable names and operators and so on. Only have one statement per line. Etc, etc. And I'll promise you, if you make an effort to write cleaner code you'll find significantly easier to track down bugs and problems in your programs.
well Lindley, the code you posted is completed new to me, but if it works, then it's exactly what I'm looking for. I'll give it a try.
Speedo, you're right, I am very sloppy about my format. Even if I can muddle my way through it, I should at least clean it up a little bit before asking for any sort of help. With that said, I'll clean up the code a little bit and repost it much later tonight.
First of all, thank you Memeloo for replying just to let me know that you're too much of an asshole to help me. That's good to know.
You need to post clear code so that others could help you. It certainly is not formatted properly, so much so that others had to format it for you so that it is clear.
Second, have you used a debugger to see why it stops on the third iteration? Loops stop for a reason -- either you have issued a "break" statement, causing the loop to stop, or the loop condition in the for() has been reached (i.e. it is false) thereby automatically stopping the loop. Usage of a debugger, or even placing output statements to see what is being done, could have been done.
Third, your code has memory leaks. You use "new[]" without a single call to "delete[]". Instead of this:
Code:
a=new double[pw*ph];
b=new double[pw*ph];
and then never cleaning up this memory, this could be replaced with this:
Fourth, as others stated, you should be using C++ if it's a C++ program, i.e. std::string should be used in place of those strcpy(), strcat(), etc. If you get a buffer overrun by using these error-prone 'C' functions, you can potentially change the values of other variables accidentally (depending on where they are declared), which causes all sorts of weird things to happen.
Lindley already pointed out one such overrun, and I wouldn't be the least bit shocked if that is the cause of the problem you're having now with loops terminating before you ecpected them to terminate (not the most likely cause, but buffer overruns do cause these types of issues).
Well, here's the revised/improved code. The reason the for() statement was stopping on the third iteration was a few break commands that shouldn't have been there (go figure). The program is working fairly well now, despite the fact that it doesn't work with any .ppm file larger than 50 x 50 pixels. Does anybody know why this is the case?
What if file happens to be more than 15 characters? What's stopping the user from typing in 100 characters? Nothing. That's why you use C++ std::string instead of code like the above.
And if frame is more than 17 characters, what then?
How do we know you do not have a buffer overrun, causing all sorts of undefined behaviour to occur? How do you know you don't have a buffer overrun? (So far, you haven't indicated if you've actually used a debugger to confirm anything).
Also, you do this:
Code:
if(z=0 && y>0)
This sets z equal to 0. It should be this:
Code:
if(z == 0 && y>0)
You make this mistake in various other places.
Another issue:
Code:
void cleartemp(char tempn[20])
{
int a;
for(a=0; a<20; a++)temp[a]='\0';
}
That parameter does not make tempn an array of 20 characters. That parameter is equivalent to this:
Code:
void cleartemp(char* tempn)
Secondly, you have temp and not tempn as being set, therefore tempn is not even being used in your function.
And lastly, are your files binary files, where they could contain control characters and embedded NULLs? If so, you are not opening them in binary mode (ios::binary).
Regards,
Paul McKenzie
Last edited by Paul McKenzie; July 5th, 2009 at 08:56 PM.
Honestly, I'm pretty fed up with C++. Too many times I've written a computer program perfectly and by the book, only to find that there's some unfathomable, and subsequently crippling, kink in the source code.
If the program were perfect, there wouldn't be any problems. Needless to say, the code you posted is far from perfect, as I see glaring errors.
But to your point -- I see no C++ in the code that you wrote. What I do see is old-style 'C' coding, not C++. The coding style is almost all 'C' -- a similar C++ program using proper containers, strings, algorithms etc. would look nothing like what you've coded. It would be around half the size in terms of number of lines of code, and more maintainable.
If you learned proper C++ coding style and technique instead of picking up source code meant for 'C' programs and programmers, you would see that C++ isn't so difficult or need troubleshooting as you've described.
If you know of a programming language that isn't as syntactically strict or so demanding of hours of troubleshooting, please let me know because that is what I need to be learning.
Then pick up "Accelerated C++" by Koenig & Moo, and not 'C' books to learn from.
Regards,
Paul McKenzie
Last edited by Paul McKenzie; July 5th, 2009 at 09:02 PM.
Well, let's see about moving things in the C++ direction then.
First, let's eliminate the global variables. It's well-known that global variables add an enormous amount of complexity to ensuring correctness in a program for very little practical gain. Each and every variable used in a function should fit into one of three categories:
1) Input
2) Output
3) Temporary
Unless performance is one of your key goals (very rarely the case in practice), there's never any reason to use a global for category #3. These should always be defined locally.
Values in category #1 should always be function parameters; either passed by value or by const reference. Never allow the behavior of a function to differ based on the current state of a global when it is called----that way lies unmaintainable code! A function's behavior should depend *only* on the parameters passed to it.
For category #2, if you've only got one output then just make it the return value. If you need more, use reference or pointer parameters to serve as outputs.
Now, moving on. In processpix, you have a massive list of case statements, each of which appears to be pretty much identical except for the index into the a array used. Another rule to good code: Minimize repeated code! You want as much as possible to exist is one and only one place, so that once it's working you can re-use it later and not have to debug again. In the case of processpix(), you should strongly consider declaring a variable like "aind" or something which is set based on the value of i or whatever, and then move your massive equation out so that it only needs to exist in one place. Also, document what the hell it's doing a bit better, because I have no idea from looking at it.
This function doesn't use its parameter, and it *does* use two values which are *not* parameters. This is bound to get you in trouble. More importantly, though, it suggests that you may want to introduce a mathematical vector class. The more logic you can move out of your general code and into generic, re-usable components the better---and something like computing the magnitude of a 2D vector with y and z components fits that bill perfectly.
There's a fairly decent (though flawed in a few ways) point/vector class here: http://softsurfer.com/Archive/algori...rithm_0301.htm
which I've found useful. I guarantee that once you start thinking in terms of objects rather than individual values, you'll have an easier time of it.
Code:
void cleartemp(char tempn[20])
{
int a;
for(a=0; a<20; a++)temp[a]='\0';
}
There's no need for this function for two reasons:
1) If you used std::strings rather than char arrays, you could just use their .clear() method.
2) Even if you did want to use a char array, simply doing "arr[0] = '\0'" is sufficient for all the C standard library string functions----they only care where the *first* null byte is. There's no need to null out the entire thing. But if you wanted to anyway, the memset() and std::fill() functions are available for that purpose, so you *still* don't need to write your own.
where the last line is commented because you may not actually *need* it in many cases, but it's included just to make the function semantically identical to what you have now. Note the difference between the mathematical vectors I mentioned above and these container vectors----same word, difference concepts.
Last edited by Lindley; July 5th, 2009 at 09:49 PM.
Well, I made a few changes. I got rid of a few global variables, I simplified processpix(), I added explanatory notes (which might be written in the wrong format, I'm not sure). I've attached a very small animation that demonstrates the goal of this program, as well as the LCinit1.ppm file that was used to make it. I want to be able to use bigger LCinit#.ppm files but, for some reason, the program fails with any file larger than 50x50 pixels. I tried using the debugging tool on DevC++, but it didn't get me very far since I didn't really know what I was doing. I still havn't implemented vectors, but only because I don't know anything about how to use them. I'm fairly satisfied with the program as is. I know that those of you who are much more experienced in programming would strongly disagree, but I feel as though the program is almost finished. My main concern is the apparent limitation to image size, but I have no idea why this is occurring.
Code:
#include<iostream>
#include<math.h>
#include<fstream>
#include<stdlib.h>
#include<cstring>
#define pi 3.14159265
using namespace std;
void cpt(int n)
{
cout<<" Checkpoint "<<n<<" ";
system("PAUSE");
}
int pw,ph,res,base[8]={520,540,560,580,600,620,640,660};
double* a=NULL;
double* b=NULL;
char temp[20];
double tempc[3];
void putbina() //puts the contents of b[] in a[] and clears b[]
{
int i;
for(i=0;i<pw*ph;i++)
{
a[i]=b[i];
b[i]=0;
}
}
double anglecalc(double current, double v[2]) //converts 2D flow vector to angle in radians
{
if(v[0]==0 && v[1]>0)return (pi/2);
else if(v[0]==0 && v[1]<0)return (-1*pi/2);
else if(v[0]>0)return atan(v[1]/v[0]);
else if(v[0]<0)return pi+atan(v[1]/v[0]);
else return 0;
}
double magcalc(double v[2]) //finds the magnitude of flow vector at current pixel
{
return sqrt(pow(v[0],2)+pow(v[1],2));
}
void processpix(int current)
{
double theta,v[2];
//set x and y components based on current pixel
v[1]=((current-fmod(current,pw))/pw)+1;
v[0]=fmod(current,pw)+1;
//define flow functions f(x) and f(y)
v[1]=(-1*v[1])+30;
v[0]=v[0]*2;
int i,offset[11]={1,1-pw,-pw,-1-pw,-1,pw-1,pw,pw+1,1,(1-ph)*pw,(ph-1)*pw}; //offset[] represents locations of adjascent pixels
theta=anglecalc(current,v)-(pi/4);
if(theta<0)theta=theta+(2*pi); //rules out negative angles
i=(int)(theta/(pi/4)); //i is the octant (adjascent pixel) containing the angle theta
//define i values for pixels on the edge of the image
if(current+1<pw && sin(theta)>0)i=10;
else if(current+1>(ph-1)*pw && sin(theta)<0)i=9;
else if(fmod(current,pw)==0 && cos(theta)<0)i=5;
else if(fmod(current+1,pw)==0 && cos(theta)>0)i=1;
if(current+offset[i]>pw*ph || current+offset[i+1]>pw*ph)i=0; //makes sure current+offset does not exceed the number of pixels
b[current]=(a[current]+(((fmod(theta,pi/4)/(pi/4))*a[current+offset[i]])+((1-(fmod(theta,pi/4)/(pi/4)))*a[current+offset[i+1]]))*magcalc(v))/magcalc(v); //defines b[current] as a weighted combination of a[current] and its adjascent pixels
}
void cleartemp(char temp[20]) //used to clear char array[20]
{
int a;
for(a=0; a<20; a++)temp[a]='\0';
}
void getinitial()
{
char file[15]={"LCinit"};
cout<<"File to upload: LCinit#.ppm\n# = "; //file to be used must be of the given format, where # is less than 3 digits
cin>>temp;
//assemble the char array used to access file
strcat(file,temp);
cleartemp(temp);
strcat(file,".ppm");
ifstream lcinit(file, std::ios_base::ate);
//put all of lcinit into the string tempfile (for easier access)
int strplace[2], filesize=lcinit.tellg();
string tempfile;
tempfile.resize(filesize);
lcinit.seekg(0);
for(strplace[0]=0; strplace[0]<filesize-100; strplace[0]++)
{
tempfile[strplace[0]]=lcinit.get();
}
lcinit.close();
//download the pixel width, pixel height, and color resolution of LCinit into pw, ph, and res
strplace[0]=strplace[1]=0;
cleartemp(temp);
strplace[0]=tempfile.find("\n",10,1);
strplace[0]=strplace[0]+1;
strplace[1]=tempfile.find(" ",strplace[0],1);
tempfile.copy(temp,strplace[1]-strplace[0],strplace[0]);
pw=atoi(temp);
cleartemp(temp);
strplace[1]++;
strplace[0]=tempfile.find("\n",strplace[1],1);
tempfile.copy(temp,strplace[0]-strplace[1],strplace[1]);
ph=atoi(temp);
cleartemp(temp);
strplace[0]++;
strplace[1]=tempfile.find("\n",strplace[0]);
tempfile.copy(temp,strplace[1]-strplace[0],strplace[0]);
res=atoi(temp);
cleartemp(temp);
//define a and b, which will index all pixel color values
a=new double[pw*ph];
b=new double[pw*ph];
int i,j;
strplace[0]=strplace[1]+1;
ofstream debug ("debug.txt", ios::ate | ios::trunc);
//download pixel color values into a, converting from RGB to spectrum color format
for(i=0;i<pw*ph;i++)
{
debug<<i<<"\t(";tempc[0]=tempc[1]=tempc[2]=0;
for(j=0;j<3;j++)
{
strplace[1]=tempfile.find(" ",strplace[0]);
tempfile.copy(temp,strplace[1]-strplace[0],strplace[0]);
tempc[j]=atoi(temp);
debug<<tempc[j]<<",";
cleartemp(temp);
strplace[0]=strplace[1]+1;
}
debug<<")\t";
if(tempc[0]==tempc[1] && tempc[0]==tempc[2] && tempc[1]==tempc[2]) //if R==G==B...
{
debug<<0;a[i]=427;
}
if(tempc[0]>=tempc[1] && tempc[0]>tempc[2]) //if red is the main RGB color...
{
debug<<1;
tempc[1]=tempc[1]*(res/tempc[0]);
tempc[2]=tempc[2]*(res/tempc[0]);
tempc[0]=res;
a[i]=((tempc[0]*428)+(tempc[1]*566))/(tempc[0]+tempc[1]);
}
if(tempc[1]>=tempc[2] && tempc[1]>tempc[0]) //if green is the main RGB color...
{
debug<<2;
tempc[0]=tempc[0]*(res/tempc[1]);
tempc[2]=tempc[2]*(res/tempc[1]);
tempc[1]=res;
a[i]=((tempc[0]*428)+(tempc[1]*566)+(tempc[2]*638))/(tempc[0]+tempc[1]+tempc[2]);
}
if(tempc[2]>=tempc[0] && tempc[2]>tempc[1]) //if blue is the main RGB color...
{
debug<<3;
tempc[0]=tempc[0]*(res/tempc[2]);
tempc[1]=tempc[1]*(res/tempc[2]);
tempc[2]=res;
a[i]=((tempc[0]*714)+(tempc[1]*566)+(tempc[2]*638))/(tempc[0]+tempc[1]+tempc[2]);
}
debug<<"\t"<<a[i]<<"\n";
}
debug.close();
}
void pixrgb(double d)//converts spectrum format to RGB
{
tempc[0]=tempc[1]=tempc[2]=0;
if(d<428)tempc[0]=tempc[1]=tempc[2]=0;
else if(d>=428 && d<=566)
{
tempc[0]=(int)((566-d)*res/138);
tempc[1]=(int)((d-428)*res/138);
tempc[2]=0;
}
else if(d>566 && d<=638)
{
tempc[0]=0;
tempc[1]=(int)((638-d)*res/72);
tempc[2]=(int)((d-566)*res/72);
}
else
{
tempc[0]=(int)((d-638)*res/152);
tempc[1]=0;
tempc[2]=(int)((714-d)*res/76);
}
}
int main()
{
int x,i,j,framequant;
cout<<"Number of Frames: ";
cin>>framequant;
getinitial();
for(x=0;x<framequant;x++)
{
//name the given frame accordingly
char frame[17]={"PLCS1-1F"};
if(x<100)strcat(frame,"0");
if(x<10)strcat(frame,"00");
itoa(x+1,temp,10);
strcat(frame,temp);
cleartemp(temp);
strcat(frame,".ppm");
ofstream ofile (frame, ios::trunc | ios::ate);
ofile<<"P3# "<<pw<<" "<<ph<<" "<<res<<" \n"; //add necessary file parameters
for(i=0;i<pw*ph;i++)
{
processpix(i); //calculate b[i] (new pixel) from a[i] (original pixel)
pixrgb(b[i]); //convert b[i] back to RGB
for(j=0;j<3;j++) //put modified pixel i back into the new frame
{
itoa((int)tempc[j],temp,10);
ofile<<temp<<" ";
cleartemp(temp);
}
}
ofile.close();
putbina();
}
return 1;
}
* The Best Reasons to Target Windows 8
Learn some of the best reasons why you should seriously consider bringing your Android mobile development expertise to bear on the Windows 8 platform.
Reply With Quote
you think my response deserves recognition? perhaps you may want to click the Rate this post link/button and add to my reputation 
Originally Posted by hixidom
