Introducing read-only interface for data types

Sometimes, I have to look into the method implementation when I call method.

For example, when I call constructor Ray(Vector3f origin, Vector3f direction).

the parameter instances are stored in Ray instance then it is possible to be accessed and modified elsewhere

using getDirection() / getOrigin() method.



That’s why I constantly insist on using read-only interface.

If it is accepted ( Please~~~ hehe :roll: ) then I’m willing to volunteer to implement it.

Because it will change very very many lines of code, if we don’t do this now then never until jme4.

That’s tragedy for me and jme engine.



If there is the read-only interface, class signature of Ray will be changed like this.

Ray(Vector3f origin, Vector3f direction) is converted to Ray(IVector3f origin, IVector3f direction)

public Vector3f getDirection() → public IVector3f getDirection()

public Vector3f getOrigin() → public IVector3f getOrigin()

Then user confirms that no dangerous situation may occur.

Application’s data will not be changed in the engine core and

engine’s data will not be changed by application.



Comments more than welcome.



Below is the example of Read-Only Vector3f interface.

[java]

package com.jme3.math;



public interface IVector3f {

/**

  • <code>add</code> adds a provided vector to this vector creating a
  • resultant vector which is returned. If the provided vector is null, null
  • is returned.

    *
  • @param vec
  •        the vector to add to this.<br />
    
  • @return the resultant vector.

    */

    public Vector3f add(IVector3f vec);



    /**

    *
  • <code>add</code> adds the values of a provided vector storing the
  • values in the supplied vector.

    *
  • @param vec
  •        the vector to add to this<br />
    
  • @param result
  •        the vector to store the result in<br />
    
  • @return result returns the supplied result vector.

    */

    public Vector3f add(IVector3f vec, Vector3f result);



    /**

    *
  • <code>add</code> adds the provided values to this vector, creating a
  • new vector that is then returned.

    *
  • @param addX
  •        the x value to add.<br />
    
  • @param addY
  •        the y value to add.<br />
    
  • @param addZ
  •        the z value to add.<br />
    
  • @return the result vector.

    */

    public Vector3f add(float addX, float addY, float addZ);



    /**

    *
  • <code>scaleAdd</code> multiplies this vector by a scalar then adds the
  • given Vector3f.

    *
  • @param scalar
  •        the value to multiply this vector by.<br />
    
  • @param add
  •        the value to add<br />
    

*/

public Vector3f scaleAdd(float scalar, IVector3f add);



/**

*

  • <code>scaleAdd</code> multiplies the given vector by a scalar then adds
  • the given vector.

    *
  • @param scalar
  •        the value to multiply this vector by.<br />
    
  • @param mult
  •        the value to multiply the scalar by<br />
    
  • @param add
  •        the value to add<br />
    

*/

public Vector3f scaleAdd(float scalar, IVector3f mult, IVector3f add);



/**

*

  • <code>dot</code> calculates the dot product of this vector with a
  • provided vector. If the provided vector is null, 0 is returned.

    *
  • @param vec
  •        the vector to dot with this vector.<br />
    
  • @return the resultant dot product of this vector and a given vector.

    */

    public float dot(IVector3f vec);



    /**
  • <code>cross</code> calculates the cross product of this vector with a
  • parameter vector v.

    *
  • @param v
  •        the vector to take the cross product of with this.<br />
    
  • @return the cross product vector.

    */

    public Vector3f cross(IVector3f v);



    /**
  • <code>cross</code> calculates the cross product of this vector with a
  • parameter vector v. The result is stored in <code>result</code>

    *
  • @param v
  •        the vector to take the cross product of with this.<br />
    
  • @param result
  •        the vector to store the cross product result.<br />
    
  • @return result, after recieving the cross product vector.

    */

    public Vector3f cross(IVector3f v,Vector3f result);



    /**
  • <code>cross</code> calculates the cross product of this vector with a
  • parameter vector v. The result is stored in <code>result</code>

    *
  • @param otherX
  •        x component of the vector to take the cross product of with this.<br />
    
  • @param otherY
  •        y component of the vector to take the cross product of with this.<br />
    
  • @param otherZ
  •        z component of the vector to take the cross product of with this.<br />
    
  • @param result
  •        the vector to store the cross product result.<br />
    
  • @return result, after recieving the cross product vector.

    */

    public Vector3f cross(float otherX, float otherY, float otherZ, Vector3f result);





