Split *Options::FromString into FromString and FromOneModString.

Check for invalid mod names when loading a course by checking whether the mod is a valid PlayerOption or a valid SongOption.  Previously, SongOption mods were being flagged as errors by PlayerOptions.

stepmania/src/ActiveAttackList.cpp

@@ -45,7 +45,7 @@ void ActiveAttackList::Refresh()
 			continue;
 
 		PlayerOptions po;
-		po.FromString( attack.sModifiers, true );
+		po.FromString( attack.sModifiers );
 		po.GetLocalizedMods( vsThemedMods );
 	}
 

stepmania/src/CatalogXml.cpp

@@ -381,7 +381,7 @@ void CatalogXml::Save( LoadingWindow *loading_window )
 			{
 				PlayerOptions po;
 				RString s = *iter;
-				po.FromString( s, false );
+				po.FromString( s );
 				vector<RString> v;
 				po.GetLocalizedMods( v );
 				if( v.empty() )

stepmania/src/CourseLoaderCRS.cpp

@@ -13,6 +13,7 @@
 #include "RageFileManager.h"
 #include "CourseWriterCRS.h"
 #include "RageUtil.h"
+#include "CourseUtil.h"
 #include <float.h>
 
 const int MAX_EDIT_COURSE_SIZE_BYTES	= 30*1024;	// 30KB
@@ -130,9 +131,8 @@ bool CourseLoaderCRS::LoadFromMsd( const RString &sPath, const MsdFile &msd, Cou
 						attack.fSecsRemaining = 0.0f;
 					}
 					
-					// warn on invalid so we catch bogus mods on load
+					// warn on invalid so we catch typos on load
-					PlayerOptions po;
+					CourseUtil::WarnOnInvalidMods( attack.sModifiers );
-					po.FromString( attack.sModifiers, true );
 
 					attacks.push_back( attack );
 				}

stepmania/src/CourseUtil.cpp

@@ -11,6 +11,8 @@
 #include "Foreach.h"
 #include "GameState.h"
 #include "LocalizedString.h"
+#include "RageLog.h"
+#include "arch/Dialog/Dialog.h"
 
 
 //
@@ -315,6 +317,29 @@ bool CourseUtil::ValidateEditCourseName( const RString &sAnswer, RString &sError
 	return true;
 }
 
+void CourseUtil::WarnOnInvalidMods( RString sMods )
+{
+	PlayerOptions po;
+	SongOptions so;
+	vector<RString> vs;
+	split( sMods, ",", vs, true );
+	FOREACH_CONST( RString, vs, s )
+	{
+		bool bValid = false;
+		RString sErrorDetail;
+		bValid |= po.FromOneModString( *s, sErrorDetail );
+		bValid |= so.FromOneModString( *s, sErrorDetail );
+		if( !bValid )
+		{
+			RString sFullError = ssprintf("Error processing '%s' in '%s'", (*s).c_str(), sMods.c_str() );
+			if( !sErrorDetail.empty() )
+				sFullError += ": " + sErrorDetail;
+			LOG->UserLog( "", "", "%s", sFullError.c_str() );
+			Dialog::OK( sFullError, "INVALID_PLAYER_OPTION_WARNING" );
+		}
+	}
+}
+
 
 //////////////////////////////////
 // CourseID

stepmania/src/CourseUtil.h

@@ -34,6 +34,8 @@ namespace CourseUtil
 	void AutogenOniFromArtist( const RString &sArtistName, RString sArtistNameTranslit, vector<Song*> aSongs, Difficulty dc, Course &out );
 
 	bool ValidateEditCourseName( const RString &sAnswer, RString &sErrorOut );
+
+	void WarnOnInvalidMods( RString sMods );
 };
 
 class CourseID

stepmania/src/PlayerOptions.cpp

@@ -1,7 +1,6 @@
 #include "global.h"
 #include "PlayerOptions.h"
 #include "RageUtil.h"
-#include "RageLog.h"
 #include "GameState.h"
 #include "NoteSkinManager.h"
 #include "song.h"
@@ -11,7 +10,6 @@
 #include "Foreach.h"
 #include "Style.h"
 #include "CommonMetrics.h"
-#include "arch/Dialog/Dialog.h"
 #include <float.h>
 
 #define ONE( arr ) { for( unsigned Z = 0; Z < ARRAYLEN(arr); ++Z ) arr[Z]=1.0f; }
@@ -232,18 +230,24 @@ void PlayerOptions::GetMods( vector<RString> &AddTo, bool bForceNoteSkin ) const
 
 /* Options are added to the current settings; call Init() beforehand if
  * you don't want this. */
