Jump to content

  • Log In with Google      Sign In   
  • Create Account


return a float array in a function


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
18 replies to this topic

#1 cozzie   Members   -  Reputation: 1073

Like
0Likes
Like

Posted 11 July 2013 - 03:43 PM

Hi,

 

I'm trying to make a const function returning a float[3] array with the position of my camera.

But I can't manage to do this, without passing the array as a function parameter.

 

To be exact, this is what I do now:

void CD3dcam::GetPositionF(float pPos[]) const
{
	pPos[0] = mPosition.x;
	pPos[1] = mPosition.y;
	pPos[2] = mPosition.z;
}

But this is ofcourse annoying if I want to pass GetPositionF to a function as a parameter, where a float array is expected.

So I tried something like

 

float* CD3dcam::GetPositionF() const

{

   float pos[3];

   pos[0] = mPosition.x;

   pos[1] = mPosition.y;

   pos[2] = mPosition.z;

 

   return pos;

}

 

This compiles, but gives a warning that I'm returning the address of a local variable in memory, instead of a pointer to the actual values.

Any ideas anyone?


Edited by cozzie, 11 July 2013 - 03:46 PM.


Sponsor:

#2 cozzie   Members   -  Reputation: 1073

Like
0Likes
Like

Posted 11 July 2013 - 03:50 PM

Update;

This seems to work, not sure though if it's correct:

float* CD3dcam::GetPositionF() const
{
	float *pos = new float[3];

	pos[0] = mPosition.x;
	pos[1] = mPosition.y;
	pos[2] = mPosition.z;

	return pos;
}


#3 Álvaro   Crossbones+   -  Reputation: 10634

Like
2Likes
Like

Posted 11 July 2013 - 03:52 PM

You either get the user to pass you the array (what you had), return the address of a global object or return an object that wraps the array. The last one is probably what I would go for. You seem to be using float[3] to represent points. Make a class Point instead, and return that.

EDIT: Yes, you could also return a dynamically allocated buffer, which is what you did in your second post. But then the calling code needs to remember to delete[] it, and they don't, and this is how you create memory leaks.

Edited by Álvaro, 11 July 2013 - 03:53 PM.


#4 Paradigm Shifter   Crossbones+   -  Reputation: 4785

Like
0Likes
Like

Posted 11 July 2013 - 04:06 PM

You can return a struct (by value, i.e. not via a pointer to the struct) which has a float[3] as its only member as well if you really want to return the array. This may copy but the compiler should be able to optimise the copy out.

 