    public Vector3f project(IVector3f other);



    /**
  • <code>length</code> calculates the magnitude of this vector.

    *
  • @return the length or magnitude of the vector.

    */

    public float length();



    /**
  • <code>lengthSquared</code> calculates the squared value of the
  • magnitude of the vector.

    *
  • @return the magnitude squared of the vector.

    */

    public float lengthSquared();



    /**
  • <code>distanceSquared</code> calculates the distance squared between
  • this vector and vector v.

    *
  • @param v the second vector to determine the distance squared.
  • @return the distance squared between the two vectors.

    */

    public float distanceSquared(IVector3f v);



    /**
  • <code>distance</code> calculates the distance between this vector and
  • vector v.

    *
  • @param v the second vector to determine the distance.
  • @return the distance between the two vectors.

    */

    public float distance(IVector3f v);



    /**

    *
  • <code>mult</code> multiplies this vector by a scalar. The resultant
  • vector is returned.

    *
  • @param scalar
  •        the value to multiply this vector by.<br />
    
  • @return the new vector.

    */

    public Vector3f mult(float scalar);



    /**

    *
  • <code>mult</code> multiplies this vector by a scalar. The resultant
  • vector is supplied as the second parameter and returned.

    *
  • @param scalar the scalar to multiply this vector by.
  • @param product the product to store the result in.
  • @return product

    */

    public Vector3f mult(float scalar, IVector3f product);



    /**
  • <code>multLocal</code> multiplies a provided vector to this vector
  • internally, and returns a handle to this vector for easy chaining of
  • calls. If the provided vector is null, null is returned.

    *
  • @param vec
  •        the vector to mult to this vector.<br />
    
  • @return this

    */

    public Vector3f mult(IVector3f vec);



    /**
  • <code>multLocal</code> multiplies a provided vector to this vector
  • internally, and returns a handle to this vector for easy chaining of
  • calls. If the provided vector is null, null is returned.

    *
  • @param vec
  •        the vector to mult to this vector.<br />
    
  • @param store result vector (null to create a new vector)
  • @return this

    */

    public Vector3f mult(IVector3f vec, Vector3f store);





    /**
  • <code>divide</code> divides the values of this vector by a scalar and
  • returns the result. The values of this vector remain untouched.

    *
  • @param scalar
  •        the value to divide this vectors attributes by.<br />
    
  • @return the result <code>Vector</code>.

    */

    public Vector3f divide(float scalar);





    /**
  • <code>divide</code> divides the values of this vector by a scalar and
  • returns the result. The values of this vector remain untouched.

    *
  • @param scalar
  •        the value to divide this vectors attributes by.<br />
    
  • @return the result <code>Vector</code>.

    */

    public Vector3f divide(IVector3f scalar);



    /**

    *
  • <code>negate</code> returns the negative of this vector. All values are
  • negated and set to a new vector.

    *
  • @return the negated vector.

    */

    public Vector3f negate();



    /**

    *
  • <code>subtract</code> subtracts the values of a given vector from those
  • of this vector creating a new vector object. If the provided vector is
  • null, null is returned.

    *
  • @param vec
  •        the vector to subtract from this vector.<br />
    
  • @return the result vector.

    */

    public Vector3f subtract(IVector3f vec);



    /**

    *
  • <code>subtract</code>

    *
  • @param vec
  •        the vector to subtract from this<br />
    
  • @param result
  •        the vector to store the result in<br />
    
  • @return result

    */

    public Vector3f subtract(IVector3f vec, Vector3f result);



    /**

    *
  • <code>subtract</code> subtracts the provided values from this vector,
  • creating a new vector that is then returned.

    *
  • @param subtractX
  •        the x value to subtract.<br />
    
  • @param subtractY
  •        the y value to subtract.<br />
    
  • @param subtractZ
  •        the z value to subtract.<br />
    
  • @return the result vector.

    */

    public Vector3f subtract(float subtractX, float subtractY, float subtractZ);



    /**
  • <code>normalize</code> returns the unit vector of this vector.

    *
  • @return unit vector of this vector.

    */

    public Vector3f normalize();





    /**
  • <code>angleBetween</code> returns (in radians) the angle between two vectors.
  • It is assumed that both this vector and the given vector are unit vectors (iow, normalized).

    *
  • @param otherVector a unit vector to find the angle against
  • @return the angle in radians.

    */

