return a float array in a function

Recommended Posts

cozzie    5029

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

Share on other sites
cozzie    5029

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;
}


Share on other sites
alvaro    21246
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

Share on other sites

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.

Share on other sites
cozzie    5029

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.

Share on other sites

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

Share on other sites
cozzie    5029

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

Share on other sites
belfegor    2834

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

Share on other sites
mdias    823

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

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;
}


Share on other sites
mdias    823

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.

Share on other sites
SiCrane    11839
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());
}


Share on other sites
rip-off    10976

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)
{
// ...
}


Share on other sites
belfegor    2834

@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?

Share on other sites
rip-off    10976

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

Share on other sites
cozzie    5029

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



Share on other sites
belfegor    2834

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

const D3DXVECTOR3& CD3dcam::GetPosition() const
{
return mPosition;
}
...

Edited by belfegor

Share on other sites
alvaro    21246

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

Share on other sites
SiCrane    11839

[background=#fafbfc]You could make a conversion operator...[/size][/background]

... 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 [tt]explicit[/tt] conversion operators which avoid many of the problems with implicit conversions.

Share on other sites
cozzie    5029

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