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
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?
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.
at this point I also have to say thank you, I've learnt that I won't do the same mistake again and replay to someone who does not respect others and pastes such a chaos and expects that I spend my time on 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
Ok, this is what I've got so far. I'd like to point out that there are roughly half as many lines in this version of the program as there were in the original (136 vs. 267). I couldn't have made so many changes without everyones help. Thanks everybody. Honestly, I don't like the effect that the program is producing, but that's just something I'll have to fiddle with until it looks good enough.
Is this satisfying, or are there still changes to be made?
Code:
#include<iostream>
#include<math.h>
#include<fstream>
#include<stdlib.h>
#include<cstring>
#include<vector>
#define pi 3.14159265
using namespace std;
void cpt(int n)
{
cout<<" Checkpoint "<<n<<" ";
system("PAUSE");
}
int pw,ph,res;
vector<int> a;
vector<int> b;
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));
}
double precset(double num, int dec) //rounds num down such that it only has dec decimal places
{
return floor(num*pow(10,dec))/pow(10,dec);
}
void processpix()
{
double theta,f[2];
int current,i,adj[2][2],v[2]; //adj[] represents offset locations of two adjascent pixels in the direction of f(t)
for(current=0;current<pw*ph;current++)
{
//set x and y components based on current pixel
v[1]=(int)((current-fmod(current,pw))/pw);
v[0]=(int)fmod(current,pw);
//define flow functions f(x) and f(y)
f[0]=.707;
f[1]=.707;
theta=anglecalc(current,f);
if(theta<0)theta=theta+(2*pi); //rules out negative angles
//determine adjascent pixels to be considered
adj[0][0]=pw+(int)(v[0]+sqrt(pow(cos(theta),2))/cos(theta)-(((int)(theta/(pi/2))%2)*sqrt(pow(cos(theta),2))*(((int)(theta/(pi/4))+1)%2)/cos(theta)));
adj[0][1]=ph+(int)(v[1]+sqrt(pow(sin(theta),2))/sin(theta)-((((int)(theta/(pi/2))+1)%2)*sqrt(pow(sin(theta),2))*(((int)(theta/(pi/4))+1)%2)/sin(theta)));
adj[1][0]=pw+(int)(v[0]+sqrt(pow(cos(theta+(pi/4)),2))/cos(theta+(pi/4))-(((int)(theta/(pi/2))%2)*sqrt(pow(cos(theta+(pi/4)),2))*(((int)((theta+(pi/4))/(pi/4))+1)%2)/cos(theta+(pi/4))));
adj[1][1]=ph+(int)(v[1]+sqrt(pow(sin(theta+(pi/4)),2))/sin(theta+(pi/4))-((((int)(theta/(pi/2))+1)%2)*sqrt(pow(sin(theta+(pi/4)),2))*(((int)((theta+(pi/4))/(pi/4))+1)%2)/sin(theta+(pi/4))));
//apply modular algorithm to pixels outside the range of the image
while(adj[0][0]>pw-1)adj[0][0]=adj[0][0]-pw;
while(adj[0][1]>ph-1)adj[0][1]=adj[0][1]-ph;//this part might have a few bugs
while(adj[1][0]>pw-1)adj[1][0]=adj[1][0]-pw;
while(adj[1][1]>ph-1)adj[1][1]=adj[1][1]-ph;
//assign the location of a given pixel
v[0]=(adj[0][1]*pw)+adj[0][0];
v[1]=(adj[1][1]*pw)+adj[1][0];
for(i=0;i<3;i++)
{
b.push_back((a[(current*3)+i]+((precset(fmod(theta,pi/4)/(pi/4),2)*a[(v[0]*3)+i])+((1-precset(fmod(theta,pi/4)/(pi/4),2))*a[(v[1]*3)+i]))*magcalc(f))/magcalc(f)); //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 temp[20],file[15];
int i,j,tempc;
cout<<"File to upload: LCinit#.ppm\n# = "; //file to be used must be of the given format, where # is less than 3 digits
cin>>i;
//assemble the char array used to access file
sprintf(file,"LCinit%d.ppm",i);
ifstream lcinit(file, ios::binary);
lcinit.ignore(100,'5');
lcinit.seekg(+1,ios::cur);
cleartemp(temp);
//lcinit.seekg(0,ios::cur);
lcinit>>temp;
pw=atoi(temp);
cleartemp(temp);
lcinit>>temp;
ph=atoi(temp);
cleartemp(temp);
lcinit>>temp;
res=atoi(temp);
//download pixel color values into a
for(i=0; i<pw*ph; i++)
{
for(j=0;j<3;j++)
{
lcinit>>tempc;
a.push_back(tempc);
}
}
}
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];
sprintf(frame,"PLCS1-3%03d.ppm",x+1);
ofstream ofile (frame, ios::trunc | ios::ate);
ofile<<"P3# "<<pw<<" "<<ph<<" "<<res<<" \n"; //add necessary file parameters
processpix();
for(i=0;i<pw*ph*3;i++)
{
ofile<<b[i]<<" ";
}
ofile.close();
a.swap(b);
b.erase(b.begin(),b.end());
}
return 1;
}
Last edited by hixidom; July 13th, 2009 at 08:28 PM.
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
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.
A Developer.com Site
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
