[Kstars-devel] [kstars] kstars/skyobjects: Make SkyObject::recomputeCoords() const by recomputing on a clone.

[Akarsh Simha]

Git commit d572b2e8aef73a8e3e152b0d15304f7f046a7f27 by Akarsh Simha.
Committed on 18/01/2015 at 09:41.
Pushed by asimha into branch 'master'.

Make SkyObject::recomputeCoords() const by recomputing on a clone.

NOTE: This has the possibility of introducing bugs, although I expect
it to solve some hard-to-trace bugs. Hence, it would be good if the
patch were checked by multiple eyes. (Hence copying the mailing list)

The problem:
------------

SkyObject::recomputeCoords() is a method heavily used within SkyObject
and outside to compute the coordinates of a SkyObject at a different
time and location and return a SkyPoint corresponding to these
coordinates, without altering the coordinates of the object itself.

The old implementation of SkyObject::recomputeCoords() did the
following:
1. Save the current coordinates (RA/Dec) of the object
2. Compute coordinates for new context (i.e. location + time)
3. Put back the original coordinates of the object

SkyObject::updateCoords() is invoked to compute the coordinates for
the new context.

However, this can lead to many bugs, since the coordinates are not the
only context-dependent variables that are change. For example,
KSPlanetBase::updateCoords() may update not only the coordinates, but
also the magnitiude, angular size, phase and other parameters of the
planet. So this could lead to bugs where the moon's phase suddenly
changes because of some other computation or the like! (I have noticed
things like this happen earlier)

The solution:
-------------

The obvious solution is to make a true copy (this might make KStars a
bit slow, but the overhead is substantial only for the planets which
are few in number) and do the recomputation on the copy, i.e.
1. Make a copy of the SkyObject
2. Compute the coordinates for the next context on the copy
3. Assign the coordinates to a SkyPoint and return that

However, the solution is not very straightforward, since we cannot
copy just the SkyObject -- we would slice KSPlanet, for
example. Thankfully, Alexey implemented clone() methods in SkyObject
and its subclasses, allowing for true copies. (See previous commits)

Other advantages:
-----------------

Apart from the advantage of (hopefully) solving bugs with respect to
properties of planets being altered, another good side-effect of this
is that the method can be constified as it should be, and so can a
number of methods that use this (see future commits). This helps
ensure that tools don't modify the instances of SkyObject, so the
display on the SkyMap is not artificially altered.

CCMAIL: kstars-devel at kde.org

M  +20   -12   kstars/skyobjects/skyobject.cpp
M  +2    -2    kstars/skyobjects/skyobject.h

http://commits.kde.org/kstars/d572b2e8aef73a8e3e152b0d15304f7f046a7f27

diff --git a/kstars/skyobjects/skyobject.cpp b/kstars/skyobjects/skyobject.cpp
index 165d62d..2f29444 100644
--- a/kstars/skyobjects/skyobject.cpp
+++ b/kstars/skyobjects/skyobject.cpp
@@ -295,27 +295,35 @@ dms SkyObject::elevationCorrection(void) {
         return dms(-0.5667);
 }
 
-SkyPoint SkyObject::recomputeCoords( const KStarsDateTime &dt, const GeoLocation *geo ) {
-    //store current position
-    SkyPoint original = *this;
+SkyPoint SkyObject::recomputeCoords( const KStarsDateTime &dt, const GeoLocation *geo ) const {
 
-    // compute coords for new time jd
+    // Create a clone
+    SkyObject *c = this->clone();
+
+    // compute coords of the copy for new time jd
     KSNumbers num( dt.djd() );
+
+    // Note: isSolarSystem() below should give the same result on this
+    // and c. The only very minor reason to prefer this is so that we
+    // have an additional layer of warnings about subclasses of
+    // KSPlanetBase that do not implement SkyObject::clone() due to
+    // the passing of lat and LST
+
     if ( isSolarSystem() && geo ) {
         dms LST = geo->GSTtoLST( dt.gst() );
-        updateCoords( &num, true, geo->lat(), &LST );
+        c->updateCoords( &num, true, geo->lat(), &LST );
     } else {
-        updateCoords( &num );
+        c->updateCoords( &num );
     }
 
-    //the coordinates for the date dt:
-    SkyPoint sp = *this;
+    // Transfer the coordinates into a SkyPoint
+    SkyPoint p = *c;
 
-    // restore original coords
-    setRA( original.ra().Hours() );
-    setDec( original.dec().Degrees() );
+    // Delete the clone
+    delete c;
 
-    return sp;
+    // Return the SkyPoint
+    return p;
 }
 
 QString SkyObject::typeName( int t ) {
diff --git a/kstars/skyobjects/skyobject.h b/kstars/skyobjects/skyobject.h
index f86fc8b..71bd7fc 100644
--- a/kstars/skyobjects/skyobject.h
+++ b/kstars/skyobjects/skyobject.h
@@ -254,12 +254,12 @@ public:
 
     /**
      *The coordinates for the object on date dt are computed and returned,
-     *but the object's internal coordinates are not permanently modified.
+     *but the object's internal coordinates are not modified.
      *@return the coordinates of the selected object for the time given by jd
      *@param dt  date/time for which the coords will be computed.
      *@param geo pointer to geographic location (used for solar system only)
      */
-    SkyPoint recomputeCoords( const KStarsDateTime &dt, const GeoLocation *geo=0 );
+    SkyPoint recomputeCoords( const KStarsDateTime &dt, const GeoLocation *geo=0 ) const;
 
     inline bool hasName() const { return ! Name.isEmpty(); }