Misleading usage of Quaternion.multLocal(Vector3f)

Hi, when browsing trough the transform code i found a few lines that are not really when looking at them. I am talking about:
[java]
parent
.rot
.multLocal(translation)
.addLocal(parent.translation);
[/java]

The syntax suggests that you are executing the .multLocal() and the .addLocal() on the parent quaternion. (If you follow the usual jme way of dooing stuff)
I tought i found a serious bug, but why should nobody else notice a bug of that severity?
As it turns out Quaterion.multLocal(Vector3f) does not multiply the quaternion, but the passed Vector3f

From my point of view, a clearer an more conform way of dooing it is:

Move Quaterion.multLocal(Vector3f) to Vector3f.multLocal(Quaterion)

The code used in Transform.combineWithParent(Transform parent) would then be:
[java]
translation
.multLocal(parent.rot)
.addLocal(parent.translation);
[/java]

NOTE: it’s not really a bug, but from my point of view a very misleading design decision. The downside is, that a clean solution would probably break lots of usercode

Hi, when browsing trough the transform code i found a few lines that are not really clear when looking at them. I am talking about:
[java]
parent
.rot
.multLocal(translation)
.addLocal(parent.translation);
[/java]

The syntax suggests that you are executing the .multLocal() and the .addLocal() on the parent quaternion. (If you follow the usual jme way of dooing stuff)
I tought i found a serious bug, but why should nobody else notice a bug of that severity?

As it turns out Quaterion.multLocal(Vector3f) does not multiply the quaternion, but the passed Vector3f.

From my point of view, a clearer an more conform way of dooing it is:

*Move Quaterion.multLocal(Vector3f) to Vector3f.multLocal(Quaterion)

The code used in Transform.combineWithParent(Transform parent) would then be:
[java]
translation
.multLocal(parent.rot)
.addLocal(parent.translation);
[/java]

NOTE: it’s not really a bug, but from my point of view a very misleading design decision. The downside is, that a clean solution would probably break lots of usercode

//Sorry for doublepost, it seems i cant edit the first post…

1 Like

I agree that the name is misleading by multiplying a Quaternion by a Vector3f in a way that modifies the quaternion in some way doesn’t really make sense. So the alternative is that it doesn’t mean what it seems to mean.

Quaternion operations should not be a part of Vector3f, though.

It’s kind of a silly method anyway since Quaternion.mult(v, v) will give the same results and is more explicit about what is happening.

1 Like

Agree that quaternion operation should not be in the vector class.
I would mark multLocal as deprecteted in the stable branch, and use the logic from .mult(v3,v3). In the next major update that deprected stuff could be removed than.

Here is a patch for that:
[java]

This patch file was generated by NetBeans IDE

It uses platform neutral UTF-8 encoding and \n newlines.

— Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -969,18 +969,9 @@
* the vector to multiply this quaternion by.
* @return v
*/

  • @Deprecated
    public Vector3f multLocal(Vector3f v) {
  •    float tempX, tempY;
    
  •    tempX = w * w * v.x + 2 * y * w * v.z - 2 * z * w * v.y + x * x * v.x
    
  •            + 2 * y * x * v.y + 2 * z * x * v.z - z * z * v.x - y * y * v.x;
    
  •    tempY = 2 * x * y * v.x + y * y * v.y + 2 * z * y * v.z + 2 * w * z
    
  •            * v.x - z * z * v.y + w * w * v.y - 2 * x * w * v.z - x * x
    
  •            * v.y;
    
  •    v.z = 2 * x * z * v.x + 2 * y * z * v.y + z * z * v.z - 2 * w * y * v.x
    
  •            - y * y * v.z + 2 * w * x * v.y - x * x * v.z + w * w * v.z;
    
  •    v.x = tempX;
    
  •    v.y = tempY;
    
  •    return v;
    
  •    return this.mult(v, v);
    

    }

    /**
    [/java]

return this.mult(v, v);

Just a note… the “this” is unnecessary and I can’t think of a good reason conventionally to put it there.

@pspeed said: return this.mult(v, v);

Just a note… the “this” is unnecessary and I can’t think of a good reason conventionally to put it there.

Heh, i am used to it, or i have trained me to use it.
Since the time i use static imports i don’t see a difference between calls on a remote static object, or inner calls of the object. “this” makes it more visible.

On the other side, beside the 5 to 2 keystrokes (depending on the IDE you use) i also don’t see a downside

1 Like
@zzuegg said: Heh, i am used to it, or i have trained me to use it. Since the time i use static imports i don't see a difference between calls on a remote static object, or inner calls of the object. "this" makes it more visible.

On the other side, beside the 5 to 2 keystrokes (depending on the IDE you use) i also don’t see a downside

It’s the most common way to call a method… and the only reason purported to use ‘this’ is to avoid confusion with static method imports which many conventions would just specifically disallow rather than pollute the rest of the code with redundant ‘this’.

Not to mention that the static imports are easily located in the rarer case that they are used.

Yeah, i agree that it’s the most common way to call a method. But it’s most likely a leftover convention from then non OO time.

I mean, the systax for executing something in an OO environment would be something like:
[java]
(some object reference).(some method/object/variable reference)
[/java]

Allowing a method call without ‘this’ probably requires an additioanl syntax rule. I personall use not ‘this’ calls only when there is no other way. Like static methods and method local variables.

I also think that using this everywhere is the saver way of dooing stuff. Immagine:

[java]
class MyFancyObject{
int x;
//some hundred lines of code

public void myFancyMethod(){
//lot’s of fancy calculations;

x=resultOfFancyCalculation;

}
}
[/java]

Now a year later you get fired and they hire a cheap junior instead. He browses trough the code, finds the fancy calculation routine and tries to make some optimisations. If he adds a tempory variable “int x” somewhere in his code he would break the result.

My point is, that from my side it really has no downsides using this. everywhere, but it has some real slight advantages… beside better readability.

Just my two cents of course…

3 Likes

For the longest time I didn’t use ‘this’ for method calls since it was unambiguous anyway, but I’ve changed my ways to using it all the time. It is more consistent I feel.
For variables I’ve always used ‘this’ since those could clash with method scoped variables and using ‘this’ was better than inventing some name-mangling to show if it was a member variable or method scope variable. But since static imports methods have the same sort of confusion. So why not be consistent all over.

Another, silly reason but still, is that I get command completion in the IDE when I type ‘this.’ :slight_smile:

Same thing with “final”, I’ve totally come around on that and write final all over the place. The code is more verbose but it also tells more about the assumptions I’ve made.

That was my two cents of course…

@zzuegg said: Now a year later you get fired and they hire a cheap junior instead. He browses trough the code, finds the fancy calculation routine and tries to make some optimisations. If he adds a tempory variable "int x" somewhere in his code he would break the result.
Except that if he uses a decent IDE, he'll have a warning like "local variable hides a field". Idk, I only use "this" when I can't do otherwise too...but I guess that's a matter of taste at some point.
@nehon said: Except that if he uses a decent IDE, he'll have a warning like "local variable hides a field". Idk, I only use "this" when I can't do otherwise too...but I guess that's a matter of taste at some point.

Likewise, when I see “this” I then look for what I’d be hiding (parameter or whatever) if I didn’t. Otherwise, leave it off.

See I have my autofromatter set to always convert varaible to full qualified versions. So always this and always Class.staticvar

But yes matter of style.
What is kinda necessary for jme3 is a code convention for developers&contributors, + a full reformating of existing code to comply with it.
For some classes you have 3 different styles in then, depending on who did the last patch ^^