-void PlayerOptions::FromString( const RString &sOptions, bool bWarnOnInvalid )
+void PlayerOptions::FromString( const RString &sMultipleMods )
+{
+	RString sTemp = sMultipleMods;
+	vector<RString> vs;
+	split( sTemp, ",", vs, true );
+	RString sThrowAway;
+	FOREACH( RString, vs, s )
+	{
+		FromOneModString( *s, sThrowAway );
+	}
+}
+
+bool PlayerOptions::FromOneModString( const RString &sOneMod, RString &sErrorOut )
 {
 	ASSERT( NOTESKIN );
-//	Init();
-	RString sTemp = sOptions;
-	sTemp.MakeLower();
-	vector<RString> asBits;
-	split( sTemp, ",", asBits, true );
 
-	FOREACH( RString, asBits, bit )
+	RString sBit = sOneMod;
-	{
+	sBit.MakeLower();
-		RString& sBit = *bit;
 	TrimLeft(sBit);
 	TrimRight(sBit);
 
@@ -269,13 +273,10 @@ void PlayerOptions::FromString( const RString &sOptions, bool bWarnOnInvalid )
 			 * they meant "*123". */
 			if( s->Right(1) == "*" )
 			{
-					if( bWarnOnInvalid )
+				/* XXX We know what they want, is there any reason not to handle it? */
-					{
+				/* Yes.  We should be strict in handling the format. -Chris */
-						LOG->UserLog( "", "", "Invalid player options \"%s\"; did you mean '*%d'?",
+				sErrorOut = ssprintf("Invalid player options \"%s\"; did you mean '*%d'?", s->c_str(), atoi(*s) );
-							      s->c_str(), atoi(*s) );
+				return false;
-					}
-					// XXX We know what they want, is there any reason not to handle it?
-					speed = StringToFloat( *s );
 			}
 			else
 			{
@@ -397,14 +398,10 @@ void PlayerOptions::FromString( const RString &sOptions, bool bWarnOnInvalid )
 	else if( sBit == "random" )				ChooseRandomModifiers();
 	else
 	{
-			if( bWarnOnInvalid )
+		return false;
-			{
-				RString sWarning = ssprintf( "The options string \"%s\" contains an invalid mod name \"%s\".", sOptions.c_str(), sBit.c_str() );
-				LOG->UserLog( "", "", "%s", sWarning.c_str() );
-				Dialog::OK( sWarning, "INVALID_PLAYER_OPTION_WARNING" );
-			}
-		}
 	}
+
+	return true;
 }
 
 void NextFloat( float fValues[], int size )

stepmania/src/PlayerOptions.h

@@ -22,7 +22,8 @@ public:
 	void ResetSavedPrefs();
 	void GetMods( vector<RString> &AddTo, bool bForceNoteSkin = false ) const;
 	void GetLocalizedMods( vector<RString> &AddTo ) const;
-	void FromString( const RString &sOptions, bool bWarnOnInvalid = false );
+	void FromString( const RString &sMultipleMods );
+	bool FromOneModString( const RString &sOneMod, RString &sErrorDetailOut );	// On error, return false and optionally set sErrorDetailOut
 	void ChooseRandomModifiers();
 	bool ContainsTransformOrTurn() const;
 

stepmania/src/SongOptions.cpp

@@ -132,29 +132,40 @@ RString SongOptions::GetLocalizedString() const
 
 /* Options are added to the current settings; call Init() beforehand if
  * you don't want this. */
-void SongOptions::FromString( const RString &sOptions )
+void SongOptions::FromString( const RString &sMultipleMods )
 {
-//	Init();
+	RString sTemp = sMultipleMods;
-	RString sTemp = sOptions;
+	vector<RString> vs;
-	sTemp.MakeLower();
+	split( sTemp, ",", vs, true );
-	vector<RString> asBits;
+	RString sThrowAway;
-	split( sTemp, ",", asBits, true );
+	FOREACH( RString, vs, s )
-
-	for( unsigned i=0; i<asBits.size(); i++ )
 	{
-		RString& sBit = asBits[i];
+		FromOneModString( *s, sThrowAway );
+	}
+}
+
+bool SongOptions::FromOneModString( const RString &sOneMod, RString &sErrorOut )
+{
+	RString sBit = sOneMod;
+	sBit.MakeLower();
 	TrimLeft(sBit);
 	TrimRight(sBit);
 	
 	Regex mult("^([0-9]+(\\.[0-9]+)?)xmusic$");
 	vector<RString> matches;
 	if( mult.Compare(sBit, matches) )
+	{
 		m_fMusicRate = StringToFloat( matches[0] );
+		return true;
+	}
 
 	matches.clear();
 	Regex lives("^([0-9]+) ?(lives|life)$");
 	if( lives.Compare(sBit, matches) )
+	{
 		m_iBatteryLives = atoi( matches[0] );
+		return true;
+	}
 
 	vector<RString> asParts;
 	split( sBit, " ", asParts, true );
@@ -162,7 +173,6 @@ void SongOptions::FromString( const RString &sOptions )
 	if( asParts.size() > 1 )
 	{
 		sBit = asParts[1];
-
 		if( asParts[0] == "no" )
 			on = false;
 	}
@@ -192,7 +202,10 @@ void SongOptions::FromString( const RString &sOptions )
 	else if( sBit == "battery" )		m_LifeType = LIFE_BATTERY;
 	else if( sBit == "lifetime" )		m_LifeType = LIFE_TIME;
 	else if( sBit == "haste" )		m_fHaste = on? 1.0f:0.0f;
-	}
+	else
+		return false;
+
+	return true;
 }
 
 bool SongOptions::operator==( const SongOptions &other ) const

stepmania/src/SongOptions.h

@@ -52,6 +52,7 @@ public:
 	RString GetString() const;
 	RString GetLocalizedString() const;
 	void FromString( const RString &sOptions );
+	bool FromOneModString( const RString &sOneMod, RString &sErrorDetailOut );	// On error, return false and optionally set sErrorDetailOut
 
 	bool operator==( const SongOptions &other ) const;
 	bool operator!=( const SongOptions &other ) const { return !operator==(other); }