Merge pull request #1016 from dafioram/star_control-cleanup

TITANIC: Star control cleanup

12 files changed, 47 insertions, 66 deletions

diff --git a/engines/titanic/star_control/camera_auto_mover.cpp b/engines/titanic/star_control/camera_auto_mover.cpp
index fcd1e9e0ee..71f7de85b2 100644
--- a/engines/titanic/star_control/camera_auto_mover.cpp
+++ b/engines/titanic/star_control/camera_auto_mover.cpp
@@ -41,27 +41,7 @@ CCameraAutoMover::CCameraAutoMover() : _srcPos(0.0, 1000000.0, 0.0) {
 	_transitionPercentInc = 0.0;
 }
 
-// TODO: same as setPath also orientations are not used
-void CCameraAutoMover::setPath2(const FVector &oldPos, const FVector &newPos,
-	const FMatrix &oldOrientation, const FMatrix &newOrientation) {
-	_srcPos = oldPos;
-	_destPos = newPos;
-	_posDelta = _destPos - _srcPos;
-
-	float temp = 0.0;
-	_posDelta.normalize(temp); // Do the normalization, put the scale amount in temp
-	_distance = temp;
-	_active = false;
-	_field34 = false;
-	_transitionPercent = 1.0;
-	_field40 = -1;
-	_field44 = -1;
-	_field48 = -1;
-	_field4C = 0;
-}
-
-// TODO: same as proc2 also orientations are not used
-void CCameraAutoMover::setOrientations(const FMatrix &srcOrient, const FMatrix &destOrient) {
+void CCameraAutoMover::clear() {
 	_srcPos.clear();
 	_destPos.clear();
 	_transitionPercent = 1.0;
@@ -70,18 +50,14 @@ void CCameraAutoMover::setOrientations(const FMatrix &srcOrient, const FMatrix &
 	_field34 = false;
 }
 
-// TODO: same as setPath2 also orientations are not used
-void CCameraAutoMover::setPath(const FVector &srcV, const FVector &destV, const FMatrix &orientation) {
+void CCameraAutoMover::setPath(const FVector &srcV, const FVector &destV) {
 	_srcPos = srcV;
 	_destPos = destV;
 	_posDelta = _destPos - _srcPos;
 
 	float temp = 0.0;
-	if (!_posDelta.normalize(temp)) {
-		// Do the normalization, put the scale amount in temp,
-		// but if it is unsuccessful, crash
-		assert(temp);
-	}
+	_posDelta.normalize(temp); // normalization won't happen if _posDelta is zero vector
+								// and that is okay
 
 	_distance = temp;
 	_active = false;
diff --git a/engines/titanic/star_control/camera_auto_mover.h b/engines/titanic/star_control/camera_auto_mover.h
index 25384632fa..9b7eb1b7ce 100644
--- a/engines/titanic/star_control/camera_auto_mover.h
+++ b/engines/titanic/star_control/camera_auto_mover.h
@@ -59,14 +59,15 @@ public:
 	CCameraAutoMover();
 	virtual ~CCameraAutoMover() {}
 
-	virtual void setPath2(const FVector &oldPos, const FVector &newPos,
-		const FMatrix &oldOrientation, const FMatrix &newOrientation);
-
 	/**
 	 * Clear src and dest orientation and set some default values for other fields
 	 */	
-	virtual void setOrientations(const FMatrix &srcOrient, const FMatrix &destOrient);
-	virtual void setPath(const FVector &srcV, const FVector &destV, const FMatrix &orientation);
+	void clear();
+
+	/**
+	 * Setup a transition to from one position to another
+	 */		
+	void setPath(const FVector &srcV, const FVector &destV);
 
 	/**
 	 * Applys speeds to the mover. More than one application is usually done for several transitions
diff --git a/engines/titanic/star_control/marked_auto_mover.cpp b/engines/titanic/star_control/marked_auto_mover.cpp
index 0787c48de3..ae987aa8fc 100644
--- a/engines/titanic/star_control/marked_auto_mover.cpp
+++ b/engines/titanic/star_control/marked_auto_mover.cpp
@@ -26,9 +26,9 @@
 
 namespace Titanic {
 
-void CMarkedAutoMover::setPath2(const FVector &oldPos, const FVector &newPos,
+void CMarkedAutoMover::setPathOrients(const FVector &oldPos, const FVector &newPos,
 	const FMatrix &oldOrientation, const FMatrix &newOrientation) {
-	CCameraAutoMover::setPath2(oldPos, newPos, oldOrientation, newOrientation);
+	CCameraAutoMover::setPath(oldPos, newPos);
 
 	double distance = _distance;
 	_active = true;
diff --git a/engines/titanic/star_control/marked_auto_mover.h b/engines/titanic/star_control/marked_auto_mover.h
index 856df99c03..ca7fbf3b7f 100644
--- a/engines/titanic/star_control/marked_auto_mover.h
+++ b/engines/titanic/star_control/marked_auto_mover.h
@@ -41,7 +41,7 @@ private:
 public:
 	virtual ~CMarkedAutoMover() {}
 
-	virtual void setPath2(const FVector &oldPos, const FVector &newPos,
+	void setPathOrients(const FVector &oldPos, const FVector &newPos,
 		const FMatrix &oldOrientation, const FMatrix &newOrientation);
 
 	/**
diff --git a/engines/titanic/star_control/marked_camera_mover.cpp b/engines/titanic/star_control/marked_camera_mover.cpp
index ff9c055abb..df6edbec5f 100644
--- a/engines/titanic/star_control/marked_camera_mover.cpp
+++ b/engines/titanic/star_control/marked_camera_mover.cpp
@@ -32,13 +32,12 @@ CMarkedCameraMover::CMarkedCameraMover(const CNavigationInfo *src) :
 		CCameraMover(src) {
 }
 
-
 void CMarkedCameraMover::transitionBetweenPosOrients(const FVector &oldPos, const FVector &newPos,
 		const FMatrix &oldOrientation, const FMatrix &newOrientation) {
 	if (isLocked())
 		decLockCount();
 
-	_autoMover.setPath2(oldPos, newPos, oldOrientation, newOrientation);
+	_autoMover.setPathOrients(oldPos, newPos, oldOrientation, newOrientation);
 	incLockCount();
 }
 
diff --git a/engines/titanic/star_control/star_camera.cpp b/engines/titanic/star_control/star_camera.cpp
index cab29a99f4..e4fcdcdead 100644
--- a/engines/titanic/star_control/star_camera.cpp
+++ b/engines/titanic/star_control/star_camera.cpp
@@ -41,7 +41,7 @@ FMatrix *CStarCamera::_newOrientation;
 
 CStarCamera::CStarCamera(const CNavigationInfo *data) :
 		_starLockState(ZERO_LOCKED), _mover(nullptr), _isMoved(false), _isInLockingProcess(false) {
-	setupHandler(data);
+	setMoverType(data);
 }
 
 CStarCamera::CStarCamera(CViewport *src) :
@@ -69,7 +69,7 @@ bool CStarCamera::isNotInLockingProcess() {
 }
 
 CStarCamera::~CStarCamera() {
-	deleteHandler();
+	removeMover();
 }
 
 void CStarCamera::proc2(const CViewport *src) {
@@ -246,7 +246,7 @@ FVector CStarCamera::proc30(int index, const FVector &v) {
 }
 
 FVector CStarCamera::proc31(int index, const FVector &v) {
-	return _viewport.getRelativePosCentering2(index, v);
+	return _viewport.getRelativePosCenteringRaw(index, v);
 }
 
 void CStarCamera::setViewportAngle(const FPoint &angles) {
@@ -391,7 +391,8 @@ void CStarCamera::setViewportAngle(const FPoint &angles) {
 		break;
 	}
 
-	// TODO: should three stars locked do anything in this function? Error?
+	// All three stars are locked on in this case so the camera does not move
+	// in response to the users mouse movements
 	case THREE_LOCKED:
 		break;
 	}
@@ -403,12 +404,12 @@ bool CStarCamera::addLockedStar(const FVector v) {
 
 	CNavigationInfo data;
 	_mover->copyTo(&data);
-	deleteHandler();
+	removeMover();
 
 	FVector &row = _lockedStarsPos[(int)_starLockState];
 	_starLockState = StarLockState((int)_starLockState + 1);
 	row = v;
-	setupHandler(&data);
+	setMoverType(&data);
 	return true;
 }
 
@@ -418,10 +419,10 @@ bool CStarCamera::removeLockedStar() {
 
 	CNavigationInfo data;
 	_mover->copyTo(&data);
-	deleteHandler();
+	removeMover();
 
 	_starLockState = StarLockState((int)_starLockState - 1);
-	setupHandler(&data);
+	setMoverType(&data);
 	return true;
 }
 
@@ -437,7 +438,7 @@ void CStarCamera::save(SimpleFile *file, int indent) {
 	_viewport.save(file, indent);
 }
 
-bool CStarCamera::setupHandler(const CNavigationInfo *src) {
+bool CStarCamera::setMoverType(const CNavigationInfo *src) {
 	CCameraMover *mover = nullptr;
 
 	switch (_starLockState) {
@@ -456,7 +457,7 @@ bool CStarCamera::setupHandler(const CNavigationInfo *src) {
 	}
 
 	if (mover) {
-		assert(!_mover);
+		assert(!_mover); // removeMover() is usually called before this function so _mover is null
 		_mover = mover;
 		return true;
 	} else {
@@ -464,7 +465,7 @@ bool CStarCamera::setupHandler(const CNavigationInfo *src) {
 	}
 }
 
-void CStarCamera::deleteHandler() {
+void CStarCamera::removeMover() {
 	if (_mover) {
 		delete _mover;
 		_mover = nullptr;
@@ -505,7 +506,9 @@ bool CStarCamera::lockMarker1(FVector v1, FVector firstStarPosition, FVector v3)
 
 	FMatrix matrix = _viewport.getOrientation();
 	const FVector &pos = _viewport._position;
-	_mover->transitionBetweenOrientations(v3, tempV, pos, matrix); // TODO: pos does not get used in this function
+	_mover->transitionBetweenOrientations(v3, tempV, pos, matrix); // TODO: pos does not get used in this function, 
+																// i.e., _mover has CUnmarkedCameraMover handle which means
+																// CUnmarkedCameraMover::transitionBetweenOrientations gets called
 
 	CStarVector *sv = new CStarVector(this, firstStarPosition);
 	_mover->setVector(sv);
@@ -613,7 +616,7 @@ bool CStarCamera::lockMarker3(CViewport *viewport, const FVector &thirdStarPosit
 	FMatrix newOr = viewport->getOrientation();
 	FMatrix oldOr = _viewport.getOrientation();
 	FVector newPos = viewport->_position;
-	FVector oldPos = _viewport._position;
+	//FVector oldPos = _viewport._position;
 
 	// WORKAROUND: set old position to new position (1st argument), this prevents 
 	// locking issues when locking the 3rd star. Fixes #9961.
diff --git a/engines/titanic/star_control/star_camera.h b/engines/titanic/star_control/star_camera.h
index 3ffb74950a..f2d27212fe 100644
--- a/engines/titanic/star_control/star_camera.h
+++ b/engines/titanic/star_control/star_camera.h
@@ -47,20 +47,22 @@ private:
 private:
 	StarLockState _starLockState;
 	FMatrix _lockedStarsPos; // Each row represents the location of a locked star
-	CCameraMover *_mover;
+	CCameraMover *_mover; // A marked or unmarked camera mover, contains an automover
 	CViewport _viewport;
-	bool _isMoved; // TODO: determine if this is being used
+	bool _isMoved; // Used in CPetStarfield to determine if a star destination can be set
 	bool _isInLockingProcess; // The mover/view is homing in on a new star
 private:
 	/**
-	 * Set up a handler
+	 * Set Mover type to be unmarked or marked camera mover based on 
+	 * the number of stars currently locked (_starLockState)
+	 * The CNavigationInfo data is used to initialize the mover
 	 */
-	bool setupHandler(const CNavigationInfo *src);
+	bool setMoverType(const CNavigationInfo *src);
 
 	/**
-	 * Deletes any previous handler
+	 * Deletes the previous mover handle
 	 */
-	void deleteHandler();
+	void removeMover();
 
 	/**
 	 * Return whether the handler is locked
diff --git a/engines/titanic/star_control/unmarked_auto_mover.cpp b/engines/titanic/star_control/unmarked_auto_mover.cpp
index c84fbbab00..b8cd042e9c 100644
--- a/engines/titanic/star_control/unmarked_auto_mover.cpp
+++ b/engines/titanic/star_control/unmarked_auto_mover.cpp
@@ -28,7 +28,7 @@
 namespace Titanic {
 
 void CUnmarkedAutoMover::setOrientations(const FMatrix &srcOrient, const FMatrix &destOrient) {
-	CCameraAutoMover::setOrientations(srcOrient, destOrient);
+	CCameraAutoMover::clear();
 	_orientationChanger.load(srcOrient, destOrient);
 	_transitionPercentInc = 0.1;
 	_transitionPercent = 0.0;
@@ -36,8 +36,8 @@ void CUnmarkedAutoMover::setOrientations(const FMatrix &srcOrient, const FMatrix
 	_active = true;
 }
 
-void CUnmarkedAutoMover::setPath(const FVector &srcV, const FVector &destV, const FMatrix &orientation) {
-	CCameraAutoMover::setPath(srcV, destV, orientation);
+void CUnmarkedAutoMover::setPathOrient(const FVector &srcV, const FVector &destV, const FMatrix &orientation) {
+	CCameraAutoMover::setPath(srcV, destV);
 
 	if (_distance > 8000.0) {
 		_active = true;
diff --git a/engines/titanic/star_control/unmarked_auto_mover.h b/engines/titanic/star_control/unmarked_auto_mover.h
index b7fb4e3c66..41c13311e0 100644
--- a/engines/titanic/star_control/unmarked_auto_mover.h
+++ b/engines/titanic/star_control/unmarked_auto_mover.h
@@ -37,9 +37,9 @@ public:
 	virtual void setOrientations(const FMatrix &srcOrient, const FMatrix &destOrient);
 
 	/**
-	 * Sets the path to animate movement between
+	 * Sets the path and starting and ending orientations to animate movement between
 	 */
-	virtual void setPath(const FVector &srcV, const FVector &destV, const FMatrix &orientation);
+	void setPathOrient(const FVector &srcV, const FVector &destV, const FMatrix &orientation);
 
 	virtual MoverState move(CErrorCode &errorCode, FVector &pos, FMatrix &orientation);
 };
diff --git a/engines/titanic/star_control/unmarked_camera_mover.cpp b/engines/titanic/star_control/unmarked_camera_mover.cpp
index 200d549ce1..c879dc25e8 100644
--- a/engines/titanic/star_control/unmarked_camera_mover.cpp
+++ b/engines/titanic/star_control/unmarked_camera_mover.cpp
@@ -41,7 +41,7 @@ void CUnmarkedCameraMover::moveTo(const FVector &srcV, const FVector &destV, con
 
 	debugC(DEBUG_BASIC, kDebugStarfield, "Starfield move %s to %s", srcV.toString().c_str(),
 		destV.toString().c_str());
-	_autoMover.setPath(srcV, destV, orientation);
+	_autoMover.setPathOrient(srcV, destV, orientation);
 }
 
 // TODO: v3 is unused
diff --git a/engines/titanic/star_control/viewport.cpp b/engines/titanic/star_control/viewport.cpp
index d277f4a2c5..e368dfa317 100644
--- a/engines/titanic/star_control/viewport.cpp
+++ b/engines/titanic/star_control/viewport.cpp
@@ -260,8 +260,8 @@ FVector CViewport::getRelativePosCentering(int index, const FVector &src) {
 	return dest;
 }
 
-// TODO: Identical to getRelativePosCentering, was this meant to be different?
-FVector CViewport::getRelativePosCentering2(int index, const FVector &src) {
+// Similar to getRelativePosCentering, but uses the raw/transpose version of Pose
+FVector CViewport::getRelativePosCenteringRaw(int index, const FVector &src) {
 	FVector dest;
 	FPose pose = getRawPose();
 	FVector tv = src.matProdRowVect(pose);
diff --git a/engines/titanic/star_control/viewport.h b/engines/titanic/star_control/viewport.h
index d5c35b6317..082d063233 100644
--- a/engines/titanic/star_control/viewport.h
+++ b/engines/titanic/star_control/viewport.h
@@ -125,7 +125,7 @@ public:
 	FPose getRawPose();
 	FVector getRelativePosNoCentering(int index, const FVector &src);
 	FVector getRelativePosCentering(int index, const FVector &src);
-	FVector getRelativePosCentering2(int index, const FVector &src);
+	FVector getRelativePosCenteringRaw(int index, const FVector &src);
 
 	/**
 	 * All arguments are return values