    public float angleBetween(IVector3f otherVector);



    /**
  • Saves this Vector3f into the given float[] object.

    *
  • @param floats
  •        The float[] to take this Vector3f. If null, a new float[3] is<br />
    
  •        created.<br />
    
  • @return The array, with X, Y, Z float values in that order

    */

    public float[] toArray(float[] floats);



    public float getX();



    public float getY();



    public float getZ();





    /**
  • @param index
  • @return x value if index == 0, y value if index == 1 or z value if index ==
  •     2<br />
    
  • @throws IllegalArgumentException
  •         if index is not one of 0, 1, 2.<br />
    

*/

public float get(int index);



}



[/java]

Well there is an issue in the tracker about that

http://code.google.com/p/jmonkeyengine/issues/detail?id=203&q=label:Product-jME3&sort=priority&colspec=ID%20Type%20Status%20Component%20Priority%20Product%20Milestone%20Owner%20Summary



Since it’s accepted, i guess at least Kirill agree :stuck_out_tongue:



On my side, i’m not really fond of the idea… If you’re going there not only Vector3f must be changed, quaternions, Vector2f etc too…

I mean this is a huge change…

What append to the multLocal, addLocal methods then? using mult and add instead will result in a lot more variable instancing…



Don’t know

How changing constructor to Ray(IVector3f origin, IVector3f direction) is going to help anything ?



[java]

Vector3f vec = new Vector(1,2,3);

new Ray(vec,vec);

vec.x = 5;

[/java]



I can understand the argument about the getters, but for setters/constructors, readonly-interface is not going to help you, as you need to handle real class anyway around that.

nehon said:
What append to the multLocal, addLocal methods then? using mult and add instead will result in a lot more variable instancing...
Don't know

***Local() method will not be included in the read-only interface because it changes the instance's internal data.
But these method is still alive in Vector3f.
Read-only interface gives a new fuctionality but does not take away old functionalites.
Ss it won't affect users at all. They can call xxxLocal() series as before.
It means "I'll let you know If you get read-only data, you should know you can't change it."
Actually, not many instancing is necessary but just copying occurs.
Instead of copying overhead, it makes the engine reliable and programmers don't need to worry about it.

abies said:
How changing constructor to Ray(IVector3f origin, IVector3f direction) is going to help anything ?

[java]
Vector3f vec = new Vector(1,2,3);
new Ray(vec,vec);
vec.x = 5;
[/java]

I can understand the argument about the getters, but for setters/constructors, readonly-interface is not going to help you, as you need to handle real class anyway around that.

The constructor of Ray will copy the vector data to inner vector.
So vec.x = 5 will not modify the Ray's inner data. That's my point.
And your example is the one of the problems what I want to cure.

Read-only interface doesn't mean elimination of real class requirement.
It means "I will use actual instance type, but if you want to use it, do not modify it".
Read-only interface will provide one step forward to separation of engine and application's data coupling.

I’m mroe for a javax.vecmath like interface



So the

Quaternion getRotation()

has a readonly:

Quaternion getRotation(Quaternion store)

That way it is impossible to get cross references



This change can go without changing any existing function just by adding the new ones.

So if I use the new ones then I’m absolutly sure that i never has any references from somewhere.

mulova said:
The constructor of Ray will copy the vector data to inner vector. So vec.x = 5 will not modify the Ray's inner data.

But we can do that without a read only interface.

I'd like to know Normen's and Kirill's opinions about that.

Don't get me wrong, those are valid points, but the change seems huge for little gain IMO.
nehon said:
But we can do that without a read only interface.

I'd like to know Normen's and Kirill's opinions about that.

Don't get me wrong, those are valid points, but the change seems huge for little gain IMO.

Thank you. I won't stick to my opinion.
If there is a better way, I'll support it.
Until now, I don't have a better idea.

Anyway, get() methods such as Ray.getDirection() need read only interface or @EmpirePhoenix 's way.
If we use read-only interface, copying overhead will be lower.

EmpirePhoenix said:
I'm mroe for a javax.vecmath like interface

So the
Quaternion getRotation()
has a readonly:
Quaternion getRotation(Quaternion store)
That way it is impossible to get cross references

This change can go without changing any existing function just by adding the new ones.
So if I use the new ones then I'm absolutly sure that i never has any references from somewhere.

