Unexpected parameter value change

DirecitonalLight.setDirection() changes the parameter value.

And this code computes same computation two times.

[patch]

Index: src/core/com/jme3/light/DirectionalLight.java

===================================================================

— src/core/com/jme3/light/DirectionalLight.java (revision 6741)

+++ src/core/com/jme3/light/DirectionalLight.java (working copy)

@@ -65,12 +65,10 @@

  • (1f, 0, 0) would represent a directional light coming from the X axis.

    */

    public void setDirection(Vector3f dir){
  •    if (!dir.isUnitVector()) {<br />
    
  •        dir.normalizeLocal();<br />
    
  •    }<br />
    

-

-

direction.set(dir);

  •    if (!direction.isUnitVector()) {<br />
    
  •        direction.normalizeLocal();<br />
    
  •    }<br />
    

}



@Override

[/patch]



In Vector3f.isUnitVetor(), length() is computed and if it is really non unit vector, length() is computed one more time to set unit vector.

I think it is better to have an ‘isUnit’ boolean value instead in Vector3f

1 Like

Using boolean variable for Vector3f.isUnitVector().

Using this, sqrt operation could be reduced and this is more precise than current implementation.



[patch]

Index: src/core/com/jme3/math/Vector3f.java

===================================================================

— src/core/com/jme3/math/Vector3f.java (revision 6741)

+++ src/core/com/jme3/math/Vector3f.java (working copy)

@@ -72,6 +72,7 @@

Float.NEGATIVE_INFINITY,

Float.NEGATIVE_INFINITY);


  • private boolean normalized;



    /**
  • the x value of the vector.

    @@ -139,6 +140,7 @@

    this.x = x;

    this.y = y;

    this.z = z;
  •    this.normalized = false;<br />
    

return this;

}



@@ -154,6 +156,7 @@

this.x = vect.x;

this.y = vect.y;

this.z = vect.z;

  •    this.normalized = vect.normalized;<br />
    

return this;

}



@@ -190,6 +193,7 @@

result.x = x + vec.x;

result.y = y + vec.y;

result.z = z + vec.z;

  •    result.normalized = false;<br />
    

return result;

}



@@ -210,6 +214,7 @@

x += vec.x;

y += vec.y;

z += vec.z;

  •    normalized = false;<br />
    

return this;

}



@@ -247,6 +252,7 @@

x += addX;

y += addY;

z += addZ;

  •    normalized = false;<br />
    

return this;

}



@@ -264,6 +270,7 @@

x = x * scalar + add.x;

y = y * scalar + add.y;

z = z * scalar + add.z;

  •    normalized = false;<br />
    

return this;

}



@@ -283,6 +290,7 @@

this.x = mult.x * scalar + add.x;

this.y = mult.y * scalar + add.y;

this.z = mult.z * scalar + add.z;

  •    this.normalized = false;<br />
    

return this;

}



@@ -349,6 +357,7 @@

float resY = ((z * otherX) - (x * otherZ));

float resZ = ((x * otherY) - (y * otherX));

result.set(resX, resY, resZ);

  •    result.normalized = false;<br />
    

return result;

}



@@ -382,6 +391,7 @@

z = (x * otherY) - (y * otherX);

x = tempx;

y = tempy;

  •    normalized = false;<br />
    

return this;

}



@@ -399,8 +409,7 @@

  • or false otherwise.

    */

    public boolean isUnitVector(){
  •    float len = length();<br />
    
  •    return 0.99f &lt; len &amp;&amp; len &lt; 1.01f;<br />
    
  •    return normalized;<br />
    

}



/**

@@ -409,6 +418,8 @@

  • @return the length or magnitude of the vector.

    */

    public float length() {
  •    if (normalized)<br />
    
  •        return 1;<br />
    

return FastMath.sqrt(lengthSquared());

}



@@ -419,6 +430,8 @@

  • @return the magnitude squared of the vector.

    */

    public float lengthSquared() {
  •    if (normalized)<br />
    
  •       return 1;<br />
    

return x * x + y * y + z * z;

}



@@ -477,6 +490,7 @@

product.x = x * scalar;

product.y = y * scalar;

product.z = z * scalar;

  •    product.normalized = false;<br />
    

return product;

}



@@ -492,6 +506,7 @@

x *= scalar;

y *= scalar;

z *= scalar;

  •    normalized = false;<br />
    

return this;

}



@@ -512,6 +527,7 @@

x *= vec.x;

y *= vec.y;

z *= vec.z;

  •    normalized = false;<br />
    

return this;

}



@@ -529,6 +545,7 @@

this.x *= x;

this.y *= y;

this.z *= z;

  •    this.normalized = false;<br />
    

return this;

}



@@ -596,6 +613,7 @@

x *= scalar;

y *= scalar;

z *= scalar;

  •    normalized = false;<br />
    

return this;

}



@@ -625,6 +643,7 @@

x /= scalar.x;

y /= scalar.y;

z /= scalar.z;

  •    normalized = false;<br />
    

return this;

}



@@ -683,6 +702,7 @@

x -= vec.x;

y -= vec.y;

z -= vec.z;

  •    normalized = false;<br />
    

return this;

}



@@ -703,6 +723,7 @@

result.x = x - vec.x;

result.y = y - vec.y;

result.z = z - vec.z;

  •    result.normalized = false;<br />
    

return result;

}



