Vector3 copy constructor

Minor issues with the Ogre API that can be trivial to fix

Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 10:10 am

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby CABAListic » Fri Jul 29, 2011 10:19 am

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?
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
 
Posts: 2902
Kudos: 58
Joined: 18 Jan 2007

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 10:24 am

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby CABAListic » Fri Jul 29, 2011 10:27 am

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 :)
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
 
Posts: 2902
Kudos: 58
Joined: 18 Jan 2007

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 10:36 am

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby CABAListic » Fri Jul 29, 2011 10:39 am

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).
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
 
Posts: 2902
Kudos: 58
Joined: 18 Jan 2007

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 10:48 am

Seems like it's not only 64bit GCC which wants to optimize a value parameter to a reference:
http://www.gamedev.net/topic/601680-crash-on-32bit-mingw-when-function-has-value-parameter/page__p__4808575__hl__asobj_app_class_cak__fromsearch__1#entry4808575
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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby CABAListic » Fri Jul 29, 2011 10:54 am

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?
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
 
Posts: 2902
Kudos: 58
Joined: 18 Jan 2007

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 11:18 am

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 11:27 am

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby CABAListic » Fri Jul 29, 2011 11:43 am

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.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
 
Posts: 2902
Kudos: 58
Joined: 18 Jan 2007

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 11:53 am

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby aguru » Fri Jul 29, 2011 1:10 pm

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
aguru
Goblin
 
Posts: 236
Kudos: 3
Joined: 26 Feb 2008

Re: Vector3 copy constructor

Postby Kojack » Fri Jul 29, 2011 1:15 pm

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
Kojack
OGRE Moderator
OGRE Moderator
 
Posts: 6453
Kudos: 410
Joined: 25 Jan 2004
Location: Brisbane, Australia

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 1:44 pm

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 2:24 pm

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby CABAListic » Fri Jul 29, 2011 2:39 pm

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...
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
 
Posts: 2902
Kudos: 58
Joined: 18 Jan 2007

Re: Vector3 copy constructor

Postby aguru » Fri Jul 29, 2011 5:50 pm

So we just IF(..) this into ogre's cmake and we should be good?
User avatar
aguru
Goblin
 
Posts: 236
Kudos: 3
Joined: 26 Feb 2008

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 5:57 pm

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 7:29 pm

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby CABAListic » Fri Jul 29, 2011 7:38 pm

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.
CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
 
Posts: 2902
Kudos: 58
Joined: 18 Jan 2007

Re: Vector3 copy constructor

Postby Kojack » Fri Jul 29, 2011 8:16 pm

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
Kojack
OGRE Moderator
OGRE Moderator
 
Posts: 6453
Kudos: 410
Joined: 25 Jan 2004
Location: Brisbane, Australia

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 8:38 pm

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 9:20 pm

In case anyone wonders 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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Re: Vector3 copy constructor

Postby jacmoe » Fri Jul 29, 2011 9:32 pm

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.
Ogre AppWizards - Ogre project wizards for VC 8-10, Code::Blocks and KDevelop.
OgreAssimpConverter - command-line to convert models to Ogre format.
TwOgreGUI - wrapper for AntTweakBar GUI library.
I accept donations | Me on Google+
User avatar
jacmoe
OGRE Moderator
OGRE Moderator
 
Posts: 21024
Kudos: 161
Joined: 22 Jan 2004
Location: Denmark

Next

Return to Papercuts

Who is online

Users browsing this forum: No registered users and 1 guest