Sign in to follow this  

Error on pointer math

This topic is 4036 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 have a crash on this code and can't figure out the math for the pointer offsets.
float *ptr;
Noise3DTexPtr = (float*) malloc(NOISE3DTEXSIZE * NOISE3DTEXSIZE * NOISE3DTEXSIZE * 4);

for (f = 0, inc = 0; f < numOctaves; ++f, frequency *= 2, ++inc, amp *= 0.5)
{
	SetNoiseFrequency(frequency);
	ptr = Noise3DTexPtr;
	ni[0] = ni[1] = ni[2] = 0;

	inci = 1.0 / (NOISE3DTEXSIZE / frequency);
	for (i = 0; i < NOISE3DTEXSIZE; ++i, ni[0] += inci)
	{
	    incj = 1.0 / (NOISE3DTEXSIZE / frequency);
		for (j = 0; j < NOISE3DTEXSIZE; ++j, ni[1] += incj)
		{
			inck = 1.0 / (NOISE3DTEXSIZE / frequency);
			for (k = 0; k < NOISE3DTEXSIZE; ++k, ni[2] += inck, ptr += 4)
				//error on the line below is where it crashes 
*(ptr + inc) = float((((noise3(ni) + 1.0) * amp) * 128.0));

			}
		}
	}






Share this post


Link to post
Share on other sites
just to make sure we're on the same page. The array you have malloc'd appears to be a 3-dimensional array of something that is 4 bytes in size. Is this what you intended?

If that's the case, an x,y,z iteration would look like this:


for ( int i = 0; i < NOISE3DTEXSIZE; ++i )
{
for ( int j = 0; j < NOISE3DTEXSIZE; ++j )
{
for ( int k = 0; k < NOISE3DTEXSIZE; ++k )
{
//i think this is correct...
int offset = (i * NOISE3DTEXSIZE * NOISE3DTEXSIZE) + (j * NOISE3DTEXSIZE) + k;
Noise3DTexPtr *cur = Noise3DTexPtr[offset];
}
}
}






-me

Share this post


Link to post
Share on other sites
This is for making a noise texture and was using unsigned byte for types but now I want to use floats and figured I only need to change the pointers and get rid of the type cast to unsigned char and leave it as a float. But I am not following the math and thinking I am not allocating enough memory so sizeof(float) * noise3dtexsize * noise3dtexsize * noise3dtextsize * 4 is probably correct now that I think about it...

Share this post


Link to post
Share on other sites
Quote:
Original post by MARS_999
float(/* stuff */)

As far as I'm aware, that ain't valid C. Which begs the question which language are you using and why aren't you using it to its full potential?

Σnigma

Share this post


Link to post
Share on other sites
Quote:
Original post by Enigma
Quote:
Original post by MARS_999
float(/* stuff */)

As far as I'm aware, that ain't valid C. Which begs the question which language are you using and why aren't you using it to its full potential?

Σnigma


It actually is, it's the same as (float)/*some stuff*/.

Share this post


Link to post
Share on other sites
In C++ it is. I can't find any reference to it being valid in any standard dialect of C though, and it doesn't compile as C under any of my compilers. Either the OP wants to be coding in C and thus that line is an error or he is coding in C++ and there are far superior ways of coding what he wants that using malloc (Boost.MultiArray or at least a std::vector implementation with bounds checking in debug spring to mind).

EDIT: That was in reply to TrueTom

Σnigma

Share this post


Link to post
Share on other sites
Not too familiar with noise functions, but the first thing I thought of is: Why are you doing all preincrements rather than postincrements? (This might cause unexpected behaviour in your loops, i.e. never starting at zero and fooling the less-than conditionals.)

Share this post


Link to post
Share on other sites
Quote:
Original post by spartanx
Why are you doing all preincrements rather than postincrements? (This might cause unexpected behaviour in your loops, i.e. never starting at zero and fooling the less-than conditionals.)

No, it does not.

for (int i=0; i<10; ++i)

Will count from 0 to 9.

Share this post


Link to post
Share on other sites
Quote:
Original post by spartanx
Not too familiar with noise functions, but the first thing I thought of is: Why are you doing all preincrements rather than postincrements? (This might cause unexpected behaviour in your loops, i.e. never starting at zero and fooling the less-than conditionals.)

A for loop of the form

for (initialisation; condition; expression) statement;

is equivalent to:
{
initialisation;
while (condition)
{
statement;
expression;
}
}
Since expression is executed as a separate statement at the end of the loop preincrement and postincrement will have identical effect (although in C++ preincrement may be more efficient for user-defined types). There will be no "unexpected behaviour" or "fooling the less-than conditionals".