@@ -740,6 +761,7 @@

x -= subtractX;

y -= subtractY;

z -= subtractZ;

  •    normalized = false;<br />
    

return this;

}



@@ -755,12 +777,9 @@

// }

//

// return divide(1);

  •    float length = x * x + y * y + z * z;<br />
    
  •    if (length != 1f &amp;&amp; length != 0f){<br />
    
  •        length = 1.0f / FastMath.sqrt(length);<br />
    
  •        return new Vector3f(x * length, y * length, z * length);<br />
    
  •    }<br />
    
  •    return clone();<br />
    
  •    Vector3f norm = new Vector3f(this);<br />
    
  •    norm.normalizeLocal();<br />
    
  •    return norm;<br />
    

}



/**

@@ -773,12 +792,13 @@

// NOTE: this implementation is more optimized

// than the old jme normalize as this method

// is commonly used.

  •    float length = x * x + y * y + z * z;<br />
    
  •    if (length != 1f &amp;&amp; length != 0f){<br />
    
  •    if (!normalized &amp;&amp; (x!=0 || y!=0 || z!=0)) {<br />
    
  •        float length = x * x + y * y + z * z;<br />
    

length = 1.0f / FastMath.sqrt(length);

x *= length;

y *= length;

z *= length;

  •        normalized = true;<br />
    

}

return this;

}

@@ -793,6 +813,7 @@

x = other.x > x ? other.x : x;

y = other.y > y ? other.y : y;

z = other.z > z ? other.z : z;

  •    normalized = false;<br />
    

}



/**

@@ -805,6 +826,7 @@

x = other.x < x ? other.x : x;

y = other.y < y ? other.y : y;

z = other.z < z ? other.z : z;

  •    normalized = false;<br />
    

}



/**

@@ -812,6 +834,7 @@

*/

public Vector3f zero() {

x = y = z = 0;

  •    normalized = false;<br />
    

return this;

}



@@ -839,6 +862,7 @@

this.x=(1-changeAmnt)this.x + changeAmntfinalVec.x;

this.y=(1-changeAmnt)this.y + changeAmntfinalVec.y;

this.z=(1-changeAmnt)this.z + changeAmntfinalVec.z;

  •    normalized = false;<br />
    

return this;

}



@@ -854,6 +878,7 @@

this.x=(1-changeAmnt)beginVec.x + changeAmntfinalVec.x;

this.y=(1-changeAmnt)beginVec.y + changeAmntfinalVec.y;

this.z=(1-changeAmnt)beginVec.z + changeAmntfinalVec.z;

  •    normalized = false;<br />
    

return this;

}



@@ -939,6 +964,7 @@

  •        the object to compare for equality<br />
    
  • @return true if they are equal

    */
  • @Override

    public boolean equals(Object o) {

    if (!(o instanceof Vector3f)) { return false; }



    @@ -957,6 +983,7 @@
  • the same hash code value.
  • @return the hash code value of this vector.

    */
  • @Override

    public int hashCode() {

    int hash = 37;

    hash += 37 * hash + Float.floatToIntBits(x);

    @@ -973,10 +1000,12 @@

    *
  • @return the string representation of this vector.

    */
  • @Override

    public String toString() {

    return "(" + x + ", " + y + ", " + z + ")";

    }


  • @Override

    public void write(JmeExporter e) throws IOException {

    OutputCapsule capsule = e.getCapsule(this);

    capsule.write(x, "x", 0);

    @@ -984,6 +1013,7 @@

    capsule.write(z, "z", 0);

    }


  • @Override

    public void read(JmeImporter e) throws IOException {

    InputCapsule capsule = e.getCapsule(this);

    x = capsule.readFloat("x", 0);

    @@ -997,6 +1027,7 @@



    public Vector3f setX(float x) {

    this.x = x;
  •    this.normalized = false;<br />
    

return this;

}



@@ -1006,6 +1037,7 @@



public Vector3f setY(float y) {

this.y = y;

  •    this.normalized = false;<br />
    

return this;

}



@@ -1015,6 +1047,7 @@



public Vector3f setZ(float z) {

this.z = z;

  •    this.normalized = false;<br />
    

return this;

}



@@ -1046,6 +1079,7 @@

  •         if index is not one of 0, 1, 2.<br />
    

*/

public void set(int index, float value) {

  •    this.normalized = false;<br />
    

switch (index) {

case 0:

x = value;

@@ -1059,5 +1093,4 @@

}

throw new IllegalArgumentException("index must be either 0, 1 or 2");

}

-

}



[/patch]

I think that adding a boolean value is not worth it in this case.



Due to padding, the number of bytes a vector takes will increase from 20 bytes to 24 bytes, a 20% increase.

The optimization you’re getting in terms of computation is minimal, how many times is “isUnitVector()” called? The same number of times as DirectionalLight.setDirection(), which is not very often as in most cases, users will set the light direction once for the entire run time of the application, and the liberal maximum number of times would be a few per frame, which is definitely isn’t worth a 20% memory usage increase.



Even if you don’t store many vectors in memory, you still have to allocate them and deallocate them at times, unless you use temp vectors a lot, which isn’t likely in most users’ projects.

okay. then drop the second suggestion.

what should I do for the first patch?

and the current isUnitVector() seems not precise enough.

Yes, the first patch should be applied. The vector given by the user should not be modified

Thanks!

committed. rev. 6754