Ambiera Forum

Discussions, Help and Support.

Ambiera Forum > CopperLicht
Bug in CL3D.Matrix4.prototype.transformPlane?

sebmaster
Registered User
Quote
2011-03-23 09:17:12

I'm currently writing JSDocs for copperlicht.js to make compiling with google closure easier.

When I went trough the copperlicht.js I noticed in the CL3D.Matrix4.prototype.transformPlane function there's a call to the rotateVect function. As far as I can see, there's a this missing in front of it.

Greetings,
Sebmaster


sebmaster
Registered User
Quote
2011-03-23 09:30:00

Ok. I guess I'll make that a overall bug thread, because I guess I'll definitly notice more:

In the CL3D.Action.IfVariable.prototype.execute function, there's a call to Core.equals. Looks like you missed something when refactoring to the CL3D namespace, niko ;)


niko
Moderator
Quote
2011-03-23 10:36:29

thanks for posting this, will be fixed soon.


sebmaster
Registered User
Quote
2011-03-23 10:59:16

Guess i'll post more of them, as I find them:

First one:
In the CL3D.SceneNode.prototype.getAnimatorOfType method, you're leaking i into the global namespace.

Second:
In the CL3D.CameraSceneNode.prototype.createClone method, there's another this missing when calling cloneMembers.


sebmaster
Registered User
Quote
2011-03-23 11:22:14

Oh, in the CL3D.CameraSceneNode.prototype.createClone method are a few this missing, not only one.

Also:
In the CL3D.SkyBoxSceneNode.prototype.createClone method, there's a this missing when checking for this.OwnedMesh.

In the CL3D.PathSceneNode.prototype.getPathNodePosition method, there's a this missing when calling updateAbsolutePosition.

In the CL3D.DummyTransformationSceneNode.prototype.createClone method, there's another missing namespace when instantiating the new object.

In the CL3D.AnimatedMeshSceneNode.prototype.render method, there's another this missing when doing setWorld with the AbsolutePosition, two times.

In the CL3D.AnimatorCameraFPS.prototype. method, there's another this missing, when calculating the targetZoomValue.

In the CL3D.AnimatorOnProximity constructor, there's another CL3D missing at EPET_LEAVE.

In the CL3D.Quaternion.prototype.toString method, there are a few this missing too.

That should be it, for now


sebmaster
Registered User
Quote
2011-03-23 11:53:59

Eh, on further investigation I found one more:

In the CL3D.Plane3d.prototype.clone method, you mistyped Plane3d.

The EPET_LEAVE I mentioned before:
I'm not quite sure where that should be defined, but should be 1.


sebmaster
Registered User
Quote
2011-03-23 12:39:32

I'll follow up with a few things, which are not actually bugs, but they generate a warning in google closure, which makes further compiling a bit verbose/generates a few warnings:

in the CL3D.CCFileLoader {this.load} method, var d = this; is defined twice.

In the CL3D.BinaryStream.prototype.decodeFloat method, m is used, before the var declaration is parsed by closure. It would be helpful, if you'd extract the other var declarations, add m and move it one line above the operations with m.

In the CL3D.Action.Shoot.prototype.execute method, h is defined twice by a var statement.

In the CL3D.AnimatorRotation.prototype.animateNode method, b in defined twice too.

It would be really nice, if you could sort these out too.


sebmaster
Registered User
Quote
2011-03-23 19:56:37

Hopefully, this will be my last post in here until at least the next update, but to be fully compatible with Closure, more things have to be modified:

In the CL3D.Renderer.prototype.removeCompatibilityProblems method, you access the array types directly. For Closure, the left part of the assignment has to be like this:
window['WebGLFloatArray'] = Float32Array;


In the CL3D.Action.MakeSceneNodeInvisible constructor, this.InvisibleMakeType is used as a boolean, even though it is a number.

In the CL3D.MeshBuffer.prototype.recalculateBoundingBox method, this.box.reset is called, but I can't find that method defined anywhere. Maybe check that further?

In the CL3D.BinaryStream.prototype.readFloat16 method, you call this._readFloatingPoint, but this method also doesn't exist. Should be decodeFloat32(fast)?

In the CL3D.AnimatorOnProximity constructor, you initialize TheActionHandler with 0, but later call a method on it. Initialize with null (although that's not that important)?
This also occurs in the CL3D.AnimatorGameAI constructor.

In the CL3D.AnimatorCollisionResponse.prototype.animateNode method, you're accessing this.triangle. Also nowhere defined?

Happy coding


niko
Moderator
Quote
2011-03-24 07:50:48

Woha, thanks for the detailed list. Just worked through it, and all of this will be fixed with the next update.
Interersting that it worked so nicely although there were so many small typos in the code.


sebmaster
Registered User
Quote
2011-03-24 10:42:11

Another quick suggestion regarding coding style:

It would maybe be worh thinking about using the instanceof keyword instead of checking with getType, because one could inherit in his own prototype, return a different type than the parent prototype and copperlicht wouldn't notice it.

It would maybe require to think about this use case in every single case of using getType, if a replacement with instanceof is possible.


niko
Moderator
Quote
2011-03-24 12:42:10

Would be an idea, yes. BTW: I've released a new CopperLicht version (1.3.5) which includes about 95% of the bug fixes reported in this thread.


sebmaster
Registered User
Quote
2011-03-24 14:55:25

Saw it and already integrated it into my copperlicht version.

The only thing left as far as i can see, is the thing in the decodeFloat function, but this could also be due to inlining from the used compiler.

I'll integrate the type checks by closure sometime in the near future, so get yourself ready


sebmaster
Registered User
Quote
2011-06-23 15:20:50

Because of the new update I looked through the copperlicht code again and noticed some more things:

max3 and min3 can be replaced by Math.max and Math.min, no need for custom functions.

In the Vect3d constructor you're setting the x,y and z coordinates to 0 but that's not needed, since you're already setting them in the prototype, same with Vect2d.

Math.floor (for example in the fract function) can be replaced with ~~ or >> 0. It's a nice speed up for example in Firefox. In Chrome it's not that noticeable. (Testcase: http://jsperf.com/vs-vs-parseint-bitwise-operators/3).

In the CL3D.Scene.prototype.drawAll function you declare a variable right after if (this.UseCulling) twice.

In the CL3D.Scene.prototype.doAnimate function, you're missing a = while comparing with 0.

In the CL3D.Animator3rdPersonCamera.prototype.animateNode function, you're also declaring a variable twice (var e = false).

In the CL3D.AnimatorCameraFPS.prototype.animateNode function, you're checking for this.moveByMouseDown but doing nothing. Is there something missing, or just overhead?

In the CL3D.Plane3d.prototype.clone function, you're supplying a unneccessary boolean to the constructor of Plane3d.

That's everything. No more complaints from closure. Maybe a few type errors if I integrate this check too, but that's going to be much work. Will begin anyways


niko
Moderator
Quote
2011-06-24 08:30:30

Thanks as always :)


Create reply:


Posted by: (you are not logged in)


Enter the missing letter in: "Interna?ional" (you are not logged in)


Text:

 

  

Possible Codes


Feature Code
Link [url] www.example.com [/url]
Bold [b]bold text[/b]
Image [img]http://www.example.com/image.jpg[/img]
Quote [quote]quoted text[/quote]
Code [code]source code[/code]

Emoticons


   






Copyright© Ambiera e.U. all rights reserved.
Privacy Policy | Terms and Conditions | Imprint | Contact