Σnigma

Share this post


Link to post
Share on other sites
Yes ++x is faster and that's why I am using it.

Now
for(int x = 0; x < 10; ++x)

is still going to 0-9 because ++x isn't incremented until the first loop is finished and evaluated after the loop.

Share this post


Link to post
Share on other sites
Mars, if you are using C++, why are you using malloc? Use new instead. In C, don't cast malloc.
I haven't checked the actual pointer arithmetic yet, just thought I'd point this out.
Run through the code by hand checking what exactly you're allocating and what you are adding as your offset vs what you should be adding...

EDIT:
What is the magic number `4'? Where are you getting it from? If you are assuming sizeof(float) == 4, don't.

In general, (In C) to allocate n elements of type T,
T *ptr = malloc(n * sizeof(*ptr));

Share this post


Link to post
Share on other sites
I always use new and delete but this is not my code. I decided that I am not going to reinvent the wheel every time when running someone else's code, unless its broken or crashes. And this isn't C the file is .cpp. So the compiler is going to compile with C++ standards ans C++ doesn't like malloc() without a explicit type cast.

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by scorpion007
EDIT:
What is the magic number `4'? Where are you getting it from? If you are assuming sizeof(float) == 4, don't.

In general, (In C) to allocate n elements of type T,
T *ptr = malloc(n * sizeof(*ptr));


sizeof(float) is 4 bytes as defined by IEEE.

Share this post


Link to post
Share on other sites
Quote:
Original post by MARS_999
I always use new and delete but this is not my code. I decided that I am not going to reinvent the wheel every time when running someone else's code, unless its broken or crashes.


Quote:
Original post by MARS_999
I have a crash on this code


So what's your excuse now? :)

Note that in C++ you have a perfectly good option for using a *real*, multiple-dimensioned and dynamically-sized array type: boost::multi_array. It's not from the standard library, but it *is* well tested, does everything you want it to, and will let you write things in a perfectly obvious way that avoids mistakes.

But since I'm feeling incredibly generous, I'm going to fix your code three times: Once doing things the way you're stubbornly insisting on doing, and again with refactoring to use the language properly.

The "real" problems are: (a) pointer arithmetic *already* takes the size of the pointed-at type into account - this is the reason why you can't do it on a void* - and therefore you should NOT add 4, but 1.

Unless you meant to skip over 4 floats, which is what I assume is what you really meant, in which case (b): you didn't allocate 4 "octaves" worth of space at the Noise3DTexPtr, because malloc() takes an amount in *bytes*, and therefore you *do* have to multiply in the sizeof() there.

So that gives us the ugly version:


float *ptr;
Noise3DTexPtr = (float*) malloc(NOISE3DTEXSIZE * NOISE3DTEXSIZE * NOISE3DTEXSIZE * 4 * sizeof(float)); // <-- teh changezor

for (f = 0, inc = 0; f < numOctaves; ++f, frequency *= 2, ++inc, amp *= 0.5)
{
SetNoiseFrequency(frequency);
ptr = Noise3DTexPtr;
ni[0] = ni[1] = ni[2] = 0;

inci = 1.0 / (NOISE3DTEXSIZE / frequency);
for (i = 0; i < NOISE3DTEXSIZE; ++i, ni[0] += inci)
{
incj = 1.0 / (NOISE3DTEXSIZE / frequency);
for (j = 0; j < NOISE3DTEXSIZE; ++j, ni[1] += incj)
{
inck = 1.0 / (NOISE3DTEXSIZE / frequency);
for (k = 0; k < NOISE3DTEXSIZE; ++k, ni[2] += inck, ptr += 4)
*(ptr + inc) = float((((noise3(ni) + 1.0) * amp) * 128.0));

}
}
}






So, now: a few things to note about good programming practice.

- Use const identifiers for constants, and have a single constant for inci/incj/inck, because it's clearly "built in" to the design that these dimensions are all the same.
- Writing *(ptr + inc) gets you nothing over writing ptr[inc]. Although, it's clearer to increment an index, rather than the 'ptr' pointer, which would also let you initialize it with 'inc', which becomes quite a bit cleaner.
- Never mind that, though; 'inc' is apparently just an alias for 'f', so cut it out. That lets us repurpose the name 'inc' to replace inci/incj/inck.
- You have hard-coded space for 4 octaves, but use a constant 'NumOctaves' within the loop. This is poor practice, as highlighted by the fact that it confused lots of posters (seeing as how sizeof(float) == 4 and you have problems with your malloc() call). Or wait, don't tell me that 'numOctaves' is a run-time variable, and you're just hoping it won't exceed 4? :( That's silly, especially if you're going to be doing dynamic allocation. (Heck, since NOISE3DTEXSIZE is a constant, if numOctaves were too, the only reason for dynamic allocation would be to keep a huge array off the stack.) No, wait, that can't be it, because you assume a "stride" of 4 in your inner loop. :s This is why magic numbers are bad, kids.
- Didn't anyone ever teach you anything about variable scope? By the way, we've got this cute little feature in C++ where you can scope your "loop counter" to the loop itself, which is the scope it normally *should* have.
- Is punctuation so expensive these days that you can't afford braces on the innermost loop? Oh, and how about lining those close braces back up?

