KSP-KOS / KOS

Fully programmable autopilot mod for KSP. Originally By Nivekk
Other
694 stars 229 forks source link

Equality operator is broken for wrapped strucure objects. #405

Closed Dunbaratu closed 9 years ago

Dunbaratu commented 9 years ago

See this post in the forums: http://forum.kerbalspaceprogram.com/threads/68089-0-25-kOS-Scriptable-Autopilot-System-v0-15-2-2014-11-19?p=1550395&viewfull=1#post1550395

The problem is caused by the fact that although both objects are Duna, each one is created by doing this under the hood: new BodyTarget( body, shared ), and the equality operator is comparing our own kOS wrappers around the CelestialBody objects, not the CelestialBody objects themselves, and it's claiming they aren't equal. (Which they're not, as far as C# is concerned. Two instances of a class, in which all the members are set to the same thing, do not qualify as "equal" to C#, and they shouldn't.)

We need to fix this by finding a way to make the equality operator work properly in kOS scripts that try to do this.

I see two approaches:

1 - By use of an abstract method defined in the base class, force all our Structure / Suffixed subclasses to have to implement the IEquatable method Equals, so that in all cases whenever C# tries to compare, say, two BodyTargets for the same body, or two VesselTargets for the same vessel, and so on, we can override the behavior and make it call them equal. While we're at it, we could also make it implement the IComparable CompareTo operator to get the right behavior from "<" and ">" and "<=" and ">=".

2 - By use of an abstract method defined in the base class Structure (/Suffixed), force all our Structure/Suffixed subclasses to have to implement our own homemade comparator (while we're at it we may as well make it a full comparator (-1,0,+1 for less, equal, greater)) and then change our implementation of the equals, lessthan, greaterthan, lessequal, geraterequal, operators such that they use our homebrewed comparator when the objects in question are instances of Structure/Suffixed.

Whether to go with approach 1 or approach 2 depends on whether or not we anticipate cases in which we need to differentiate the two instances under the hood in our raw C# code or not. If we do approach 1, not only do we make the instances equal as far as kOS scripts are concerned, we also make them equal as far as our own C# code under the hood is concerned.

Approach 1 elegant and nice because it is guaranteed to catch all cases with no possibility of any of them slipping through the cracks because we missed a spot in our code, but it may catch too many cases, so to speak. I don't know.

Discuss.

erendrake commented 9 years ago

I would rather go through and override the equals/gethashcode of the structures that we want to do value equality, I just went through the list and the structures that seemed to need it most would be

Dunbaratu commented 9 years ago

merged pull reqest seems to have fixed it.