Implement changes from feedback

src/Group.cpp

@@ -37,7 +37,7 @@ Group::Group() {
 }
 
 Group::~Group() {
-    SONGMAN->GetGroupGroupMap().clear();
+    SONGMAN->GetGroupGroupMap().erase(m_sGroupName);
 }
 
 Group::Group(const RString& sDir, const RString& sGroupDirName) {
@@ -48,29 +48,47 @@ Group::Group(const RString& sDir, const RString& sGroupDirName) {
     RString sSeries = "";
     RString sBannerPath = "";
     float fOffset = PREFSMAN->m_fMachineSyncBias;
+    m_iVersion = INI_VERSION;
     
     if (FILEMAN->DoesFileExist(sPackIniPath)) {
         IniFile ini;
         ini.ReadFile(sPackIniPath);
+
+        RString sVersion = "";
+        ini.GetValue("Group", "Version", sVersion);
+        Trim(sVersion);
+
+        // Only read the Pack.ini if the version is set
+        // Otherwise log a warning and skip reading the 
+        if (!sVersion.empty()) {
+            m_bHasPackIni = true;
+            m_iVersion = StringToInt(sVersion);
+            
             ini.GetValue("Group", "DisplayTitle", sDisplayTitle);
             Trim(sDisplayTitle);
             if (sDisplayTitle.empty()) {
                 sDisplayTitle = Basename(sGroupDirName);
             }
             ini.GetValue("Group", "Banner", sBannerPath);
+            if (sBannerPath.empty()) {
+                m_sBannerPath = sBannerPath;
+            }
+
             ini.GetValue("Group", "SortTitle", sSortTitle);
             Trim(sSortTitle);
             if (sSortTitle.empty()) {
                 sSortTitle = sDisplayTitle;
             }
+            
             ini.GetValue("Group", "TranslitTitle", sTranslitTitle);
             Trim(sTranslitTitle);
             if (sTranslitTitle.empty()) {
                 sTranslitTitle = sDisplayTitle;
             }
-        ini.GetValue("Group", "Series", sSeries);
-        RString sValue = "";
             
+            ini.GetValue("Group", "Series", sSeries);
+
+            RString sValue = "";
             ini.GetValue("Group", "SyncOffset", sValue);
             Trim(sValue);
             if (!sValue.empty()) {
@@ -79,93 +97,16 @@ Group::Group(const RString& sDir, const RString& sGroupDirName) {
                 } else if (sValue.CompareNoCase("itg") == 0) {
                     fOffset = -0.009f;
                 } else {
-                fOffset = StringToFloat(sValue);
+                    LOG->Warn("Group::Group: Invalid SyncOffset value: %s in Pack.ini. Valid values are NULL and ITG. Defaulting to NULL.", sValue.c_str());
                 }
             }
+
             ini.GetValue("Group", "Year", m_iYearReleased);
-        
-        RString sVersion = "";
-        ini.GetValue("Group", "Version", sVersion);
-        Trim(sVersion);
-        if (!sVersion.empty()) {
-            m_iVersion = StringToInt(sVersion);
         } else {
-            // If the version is not set, set it to the current version
+            LOG->Warn("Group::Group: Pack.ini version not set. Using default values.");
-            // and write it to the file.
-            m_iVersion = INI_VERSION;
-            ini.SetValue("Group", "Version", INI_VERSION);
-            if( !ini.WriteFile(sPackIniPath)) {
-                RString sError = ssprintf( "Error writing pack.ini ", sPackIniPath.c_str(), ini.GetError().c_str() );
-                Dialog::OK( sError );
         }  
     }
 
-    } else {
-        m_bHasPackIni = false;
-        m_iVersion = INI_VERSION;
-        fOffset = PREFSMAN->m_fMachineSyncBias;
-    }
-
-    
-	// Look for a group banner in this group folder
-	std::vector<RString> arrayGroupBanners;
-	
-	// First check if there is a banner provided in pack.ini
-	if(!sBannerPath.empty())
-	{
-		GetDirListing(sDir + sGroupDirName + "/" + sBannerPath, arrayGroupBanners);
-	}
-	GetDirListing(sDir + sGroupDirName + "/*.png", arrayGroupBanners);
-	GetDirListing(sDir + sGroupDirName + "/*.jpg", arrayGroupBanners);
-	GetDirListing(sDir + sGroupDirName + "/*.jpeg", arrayGroupBanners);
-	GetDirListing(sDir + sGroupDirName + "/*.gif", arrayGroupBanners);
-	GetDirListing(sDir + sGroupDirName + "/*.bmp", arrayGroupBanners);
-
-	if(!arrayGroupBanners.empty()) {
-		m_sBannerPath = sDir + sGroupDirName + "/" + arrayGroupBanners[0] ;
-    }
-	else
-	{
-		// Look for a group banner in the parent folder
-		GetDirListing(sDir + sGroupDirName + ".png", arrayGroupBanners);
-		GetDirListing(sDir + sGroupDirName + ".jpg", arrayGroupBanners);
-		GetDirListing(sDir + sGroupDirName + ".jpeg", arrayGroupBanners);
-		GetDirListing(sDir + sGroupDirName + ".gif", arrayGroupBanners);
-		GetDirListing(sDir + sGroupDirName + ".bmp", arrayGroupBanners);
-		if(!arrayGroupBanners.empty())
-			m_sBannerPath = sDir + arrayGroupBanners[0];
-	}
-
-        /* Other group graphics are a bit trickier, and usually don't exist.
-        * A themer has a few options, namely checking the aspect ratio and
-        * operating on it. -aj
-        * TODO: Once the files are implemented in Song, bring the extensions
-        * from there into here. -aj */
-        // Group background
-
-        //vector<RString> arrayGroupBackgrounds;
-        //GetDirListing(sDir + sGroupDirName + "/*-bg.png", arrayGroupBanners);
-        //GetDirListing(sDir + sGroupDirName + "/*-bg.jpg", arrayGroupBanners);
-        //GetDirListing(sDir + sGroupDirName + "/*-bg.jpeg", arrayGroupBanners);
-        //GetDirListing(sDir + sGroupDirName + "/*-bg.gif", arrayGroupBanners);
-        //GetDirListing(sDir + sGroupDirName + "/*-bg.bmp", arrayGroupBanners);
-    /*
-        RString sBackgroundPath;
-        if(!arrayGroupBackgrounds.empty())
-            sBackgroundPath = sDir + sGroupDirName + "/" + arrayGroupBackgrounds[0];
-        else
-        {
-            // Look for a group background in the parent folder
-            GetDirListing(sDir + sGroupDirName + "-bg.png", arrayGroupBackgrounds);
-            GetDirListing(sDir + sGroupDirName + "-bg.jpg", arrayGroupBackgrounds);
-            GetDirListing(sDir + sGroupDirName + "-bg.jpeg", arrayGroupBackgrounds);
-            GetDirListing(sDir + sGroupDirName + "-bg.gif", arrayGroupBackgrounds);
-            GetDirListing(sDir + sGroupDirName + "-bg.bmp", arrayGroupBackgrounds);
-            if(!arrayGroupBackgrounds.empty())
-                sBackgroundPath = sDir + arrayGroupBackgrounds[0];
-        }
-    */
-
     m_sDisplayTitle = sDisplayTitle;
     m_sSortTitle = sSortTitle;
     m_sTranslitTitle = sTranslitTitle;

src/MusicWheelItem.cpp

@@ -231,8 +231,10 @@ void MusicWheelItem::LoadFromWheelItemData( const WheelItemBaseData *pData, int
 					sTranslitName = SONGMAN->ShortenGroupName(pWID->m_pGroup->GetTranslitTitle());
 				}
 				else {
-					sDisplayName = SONGMAN->ShortenGroupName("[" + pWID->m_pGroup->GetSeries() +"] " + pWID->m_pGroup->GetDisplayTitle());
+					sDisplayName = SONGMAN->ShortenGroupName(pWID->m_sText);
-					sTranslitName = SONGMAN->ShortenGroupName("[" + pWID->m_pGroup->GetSeries() +"] " + pWID->m_pGroup->GetTranslitTitle());
+					sTranslitName = SONGMAN->ShortenGroupName(pWID->m_pGroup->GetTranslitTitle());
+					// sDisplayName = SONGMAN->ShortenGroupName("[" + pWID->m_pGroup->GetSeries() +"] " + pWID->m_pGroup->GetDisplayTitle());
+					// sTranslitName = SONGMAN->ShortenGroupName("[" + pWID->m_pGroup->GetSeries() +"] " + pWID->m_pGroup->GetTranslitTitle());
 				}
 			}
 			if( GAMESTATE->sExpandedSectionName == pWID->m_sText )

src/SongManager.cpp

@@ -279,22 +279,77 @@ void SongManager::AddGroup( RString sDir, RString sGroupDirName, Group* group )
 	if( j != m_sSongGroupNames.size() )
 		return; // the group is already added
 
-	m_sSongGroupNames.push_back( sGroupDirName );
+	RString sBannerPath;
-	// add to the group list
+
-	if( group )
+	// Look for a group banner in this group folder
+	std::vector<RString> arrayGroupBanners;
+	
+	// First check if there is a banner provided in pack.ini
+	if(!group->GetBannerPath().empty())
 	{
-		m_pGroups.push_back( group );
+		GetDirListing(sDir + sGroupDirName + "/" + group->GetBannerPath(), arrayGroupBanners);
+	}
+	GetDirListing( sDir+sGroupDirName+"/*.png", arrayGroupBanners );
+	GetDirListing( sDir+sGroupDirName+"/*.jpg", arrayGroupBanners );
+	GetDirListing( sDir+sGroupDirName+"/*.jpeg", arrayGroupBanners );
+	GetDirListing( sDir+sGroupDirName+"/*.gif", arrayGroupBanners );
+	GetDirListing( sDir+sGroupDirName+"/*.bmp", arrayGroupBanners );
+
+	if( !arrayGroupBanners.empty() )
+		sBannerPath = sDir+sGroupDirName+"/"+arrayGroupBanners[0] ;
+	else
+	{
+		// Look for a group banner in the parent folder
+		GetDirListing( sDir+sGroupDirName+".png", arrayGroupBanners );
+		GetDirListing( sDir+sGroupDirName+".jpg", arrayGroupBanners );
+		GetDirListing( sDir+sGroupDirName+".jpeg", arrayGroupBanners );
+		GetDirListing( sDir+sGroupDirName+".gif", arrayGroupBanners );
+		GetDirListing( sDir+sGroupDirName+".bmp", arrayGroupBanners );
+		if( !arrayGroupBanners.empty() )
+			sBannerPath = sDir+arrayGroupBanners[0];
 	}
 
-	if (m_mapGroupsByName.find(sGroupDirName) == m_mapGroupsByName.end())
+        /* Other group graphics are a bit trickier, and usually don't exist.
+        * A themer has a few options, namely checking the aspect ratio and
+        * operating on it. -aj
+        * TODO: Once the files are implemented in Song, bring the extensions
+        * from there into here. -aj */
+        // Group background
+
+        //vector<RString> arrayGroupBackgrounds;
+        //GetDirListing(sDir + sGroupDirName + "/*-bg.png", arrayGroupBanners);
+        //GetDirListing(sDir + sGroupDirName + "/*-bg.jpg", arrayGroupBanners);
+        //GetDirListing(sDir + sGroupDirName + "/*-bg.jpeg", arrayGroupBanners);
+        //GetDirListing(sDir + sGroupDirName + "/*-bg.gif", arrayGroupBanners);
+        //GetDirListing(sDir + sGroupDirName + "/*-bg.bmp", arrayGroupBanners);
+    /*
+        RString sBackgroundPath;
+        if(!arrayGroupBackgrounds.empty())
+            sBackgroundPath = sDir + sGroupDirName + "/" + arrayGroupBackgrounds[0];
+        else
         {
-		m_mapGroupsByName[sGroupDirName] = group;
+            // Look for a group background in the parent folder
+            GetDirListing(sDir + sGroupDirName + "-bg.png", arrayGroupBackgrounds);
+            GetDirListing(sDir + sGroupDirName + "-bg.jpg", arrayGroupBackgrounds);
+            GetDirListing(sDir + sGroupDirName + "-bg.jpeg", arrayGroupBackgrounds);
+            GetDirListing(sDir + sGroupDirName + "-bg.gif", arrayGroupBackgrounds);
+            GetDirListing(sDir + sGroupDirName + "-bg.bmp", arrayGroupBackgrounds);
+            if(!arrayGroupBackgrounds.empty())
+                sBackgroundPath = sDir + arrayGroupBackgrounds[0];
+        }
+    */
+   	m_sSongGroupBannerPaths.push_back( sBannerPath );
+	m_sSongGroupNames.push_back( sGroupDirName );
+	
+	if (m_mapNameToGroup.find(sGroupDirName) == m_mapNameToGroup.end())
+	{
+		m_mapNameToGroup[sGroupDirName] = group;
 	} 
 
 	// Add the group to its series if the group has one and if the series exists
 	if( group->GetSeries() != "" )
 	{
-		std::vector<Group*>& series = m_mapSeries[group->GetSeries()];
+		std::vector<Group*>& series = m_mapSeriesToGroup[group->GetSeries()];
 		if( std::find(series.begin(), series.end(), group) == series.end() )
 			series.push_back(group);
 	}
@@ -415,7 +470,7 @@ void SongManager::LoadSongDir( RString sDir, LoadingWindow *ld, bool onlyAdditio
 				continue;
 			}
 			// Apply Group Offset if applicable
-			if( group->GetSyncOffset() != 0 )
+			if( group->GetSyncOffset() != 0.0f )
 			{
 				pNewSong->m_SongTiming.m_fBeat0GroupOffsetInSeconds = group->GetSyncOffset();
 				const std::vector<Steps*>& vpSteps = pNewSong->GetAllSteps();
@@ -444,7 +499,7 @@ void SongManager::LoadSongDir( RString sDir, LoadingWindow *ld, bool onlyAdditio
 		AddGroup(sDir, sGroupDirName, group);
 
 		// Cache and load the group banner. (and background if it has one -aj)
-		IMAGECACHE->CacheImage( "Banner", group->GetBannerPath() );
+		IMAGECACHE->CacheImage( "Banner", GetSongGroupBannerPath(sGroupDirName) );
 
 		// Load the group sym links (if any)
 		LoadGroupSymLinks(sDir, sGroupDirName);
@@ -527,7 +582,6 @@ void SongManager::PreloadSongImages()
 void SongManager::FreeSongs()
 {
 	m_sSongGroupNames.clear();
-	m_mapGroupsByName.clear();
 	m_mapSongsByDifficulty.clear();
 	m_mapPreferredSectionToSongs.clear();
 
@@ -537,13 +591,15 @@ void SongManager::FreeSongs()
 	{
 		RageUtil::SafeDelete( song );
 	}
-    // Loop through all groups and delete them.
+    // Loop through all groups in the mapand delete them.
-    for (Group *group : m_pGroups) {
+	for (auto it = m_mapNameToGroup.begin(); it != m_mapNameToGroup.end(); ++it)
+	{
+		Group* group = it->second;
 		RageUtil::SafeDelete( group );
 	}
 
 	m_pSongs.clear();
-	m_pGroups.clear();
+	m_mapNameToGroup.clear();
 	m_SongsByDir.clear();
 
 	// also free the songs that have been deleted from disk
@@ -583,13 +639,13 @@ bool SongManager::IsGroupNeverCached(const RString& group) const
 
 RString SongManager::GetSongGroupBannerPath( RString sSongGroup ) const
 {
-	Group* group = GetGroupFromName(sSongGroup);
+	for( unsigned i = 0; i < m_sSongGroupNames.size(); ++i )
-	if(group == nullptr)
 	{
-		LOG->Warn("Requested banner for song group '%s' that doesn't exist", sSongGroup.c_str());
+		if( sSongGroup == m_sSongGroupNames[i] )
-		return RString();
+			return m_sSongGroupBannerPaths[i];
 	}
-	return group->GetBannerPath();
+
+	return RString();
 }
 /*
 RString SongManager::GetSongGroupBackgroundPath( RString sSongGroup ) const
@@ -622,9 +678,6 @@ bool SongManager::HasPackIni(RString sSongGroup) const
 		return group->HasPackIni();
 	}
 	LOG->Warn("Requested pack.ini for song group '%s' that doesn't exist", sSongGroup.c_str());
-	#ifdef DEBUG
-		ASSERT_M( 0, ssprintf("requested pack.ini for song group '%s' that doesn't exist",sSongGroup.c_str()) );
-	#endif
 	return false;
 }
 
@@ -871,8 +924,8 @@ Group* SongManager::GetGroup( const Song* pSong ) const
 
 Group* SongManager::GetGroupFromName( const RString& sGroupName ) const
 {
-	auto iter = m_mapGroupsByName.find( sGroupName );
+	auto iter = m_mapNameToGroup.find( sGroupName );
-	if( iter != m_mapGroupsByName.end() )
+	if( iter != m_mapNameToGroup.end() )
 		return iter->second;
 	return nullptr;
 }

src/SongManager.h

@@ -147,8 +147,8 @@ public:
 	void GetPreferredSortSongs( std::vector<Song*> &AddTo ) const;
 	std::map<RString, std::vector<Song*>> GetPreferredSortSongsMap() const { return m_mapPreferredSectionToSongs;};
 	RString SongToPreferredSortSectionName( const Song *pSong ) const;
-	std::map<RString, Group*> GetGroupGroupMap() const { return m_mapGroupsByName;};
+	std::map<RString, Group*> GetGroupGroupMap() const { return m_mapNameToGroup;};
-	std::map<RString, std::vector<Group*>> GetSeriesGroupMap() const { return m_mapSeries;};
+	std::map<RString, std::vector<Group*>> GetSeriesGroupMap() const { return m_mapSeriesToGroup;};
 	Group* GetGroupFromName(  const RString &sGroupName ) const;
 	Group* GetGroup( const Song *pSong ) const;
 	std::vector<RString> GetPreferredSortSectionNames() const;
@@ -223,8 +223,7 @@ protected:
 	void AddSongToList(Song* new_song);
 	/** @brief All of the songs that can be played. */
 	std::vector<Song*>		m_pSongs;
-	/** @brief All of the groups available */
+
-	std::vector<Group*>		m_pGroups;
 	std::map<RString, Song*> m_SongsByDir;
 	std::set<RString> m_GroupsToNeverCache;
 
@@ -248,10 +247,11 @@ protected:
 	std::map<RString, std::vector<Song*>> m_mapPreferredSectionToSongs;
 	std::vector<PreferredSortSection> m_vPreferredSongSort;
 	std::vector<RString>		m_sSongGroupNames;
+	std::vector<RString>		m_sSongGroupBannerPaths; // each song group may have a banner associated with it
 	//vector<RString>		m_sSongGroupBackgroundPaths; // each song group may have a background associated with it (very rarely)
 	
-	std::map<RString, Group*> m_mapGroupsByName;
+	std::map<RString, Group*> m_mapNameToGroup;
-	std::map<RString, std::vector<Group*>>	m_mapSeries;
+	std::map<RString, std::vector<Group*>>	m_mapSeriesToGroup;
 
 	struct Comp { bool operator()(const RString& s, const RString &t) const { return CompareRStringsAsc(s,t); } };
 	typedef std::vector<Song*> SongPointerVector;

src/SongUtil.cpp

@@ -597,8 +597,6 @@ int SongUtil::CompareSongPointersByGroup(const Song *pSong1, const Song *pSong2)
 {
 	// Check if the sort title exists
 	if( SONGMAN->GetGroup(pSong1)->GetSortTitle().empty() || SONGMAN->GetGroup(pSong2)->GetSortTitle().empty() ) {
-		// LOG the DEETS
-		LOG->Warn("SongUtil::CompareSongPointersByGroup: Song %s or %s has an empty group name. Using group name instead.", pSong1->m_sSongName.c_str(), pSong2->m_sSongName.c_str());
 		return pSong1->m_sGroupName < pSong2->m_sGroupName;
 	} else {
 		return SONGMAN->GetGroup(pSong1)->GetSortTitle() < SONGMAN->GetGroup(pSong2)->GetSortTitle();