Oh, and I'll use a vector for the dynamic allocation. I'm sure that performance is *critical* here, and I couldn't possibly convince you that boost::multi_array will satisfy your performance requirements ("It looks so elegant, with its ability to index in as if it were a static array; it can't *possibly* be optimized!" - well, you might be surprised; and there is a less sugary version that performs even better; and anyway, have you done any profiling yet?), but surely you'll agree that a pre-reserved vector costs you only a couple extra words of bookkeeping information?


std::vector<float> Noise3DTexture;
Noise3DTexture.reserve(NOISE3DTEXSIZE * NOISE3DTEXSIZE * NOISE3DTEXSIZE * numOctaves); // since the constructor would .resize() instead, and I'm sure you
// can't bear the cost of all that zero-initialization ;
// We can pull the multiplication by 128 all the way out, and might as well:
// it simplifies a complex expression.
// Actually, what is the initial value of 'amp'? Have you considered the
// possibility, since you seem to be concerned with performance, that you're
// doing more floating-point math than is necessary?
amp *= 128.0;

for (int octave = 0; octave < numOctaves; ++octave, frequency *= 2, amp *= 0.5) {
SetNoiseFrequency(frequency);
int index = f; // replaces 'ptr'
// This is the proper scope for 'ni':
float ni[3] = {0};
// Note how we replace the assignment with initialization. Yes, it will
// reoccur each time through the loop.

// Now, let's extract our single 'inc' constant. BTW, there's an obvious
// mathematical simplifcation here:
const float inc = frequency / NOISE3DTEXSIZE;
// I'm assuming frequency is a float, naturally, but if not, the other version
// probably wasn't correct either.

// Now, loops with properly scoped counters:
for (int i = 0; i < NOISE3DTEXSIZE; ++i, ni[0] += inc) {
for (int j = 0; j < NOISE3DTEXSIZE; ++j, ni[1] += inc) {
for (int k = 0; k < NOISE3DTEXSIZE; ++k, ni[2] += inc) {
Noise3DTexture[index] = float((noise3(ni) + 1.0) * amp);
index += numOctaves; // doesn't cost any more to do it here, you know.
// Might as well keep the three loops consistent and lines short.
}
}
}
}
amp /= 128.0; // assuming you need it again - I don't want to take any chances,
// given what I've seen so far!



(There have been a few miscellaneous edits to this post since the original, that all seem to have made it in under the time limit :\)

[Edited by - Zahlman on November 27, 2006 11:45:41 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Anonymous Poster
sizeof(float) is 4 bytes as defined by IEEE.


While that may be true, there is nothing in the C standard that guarentees this. So you're better off not relying on it.

Share this post


Link to post
Share on other sites
To Zahlman I did fix it by sizeof(float) * blah blah, I think I stated that earlier.

And like I said this isn't my code, so don't assume it to be my problem, but the coding practices are the persons who wrote the tutorial.

I myself I use pointer[index] instead of *(ptr + inc) I don't like the syntax.

But thank you for helping and showing the boost side of things. I haven't used boost just haven't had the need or desire to.

Share this post


Link to post
Share on other sites
Quote:
Original post by MARS_999
And like I said this isn't my code, so don't assume it to be my problem, but the coding practices are the persons who wrote the tutorial.


Well, I hope you get in touch with the tutorial author, then, and let him or her know that he or she is incompetent and has no business writing such a tutorial. :) Feel free to forward a link to this thread.

But if you want to have a project that includes code that performs the task illustrated here, and you want it to, you know, *work*, then you have to move beyond issues of who wrote what code, and fix the problem. :) (It *was* inappropriate of me to assume lack of knowledge of your part while going on my rhetorical spiel; and I apologize for that.)

(Also, don't let the bad practices of others drag you down. If you need to rely on code in a tutorial in order to figure something out, and you recognize that the code is badly written, fix it as you enter it. This process often helps you understand better what the code is really doing, too.)

Share this post


Link to post
Share on other sites

This topic is 4036 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.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this