Vector3 copy constructor

Minor issues with the Ogre API that can be trivial to fix
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Vector3 copy constructor

Post by jacmoe »

When trying to bind Ogre::Vector3 with Angelscript on 64bit Debian using GCC I encounter a problem:
Vector3 doesn't have a copy constructor.

The problem is:
On Linux/amd64 with GNUC a class is treated very differently if the copy constructor is declared or not. A class with a copy constructor is always passed through a reference even when the function is declared to pass the type by value. When the copy constructor is not declared, the compiler will pass the type in the CPU registers instead, if the class is small enough. The real complicated part is that when the type is passed in the CPU registers, it is split up by the type of each member, so integer members are passed in the general purpose registers, and float members are passed in the floating point registers. This is why AngelScript doesn't allow these types to be passed by value, the implementation to support that in the native calling convention is just too difficult.

You're not seeing this problem on Windows or Linux 32bit because the existence of the copy constructor or not doesn't make a difference on these platforms.
Quote is from Andreas Jönsson, the Angelscript developer.
(Topic)

I wonder if this could be added?

I am not a C++ guru, and I have no idea if it will be affecting performance, but I nearly gave up on binding Ogre to Angelscript due to this issue. Works a treat on 32bit *nix and all-bit Windows.

I am certain that it has some consequence in other situations as well when on 64bit *nix.
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
x 58
Contact:

Re: Vector3 copy constructor

Post by CABAListic »

I don't follow. Vector3 has no manually implemented copy constructor, but it doesn't declare one, either, so the standard guarantees that a default copy constructor is made available which does what is needed for Vector3.

What the hell is AngelScript doing that it even cares how the compiler passes a value to a function?
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

Mind your language. :wink:

It's what he said: 64bit GCC treats an object differently if it has a copy constructor or not.
32bit doesn't care. Neither does nmake.

If it doesn't cost anything, why not add it?
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
x 58
Contact:

Re: Vector3 copy constructor

Post by CABAListic »

Because Vector3 has a copy constructor. An implicitly-generated one. This is guaranteed by the standard, and if that doesn't work, then someone is doing something very wrong, and it's not us :)
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

I am led to believe that it is GCC which is doing this to us.
When an object doesn't feature an explicit copy constructor and passed by value, it is really passing it by reference deep under the hood - but only for 64bit.
You only notice this when doing something in low level native code, like they do in Angelscript.
I'll see if I can find a reference for you. :wink:
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
x 58
Contact:

Re: Vector3 copy constructor

Post by CABAListic »

I suppose, but that's why I wanted to know why AngelScript even cares. As long as you do not manipulate raw byte data (which is never guaranteed to be portable), it doesn't really matter to my code if it's passed by reference or not (so long as I'm not manipulating the data, but I expect the compiler to notice that).
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

Seems like it's not only 64bit GCC which wants to optimize a value parameter to a reference:
http://www.gamedev.net/topic/601680-cra ... try4808575
I guess it matters to Angelscript because it creates low level native calling voodoo code.
At the assembly level (registers and stuff).

If it works, and doesn't hurt anything - because it would be implicitly created anyway - it would be really nice if an explicit copy constructor could be added to Vector3 and friends.
I guess Quaternion and the other primitive PODs needs it too..
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
x 58
Contact:

Re: Vector3 copy constructor

Post by CABAListic »

Well, I suppose it's not that big a deal, but I don't like it - implicitly created copy constructors are a language feature and help reduce the amount of code to write (as you said, this wouldn't just affect Vector3) and consequently increase maintainabilty - it's just one less function you have to remember if ever you do make some changes to your class.

The better 'fix' might be to change the relevant functions to take a const reference to Vector3, which, given its size, might be preferable anyway.

BTW, have you tried the fix that was proposed in the post you linked to?
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

Of course. :wink:

Everything. Anything. :)
The 'K' in _CAK stands for copy constructor.

If only one could get away with passing Ogre::Vector3 around by reference..
You can't always do that though.

But since Vector3 already has an explicit default constructor... :twisted:

Well, it would be a tremendous help if it could be added, but I get your point of view.
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

I added this to Ogre::Vector3:

Code: Select all

        inline Vector3(const Vector3& other)
            : x(other.x), y(other.y), z(other.z)
        {
        }
And my issue is fixed. :)

I am not willing to think about the amount of work I went through to fix Angelscript/Ogre on 64bit *nix ... but we're talking months of frustration.. :mrgreen:
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
x 58
Contact:

Re: Vector3 copy constructor

Post by CABAListic »

If it isn't possible to pass by const reference, then you really need a copy, because you are modifying the instance. In that case, this compiler optimisation cannot possibly apply.
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

I don't mean pass - passing by const reference works a treat. Of course.

The issue was that I could neither pass by value nor return by value.
The first you seldom do, but the latter you do a lot.
Ogre::Vector3 does it all over the place, because it's a POD.
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
User avatar
aguru
Goblin
Posts: 236
Joined: Tue Feb 26, 2008 5:48 pm
x 3

Re: Vector3 copy constructor

Post by aguru »

So what are the chances of this to make it into Ogre trunk? This was really a big show-stopper for us on gcc x64.
User avatar
Kojack
OGRE Moderator
OGRE Moderator
Posts: 7157
Joined: Sun Jan 25, 2004 7:35 am
Location: Brisbane, Australia
x 534

Re: Vector3 copy constructor

Post by Kojack »