Don't return the address of a global variable, it doesn't scale well (meaning: doesn't scale at all) if you need to call it recursively or on multiple threads.

 

EDIT: I see Alvaro suggested making a Point class, missed that, it's the same as what I suggested using a class instead of a struct.


Edited by Paradigm Shifter, 11 July 2013 - 04:14 PM.

"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#5 cozzie   Members   -  Reputation: 1073

Like
0Likes
Like

Posted 11 July 2013 - 04:13 PM

Thanks.

I'm not sure though if making a struct will bring me what I'm looking for.

The function I have to pass the float array too, expects a pointer array:

 

HRESULT SetFloatArray(
  [in]  D3DXHANDLE hParameter,
  [in]  const FLOAT *pf,
  [in]  UINT Count
);
 

That would mean that I have to create a local object of the struct in every frame, filling it with GetPositionF and passing the member of the object to the function above.

Sounds a bit 'waste', although I don't know how to do it easier.



#6 Paradigm Shifter   Crossbones+   -  Reputation: 4785

Like
0Likes
Like

Posted 11 July 2013 - 04:21 PM

Make mPosition a union? Of a float[3] array and float x, y, z; Then just pass &mPosition directly.


"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

#7 cozzie   Members   -  Reputation: 1073

Like
0Likes
Like

Posted 11 July 2013 - 04:22 PM

I 'solved' it now by making a public member float array: mPosF[3] and update it when the camera moves.

And in the SetFloatArray I just call that public member of my camera class.

This works for now, still not sure if there's a better way though.

 

Paradigm shifter: that's sort of what I did now, I think :)


Edited by cozzie, 11 July 2013 - 04:22 PM.


#8 belfegor   Crossbones+   -  Reputation: 2126

Like
2Likes
Like

Posted 11 July 2013 - 04:23 PM

You could make a conversion operator:

struct Vec3
{
    float x,y,z;
 
    operator float* ()
    {
        return &x;
    }
 
    operator const float* () const
    {
        return &x;
    }

};

Edited by belfegor, 11 July 2013 - 04:26 PM.


#9 mdias   Members   -  Reputation: 737

Like
0Likes
Like

Posted 12 July 2013 - 07:41 AM

I 'solved' it now by making a public member float array: mPosF[3] and update it when the camera moves.

And in the SetFloatArray I just call that public member of my camera class.

This works for now, still not sure if there's a better way though.

 

Paradigm shifter: that's sort of what I did now, I think smile.png

 

This is NOT the way to go. If you have public access to the position, you will be able to change it's data without letting your CD3dcam class know about it, which is bad because you'll not be able to cache the view matrix (to recalculate it only when needed later, when you're optimizing).

 

The best approach is to use a struct much like belfegor posted and have the following methods on your CD3dcam:

const Vec3& getPosition() const
{
   return mPos;
}

void setPosition( const Vec3& pos )
{
   mPos = pos;
}


#10 mdias   Members   -  Reputation: 737

Like
0Likes
Like

Posted 12 July 2013 - 07:45 AM

 

Update;

This seems to work, not sure though if it's correct:

float* CD3dcam::GetPositionF() const
{
	float *pos = new float[3];

	pos[0] = mPosition.x;
	pos[1] = mPosition.y;
	pos[2] = mPosition.z;

	return pos;
}

Indeed that will work. It'll make it very easy to have memory leaks though, and will possibly bring problems if this method uses a different memory allocator implementation than what is used by the caller to free it up later.



#11 SiCrane   Moderators   -  Reputation: 9125

Like
1Likes
Like

Posted 12 July 2013 - 09:02 AM

Note that C++ has a standard container to encapsulate fixed size arrays: std::array. Here you could use a std::array<float, 3>. Of course, std::array is a relatively recent addition to C++, so not all compilers will have it. If yours doesn't you can use boost::array instead. While there is no implicit conversion from a std::array<float, 3> to float *, you can use .data() on a non-const rvalue. Ex:
std::array<float, 3> foo();
void bar(float *);

int main(int, char **) {
  bar(foo().data());
}


#12 rip-off   Moderators   -  Reputation: 6875

Like
3Likes
Like

Posted 12 July 2013 - 09:57 AM


You could make a conversion operator...

... but don't. There is a reason why the standard committed opted for std::string::c_str() rather than making an implicit conversion to C string.

 

Consider what that code allows:

Vec3 vec = /* ... */;
 
if(vec)
{
   // ...
}

The intent here was probably to perform a more interesting condition, but the implicit conversion allows this code to compile.

 

You might say that is unlikely, and you are probably right. What about this example, where the programmer probably meant to apply a function such as lengthSqrd()  to each vector:

Vec3 a = /* ... */;
Vec3 b = /* ... */;
    
if(a < b)
{
    // ...
}


#13 belfegor   Crossbones+   -  Reputation: 2126

Like
0Likes
Like

Posted 12 July 2013 - 11:23 AM

@rip-off

Thank you for pointing out possible problems with this.

One way i could see to avoid that is to make:

struct Vec3
{
private:

    operator bool ()
    {
        return false;
    }

public:
...

and have compile time error if such thing happen by accident.

Do you see any problem with this?



#14 rip-off   Moderators   -  Reputation: 6875

Like
2Likes
Like

Posted 12 July 2013 - 11:48 AM

I still think a named function, such as toArray() / asArray(), would be better.

 

You'd have to add "operator bool() const" too, to avoid the same problem with const objects. You could still run into this problem inside private or friend functions, though this is unlikely to be a scenario for a (formerly) simple vector class. You could however omit the implementation, thus getting a linker error if it is erroneously used even by a privileged function. Finally, you have to explain/justify these bizarre function stubs to your team, and write a suitably menacing comment so no-one accidentally deletes the apparently unused code...



#15 cozzie   Members   -  Reputation: 1073

Like
0Likes
Like

Posted 12 July 2013 - 12:22 PM

Thanks all.

I think I've a found a good balance without taking risk and now did it like this:

// private members

D3DXVECTOR3 mPosition;
float *mPosF;

// public member function prototype
float* GetPositionF() const;

// the member function implementation
float* CD3dcam::GetPositionF() const
{
	return mPosF;
}

// in constructor
mPosF = new float[3];

// in destructor
delete[] mPosF;

// the call where I use the array
if(FAILED(pD3dscene->mShaders[0].mEffect->SetFloatArray(pD3dscene->mShaders[0].mHCameraPos, (float*)pCam->GetPositionF(), 3))) return false;	// NOT SHARED YET!!!! MIND YOU!!



#16 belfegor   Crossbones+   -  Reputation: 2126

Like
0Likes
Like

Posted 12 July 2013 - 12:42 PM

D3DXVECTOR3 already have conversion/cast operator so you could just pass that:

const D3DXVECTOR3& CD3dcam::GetPosition() const
{
    return mPosition;
}
...
if(FAILED(pD3dscene->mShaders[0].mEffect->SetFloatArray(pD3dscene->mShaders[0].mHCameraPos, pCam->GetPosition(), 3)) return false;

Edited by belfegor, 12 July 2013 - 12:48 PM.


#17 Álvaro   Crossbones+   -  Reputation: 10634

Like
0Likes
Like

Posted 12 July 2013 - 12:49 PM


Thanks all.

I think I've a found a good balance without taking risk and now did it like this:
[...]

 

Your use of new and delete[] seems suspicious. Why do you want to allocate a separate piece of memory to contain the same stuff you already have in the D3DXVECTOR3 representation? Taking the address of the `x' member and treating it as an array of 3 floats should work just fine. The C++ standard doesn't guarantee that it will work, of course. If that bothers you, I am sure there are some pills you can take. :)



#18 SiCrane   Moderators   -  Reputation: 9125

Like
1Likes
Like

Posted 12 July 2013 - 04:33 PM

You could make a conversion operator...[/size]

... but don't. There is a reason why the standard committed opted for std::string::c_str() rather than making an implicit conversion to C string.
Note that if you have a C++11 compiler, you can use explicit conversion operators which avoid many of the problems with implicit conversions.

#19 cozzie   Members   -  Reputation: 1073

Like
0Likes
Like

Posted 13 July 2013 - 04:42 AM

Thanks Alvaro. I actually didn't check if D3DXVECTOR3 had an operator to return a float array for XYZ.

Got it working now, with no unnecessary float array and keeping track of XYZ pos 2 times each frame






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