I think the existing method should be removed if we add new method like that.
If not, the vulnerability still exists.

Frankly speaking, this is actually nothing at all for guru.
But we can't work alone when developing games, especially large scale games.
Someone makes a hole in the wall.

When I develop my game, I suffered a instance reference problem like above.
Actually it was too hard to track the bug, so I applied read-only interface in my local version of jme2.
After that time, I don't need to worry about reference related problem at all whoever modifies my code.
It is very trivial issue. But when we met this problem, it is horrible.

When I first met Ardor3D, what made me happy were read-only interface and visitor pattern for scene graph.
These two had been very useful from my experience and I thought Ardor3D people knew it because they experienced in the same way.
Now I choose jme3 because there are another powerful features than Ardor3D.
But if jme3 accepts the good things from Ardor3D, I will be happy and eventually all the people will be happier.

I promise I'll continue to keep track of Ardor3D and make jme people annoying. :roll:
mulova said:
When I develop my game, I suffered a instance reference problem like above.
Actually it was too hard to track the bug, so I applied read-only interface in my local version of jme2.
After that time, I don't need to worry about reference related problem at all whoever modifies my code.
It is very trivial issue. But when we met this problem, it is horrible.

This is a very valid point, this part convinced me.
You are the one here with the real game development experience, and I believe you when you say that those kind of issues are a nightmare to track.

We could add some kind of scanning to jMP that warns you when you set the value of a Vector3f etc. constant if thats the main problem. (you know I dont like the idea of readonly wrappers ;)).

normen said:
We could add some kind of scanning to jMP that warns you when you set the value of a Vector3f etc. constant if thats the main problem. (you know I dont like the idea of readonly wrappers ;)).

Thank you for your kindness. :)
But you don't need to do that. I don't think it helps.
I'll maintain local version of jme3 instead.

Anyway, read-only is not wrapper. It is just instance itself.

May I create a branch on svn about this issue?

I do think its a good idea.

So, how much code will break if we do this?

This kind of change should be mostly smooth, right?

Momoko_Fan said:
I do think its a good idea.
So, how much code will break if we do this?
This kind of change should be mostly smooth, right?

For my experience, it changes very very many code.
But it doesn't break code much.
If there is a compile error or it needs code change, It means there is a vulnerability hole.
We can find it by this work.

If allowed, I'd like to make a branch and work there.
If it is not proper and different from what people imagine, just drop it.

We could add some kind of scanning to jMP



Well while this could help, I think it’s the wrong way. If we have problems with our interface we should fix it instead of building a workaround that only helps jmp users.

What about approching it from different side - like EmpirePhoenix suggested,.



Let’s change all setters/constructors in jme3 to always copy the value (into internal holder, doesn’t have to allocate new objects). Make all getters with two versions, one of them with output paramter (no new object allocated) and second just returning a value (which will always do a copy).



It will be fully compatible with existing code and fully safe. It might require some changes for efficiency sake (things calling getLocalTranslation() may want to call getLocalTranslation(holder) to avoid object creation), but this can be done incrementally.



No new interfaces required, no code breakage, you can still be as efficient as you want, no way of destroying the things by casting interface to mutable class or passing value to setter expecting read-only version, this setter storing the value and you then modifying it through real class pointer. And best thing - all can be done incrementally, no need to do it all at once.

Yeah, that incrementally was why I suggested to not remove those unsafe methods for now.

Of course that can and should be done once everything is finsihed.



As a sidenode, jbullet uses that way as well, so I kinda think it’s not really much of a performance issue if we only have to copy the values and not create new Objects.

My suggestion would be not remove them ever, just convert them to return new objects. Slight performance loss instead of incompatibility - and in some cases, you will want new object anyway to process it further, so it saves one line of code for creating it and passing to ‘performant’ getter.

I prefer read-only interface but support whatever methodology fulfilling the data separation.

No, new objects are death to performance really when we talk about at least 60fps! The getters shouldnt return new objects, the only way its used right now is like setLocalTanslation(getLocalTranslation.set(newVector)); to use the vector like a temp vector in some places.

Well new obects are less a fps problem but more a garbage collect problem dependin on machine we could loose fps or we could have hard lags for a few second every once in a while. (Contious garbage collection vs full collect)



Thats why I suggest to pass the referecen where the values should be copied to. Normally you need if at all very few object creations while running that way.