Actually Vector3 isn't a POD.
According to the c++ standard, POD's aren't allowed to have user defined constructors, and Vector3 has 6 of them.

(It meets all the other rules of a POD, but not the "aggregate class" rule which says no user constructors).
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

Maybe it doesn't fit with the nerdy official definition of a POD, but it is in other regards a primitive type.
Let's call it a border-line case. :)

Ironically, since it doesn't have an explicit copy constructor ...
The class already has init constructors, etc.

I believe that adding explicit copy constructors would help prevent some rare inexplicable crashes/issues/whatnot when in 64-bit GCC mode.
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

I glanced at Ogre..
At least Matrix3 has an explicit copy constructor..
All the rest (AFAIK) haven't.
A lot of trivial changes. Turning this into something not so trivial. :)

I'll make a fork and issue a pull request once Ogre works the way I need it to work.
Then we'll see.
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
x 58
Contact:

Re: Vector3 copy constructor

Post by CABAListic »

jacmoe wrote: I believe that adding explicit copy constructors would help prevent some rare inexplicable crashes/issues/whatnot when in 64-bit GCC mode.
Unlikely. If so, that would be a compiler bug and needs to be addressed by GCC :)

Just for the record, how many classes are we talking about here? Because AngelScript really is the very first instance I've seen where this would actually matter, and that kind of seems to me to be AngelScript's fault for doing something which is probably not quite standards-compliant...
User avatar
aguru
Goblin
Posts: 236
Joined: Tue Feb 26, 2008 5:48 pm
x 3

Re: Vector3 copy constructor

Post by aguru »

So we just IF(..) this into ogre's cmake and we should be good?
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

aguru wrote:So we just IF(..) this into ogre's cmake and we should be good?
I believe you referred to the post I got rid of which was about a flag which will turn off that optimization for GCC..
In that case: yes.
*if* it even works, that is.
And since it requires a custom built Ogre anyway, we could just go ahead and patch the beast with proper explicit copy constructors and be done with it. :wink:
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

CABAListic wrote:that would be a compiler bug and needs to be addressed by GCC :)
I see plenty of ugly code in Ogre code meant to work around various compiler bugs already.
So that's not really an argument. :wink:
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
x 58
Contact:

Re: Vector3 copy constructor

Post by CABAListic »

Most of which are probably no longer necessary. In any case I still think this is not a compiler bug, because I still fail to see why all this matters to AngelScript. Imho, they must be doing something really, really hacky to suffer any issues from this, and I don't really want Ogre to suffer from their exploits. So as long as I don't know what exactly is causing problems, I'm feeling uneasy with fixing it in Ogre. Imho, it would be much better to find a workaround in AngelScript because the issue is not exclusive to Ogre.
User avatar
Kojack
OGRE Moderator
OGRE Moderator
Posts: 7157
Joined: Sun Jan 25, 2004 7:35 am
Location: Brisbane, Australia
x 534

Re: Vector3 copy constructor

Post by Kojack »

Maybe it doesn't fit with the nerdy official definition of a POD, but it is in other regards a primitive type.
Let's call it a border-line case. :)
Ok, it's LIKE a POD.

But you are registering the class with the angelscript asOBJ_POD flag, the C++ standard says the class is NOT a POD, and the compiler is generating code which can't read the vector3 correctly. Since the compiler follows the "nerdy definition" and not personal opinion, what definition is asOBJ_POD expecting? Does it want a real POD or just something almost a POD?

It would be interesting to see what would happen if vector3 didn't have any constructors and was a true POD type. Would that fix the problem? Of course that would suck for ogre, I'm not suggesting we remove them permanently, I'm just curious if this POD stuff is a coincidence or part of the problem.
I believe you referred to the post I got rid of which was about a flag which will turn off that optimization for GCC..
Which flag was that?
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

Code: Select all

-fno-ipa-sra
But, I think I picked the wrong flag: zero effect. :(

Anyway, I am not in mood for exercises in futility, so I am dropping the case.
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

In case anyone [s]wonders[/s] cares, I found a fix:

I am now using the Angelscript autowrapper addon:

Code: Select all

#include "aswrappedcall.h"
And use it to wrap the problem function(s) - those which returns/passes Ogre PODs by value:

Code: Select all

asDECLARE_METHOD_WRAPPER(AngelscriptTest_getCameraPosition_Wrapper, AngelscriptTest, getCameraPosition);
Then use the wrapper instead of the method:

Code: Select all

    // Register the wrapper, instead of the real method
    r = mEngine->RegisterObjectMethod("AngelscriptTest", "vector3 getcp()", asFUNCTION(AngelscriptTest_getCameraPosition_Wrapper), asCALL_GENERIC); assert( r >= 0);
And - it works. :wink:

<edit>
Andreas Jönsson - the Angelscript developer - is one of my personal heroes! :)
</edit>

<edit2>
What does it mean??
It means that the problem functions are wrapped generically instead of natively.
</edit2>
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
User avatar
jacmoe
OGRE Retired Moderator
OGRE Retired Moderator
Posts: 20570
Joined: Thu Jan 22, 2004 10:13 am
Location: Denmark
x 179
Contact:

Re: Vector3 copy constructor

Post by jacmoe »

This means that you can have your Ogre and eat him too.. :wink:
/* Less noise. More signal. */
Ogitor Scenebuilder - powered by Ogre, presented by Qt, fueled by Passion.
OgreAddons - the Ogre code suppository.
Post Reply