Fix several annoying edit mode bugs, mostly by fixing the Song copy

operator and the various uses of it.

- revert file from disk: keeps old BPM.  The BPMS on disk are added to
  the existing BPM information!
- edit mode turns X stepcharts into 2X when reverting
- sometimes edit mode loses an entire stepchart: open a stepchart,
  change to a new stepchart, exit without going back, original chart gone
- edit a chart, make a change in a different chart, exit from the first chart:
  change made in different chart doesn't revert
- normally, when you create a stepchart and don't save it after editting it,
  it is deleted.  however, if you create a stepchart, don't save it, and exit
  edit mode while looking at a different chart, the chart isn't deleted.

Some existing bugs are not fixed, though:
- delete the last stepchart for a step type.  CTD.
- Edit one of the fake doubles stepcharts for a song that doesn't have
  real doubles stepcharts.  Exit without doing anything.  CTD.  This is
  probably the same problem as the previous bug.
- delete a song or a steps that happens to be part of a trail.  Trail is now
  broken.  Some other edits should affect a trail as well, but don't.

stepmania/src/ScreenEdit.cpp

@@ -769,9 +769,6 @@ void ScreenEdit::Init()
 
 ScreenEdit::~ScreenEdit()
 {
-	// UGLY: Don't delete the Song's steps.
-	m_songLastSave.DetachSteps();
-
 	LOG->Trace( "ScreenEdit::~ScreenEdit()" );
 	m_soundMusic.StopPlaying();
 }
@@ -2580,7 +2577,6 @@ void ScreenEdit::HandleScreenMessage( const ScreenMessage SM )
 	else if( SM == SM_SaveSuccessful )
 	{
 		LOG->Trace( "Save successful." );
-		m_pSteps->SetSavedToDisk( true );
 		CopyToLastSave();
 		SetDirty( false );
 
@@ -2602,17 +2598,24 @@ void ScreenEdit::HandleScreenMessage( const ScreenMessage SM )
 		// IMPORTANT: CopyFromLastSave before deleting the Steps below
 		CopyFromLastSave();
 
-		// If these steps have never been saved, then we should delete them.
+		// We have to scroll through the entire Steps array because the user
-		// If the user created them in the edit menu and never bothered
+		// may have changed to a different stepchart after starting the edit.
-		// to save them, then they aren't wanted.
+		vector<Steps*> vStepsToDelete;
-		Steps* pSteps = GAMESTATE->m_pCurSteps[PLAYER_1];
+		for( vector<Steps*>::const_iterator it = GAMESTATE->m_pCurSong->GetAllSteps().begin();
-		if( !pSteps->GetSavedToDisk() )
+			it != GAMESTATE->m_pCurSong->GetAllSteps().end(); ++it )
 		{
-			Song* pSong = GAMESTATE->m_pCurSong;
+			Steps *pSteps = *it;
-			pSong->DeleteSteps( pSteps );
+			if( !pSteps->GetSavedToDisk() && !pSteps->IsAutogen() )
-			m_pSteps = NULL;
+			{
-			GAMESTATE->m_pCurSteps[PLAYER_1].Set( NULL );
+				vStepsToDelete.push_back( pSteps );
-		}
+				if( pSteps == GAMESTATE->m_pCurSteps[PLAYER_1] )
+					GAMESTATE->m_pCurSteps[PLAYER_1].Set( NULL );
+				if( pSteps == m_pSteps )
+					m_pSteps = NULL;
+			}
+ 		}
+		FOREACH( Steps*, vStepsToDelete, pSteps )
+			GAMESTATE->m_pCurSong->DeleteSteps( *pSteps );
 
 		m_Out.StartTransitioning( SM_GoToNextScreen );
 	}
@@ -3429,16 +3432,23 @@ void ScreenEdit::SetupCourseAttacks()
 
 void ScreenEdit::CopyToLastSave()
 {
-	m_songLastSave = *GAMESTATE->m_pCurSong;
+	m_SongLastSave.DeepCopy( *GAMESTATE->m_pCurSong );
-	if( GAMESTATE->m_pCurSteps[PLAYER_1] )
-		m_stepsLastSave = *GAMESTATE->m_pCurSteps[PLAYER_1];
 }
 
 void ScreenEdit::CopyFromLastSave()
 {
-	*GAMESTATE->m_pCurSong = m_songLastSave;
+	// We have to make sure the new m_pCurSteps pointer points to the correct Steps 
-	if( GAMESTATE->m_pCurSteps[PLAYER_1] )
+	// object.  The Song copy destroys all of the old pointers.
-		*GAMESTATE->m_pCurSteps[PLAYER_1] = m_stepsLastSave;
+	StepsID id;
+ 	if( GAMESTATE->m_pCurSteps[PLAYER_1] )
+		id.FromSteps( GAMESTATE->m_pCurSteps[PLAYER_1] );
+
+	GAMESTATE->m_pCurSong->DeepCopy( m_SongLastSave );
+
+	if( id.IsValid() ) {
+		GAMESTATE->m_pCurSteps[PLAYER_1].Set( id.ToSteps( GAMESTATE->m_pCurSong, false ) );
+		m_pSteps = GAMESTATE->m_pCurSteps[PLAYER_1];
+	}
 }
 
 void ScreenEdit::RevertFromDisk()
@@ -3448,14 +3458,25 @@ void ScreenEdit::RevertFromDisk()
 		id.FromSteps( GAMESTATE->m_pCurSteps[PLAYER_1] );
 
 	RString sSongDir = GAMESTATE->m_pCurSong->GetSongDir();
-	GAMESTATE->m_pCurSong->LoadFromSongDir( sSongDir );
+	GAMESTATE->m_pCurSong->ReloadFromSongDir( sSongDir );
 
 	if( id.IsValid() )
-		GAMESTATE->m_pCurSteps[PLAYER_1].Set( id.ToSteps( GAMESTATE->m_pCurSong, false ) );
+	{
+		Steps *pChosenSteps = id.ToSteps( GAMESTATE->m_pCurSong, true );
+		if( pChosenSteps == NULL )
+		{
+			// This can happen in a couple ways.  For example, the user might be editting a new
+			// stepchart and revert from disk.
+			pChosenSteps = new Steps();
+			pChosenSteps->CreateBlank( id.GetStepsType() );
+			pChosenSteps->SetDifficultyAndDescription( id.GetDifficulty(), id.GetDescription() );
+			GAMESTATE->m_pCurSong->AddSteps( pChosenSteps );
+		}
+		GAMESTATE->m_pCurSteps[PLAYER_1].Set( pChosenSteps );
+		m_pSteps = pChosenSteps;
+	}
 
-	m_songLastSave = *GAMESTATE->m_pCurSong;
+	CopyToLastSave();
-	if( GAMESTATE->m_pCurSteps[PLAYER_1] )
-		m_stepsLastSave = *GAMESTATE->m_pCurSteps[PLAYER_1];
 
 	SetDirty(false);
 }

stepmania/src/ScreenEdit.h

@@ -253,8 +253,7 @@ protected:
 	void CopyFromLastSave();
 	void RevertFromDisk();
 
-	Song m_songLastSave;
+	Song m_SongLastSave;
-	Steps m_stepsLastSave;
 	
 
 // for MODE_RECORD

stepmania/src/ScreenGameplay.cpp

@@ -850,6 +850,11 @@ void ScreenGameplay::InitSongQueues()
 			pi->m_asModifiersQueue.clear();
 			FOREACH_CONST( TrailEntry, pTrail->m_vEntries, e )
 			{
+				// Here we assume there are never any NULL Steps in any Trails.  
+				// This assumption can fail if we delete a Steps from a 
+				// Song or revert a Song where the stepchart was deleted
+				// ouside the editor.  In those cases: goodbye cruel world.
+				// TODO: fix that somehow
 				ASSERT( e->GetSteps() );
 				pi->m_vpStepsQueue.push_back( e->GetSteps() );
 				AttackArray a;

stepmania/src/Song.cpp

@@ -42,9 +42,8 @@ static Preference<float>	g_fMarathonVerSongSeconds( "MarathonVerSongSeconds",	60
 Song::Song()
 {
 	FOREACH_BackgroundLayer( i )
-		m_BackgroundChanges[i] = AutoPtrCopyOnWrite<VBackgroundChange>(new VBackgroundChange);
+		m_BackgroundChanges[i] = new VBackgroundChange;
-	m_ForegroundChanges = AutoPtrCopyOnWrite<VBackgroundChange>(new VBackgroundChange);
+	m_ForegroundChanges = new VBackgroundChange;	
-	
 
 	m_LoadedFromProfile = ProfileSlot_Invalid;
 	m_fMusicSampleStartSeconds = -1;
@@ -68,33 +67,95 @@ Song::~Song()
 		SAFE_DELETE( *s );
 	m_vpSteps.clear();
 	
+	FOREACH_BackgroundLayer( i )
+		SAFE_DELETE( m_BackgroundChanges[i] );
+	SAFE_DELETE( m_ForegroundChanges );
+
 	// It's the responsibility of the owner of this Song to make sure
 	// that all pointers to this Song and its Steps are invalidated.
 }
 
-void Song::DetachSteps()
+// Reset to an empty song.
-{
-	m_vpSteps.clear();
-	FOREACH_StepsType( st )
-		m_vpStepsByType[st].clear();
-}
-
-/* Reset to an empty song. */
 void Song::Reset()
 {
-	FOREACH( Steps*, m_vpSteps, s )
-		SAFE_DELETE( *s );
-	m_vpSteps.clear();
-	FOREACH_StepsType( st )
-		m_vpStepsByType[st].clear();
-
 	Song empty;
-	*this = empty;
+	this->DeepCopy(empty);
-
 	// It's the responsibility of the owner of this Song to make sure
 	// that all pointers to this Song and its Steps are invalidated.
 }
 
+void Song::DeepCopy(const Song &song)
+{
+	m_sSongFileName = song.m_sSongFileName;
+	m_sGroupName = song.m_sGroupName;
+
+	m_LoadedFromProfile = song.m_LoadedFromProfile;
+	m_bIsSymLink = song.m_bIsSymLink;
+
+	m_sMainTitle = song.m_sMainTitle;
+	m_sSubTitle = song.m_sSubTitle;
+	m_sArtist = song.m_sArtist;
+	m_sMainTitleTranslit = song.m_sMainTitleTranslit;
+	m_sSubTitleTranslit = song.m_sSubTitleTranslit;
+	m_sArtistTranslit = song.m_sArtistTranslit;
+
+	m_sGenre = song.m_sGenre;
+	m_sCredit = song.m_sCredit;
+	m_sMusicFile = song.m_sMusicFile;
+
+	m_fMusicLengthSeconds = song.m_fMusicLengthSeconds;
+	m_fFirstBeat = song.m_fFirstBeat;
+	m_fLastBeat = song.m_fLastBeat;
+	m_fSpecifiedLastBeat = song.m_fSpecifiedLastBeat;
+	m_fMusicSampleStartSeconds = song.m_fMusicSampleStartSeconds;
+	m_fMusicSampleLengthSeconds = song.m_fMusicSampleLengthSeconds;
+	m_DisplayBPMType = song.m_DisplayBPMType;
+	m_fSpecifiedBPMMin = song.m_fSpecifiedBPMMin;
+	m_fSpecifiedBPMMax = song.m_fSpecifiedBPMMax;
+
+	m_sBannerFile = song.m_sBannerFile;
+	m_sLyricsFile = song.m_sLyricsFile;
+	m_sBackgroundFile = song.m_sBackgroundFile;
+	m_sCDTitleFile = song.m_sCDTitleFile;
+	m_bHasMusic = song.m_bHasMusic;
+	m_bHasBanner = song.m_bHasBanner;
+
+	m_Timing = song.m_Timing;
+	
+	FOREACH_BackgroundLayer( i )
+		*(m_BackgroundChanges[i]) = *(song.m_BackgroundChanges[i]);
+	*m_ForegroundChanges = *song.m_ForegroundChanges;
+	
+	m_LyricSegments = song.m_LyricSegments;
+
+	m_vsKeysoundFile = song.m_vsKeysoundFile;
+
+	// First, get rid of all the old steps ...
+ 	FOREACH( Steps*, m_vpSteps, s )
+ 		SAFE_DELETE( *s );
+ 	m_vpSteps.clear();
+	FOREACH_StepsType( i )
+		m_vpStepsByType[i].clear();
+ 
+	// ... then create duplicates of the new steps, correctly arranging them in the m_vpStepsByType array
+	m_vpSteps.clear();
+	for( vector<Steps*>::const_iterator it = song.m_vpSteps.begin(); it != song.m_vpSteps.end(); ++it )
+	{
+		Steps *steps = new Steps( **it );
+		m_vpSteps.push_back( steps );
+		m_vpStepsByType[steps->m_StepsType].push_back( steps );
+	}
+
+	// TODO: This is horribly ugly.  A much better solution is to make the Song class in charge of its
+	// own Steps cache.  StepsID::ToSteps can simply reference the Song cache then.
+	StepsID::ClearCache();
+	// TODO: we have also ruined the pointers for GAMESTATE->m_pCurSteps, STATSMAN->m_CurStageStats.m_player[].vpPlayedSteps,
+	// STATSMAN->m_vPlayedStageStats->m_player[].vpPlayedSteps, and GAMESTATE->m_pEditSourceSteps.  Probably we've also
+	// ruined other pointers aside from those.  In general, we need to use StepsIDs everywhere instead of Step*s.
+	// FOR NOW this is not a problem, as the only place this copy is used is in the editor.  The editor does not need any of
+	// the various GAMESTATE or STATSMAN pointers except for m_pCurSteps, which it already takes care of on its own.  
+	// However, we have to fix this if we are going to use DeepCopy anywhere else.
+ }
  
 void Song::AddBackgroundChange( BackgroundLayer iLayer, BackgroundChange seg )
 {
@@ -173,7 +234,7 @@ static set<RString> BlacklistedImages;
  *
  * If true, check the directory hash and reload the song from scratch if it's changed.
  */
-bool Song::LoadFromSongDir( RString sDir )
+bool Song::LoadFromSongDir( RString sDir, bool bIgnoreCache )
 {
 //	LOG->Trace( "Song::LoadFromSongDir(%s)", sDir.c_str() );
 	ASSERT( sDir != "" );
@@ -196,7 +257,7 @@ bool Song::LoadFromSongDir( RString sDir )
 	// First look in the cache for this song (without loading NoteData)
 	//
 	unsigned uCacheHash = SONGINDEX->GetCacheHash(m_sSongDir);
-	bool bUseCache = true;
+	bool bUseCache = !bIgnoreCache;
 	if( !DoesFileExist(GetCacheFilePath()) )
 		bUseCache = false;
 	if( !PREFSMAN->m_bFastLoad && GetHashForDirectory(m_sSongDir) != uCacheHash )
@@ -283,6 +344,14 @@ bool Song::LoadFromSongDir( RString sDir )
 	return true;	// do load this song
 }
 
+bool Song::ReloadFromSongDir( RString sDir )
+{
+	// If we are loading from the song dir a second time around, the song is filled
+	// with all sorts of data we no longer want.  Get rid of it.
+	Reset();
+	return LoadFromSongDir( sDir, true );
+}
+
 static void GetImageDirListing( RString sPath, vector<RString> &AddTo, bool bReturnPathToo=false )
 {
 	GetDirListing( sPath + ".png", AddTo, false, bReturnPathToo ); 
@@ -743,6 +812,12 @@ void Song::Save()
 		else
 			FILEMAN->Remove( sOldPath );
 	}
+
+	// Now we need to mark all of the stepcharts as "saved to disk".
+	FOREACH( Steps*, m_vpSteps, steps )
+	{
+		(*steps)->SetSavedToDisk( true );
+	}
 }
 
 
@@ -963,7 +1038,7 @@ const vector<BackgroundChange> &Song::GetBackgroundChanges( BackgroundLayer bl )
 }
 vector<BackgroundChange> &Song::GetBackgroundChanges( BackgroundLayer bl )
 {
-	return *(m_BackgroundChanges[bl].Get());
+	return *(m_BackgroundChanges[bl]);
 }
 
 const vector<BackgroundChange> &Song::GetForegroundChanges() const
@@ -972,7 +1047,7 @@ const vector<BackgroundChange> &Song::GetForegroundChanges() const
 }
 vector<BackgroundChange> &Song::GetForegroundChanges()
 {
-	return *m_ForegroundChanges.Get();
+	return *m_ForegroundChanges;
 }
 
 
@@ -1083,7 +1158,6 @@ void Song::DeleteSteps( const Steps* pSteps, bool bReAutoGen )
 	if( bReAutoGen )
 		RemoveAutoGenNotes();
 
-
 	vector<Steps*> &vpSteps = m_vpStepsByType[pSteps->m_StepsType];
 	for( int j=vpSteps.size()-1; j>=0; j-- )
 	{

stepmania/src/SongManager.cpp

@@ -235,7 +235,7 @@ void SongManager::LoadStepManiaSongDir( RString sDir, LoadingWindow *ld )
 				ld->Paint();
 			}
 			Song* pNewSong = new Song;
-			if( !pNewSong->LoadFromSongDir( sSongDirName ) )
+			if( !pNewSong->LoadFromSongDir( sSongDirName, false ) ) // don't ignore cache
 			{
 				/* The song failed to load. */
 				delete pNewSong;
@@ -277,7 +277,7 @@ void SongManager::LoadGroupSymLinks(RString sDir, RString sGroupFolder)
 		RString	sSymDestination = msdF.GetParam(0,1);	// Should only be 1 vale&param...period.
 		
 		Song* pNewSong = new Song;
-		if( !pNewSong->LoadFromSongDir( sSymDestination ) )
+		if( !pNewSong->LoadFromSongDir( sSymDestination, false ) ) // don't ignore cache
 		{
 			delete pNewSong; // The song failed to load.
 		}
@@ -993,7 +993,7 @@ void SongManager::RevertFromDisk( Song *pSong, bool bAllowNotesLoss )
 	pSong->Reset();
 	const bool OldVal = PREFSMAN->m_bFastLoad;
 	PREFSMAN->m_bFastLoad.Set( false );
-	pSong->LoadFromSongDir( dir );	
+	pSong->LoadFromSongDir( dir, true ); // when reverting, ignore cache
 	/* XXX: reload edits? */
 	PREFSMAN->m_bFastLoad.Set( OldVal );
 

stepmania/src/StepsUtil.h

@@ -90,6 +90,8 @@ public:
 	static void ClearCache();
 	
 	StepsType GetStepsType() const { return st; }
+	Difficulty GetDifficulty() const { return dc; }
+	RString GetDescription() const { return sDescription; }
 
 	bool operator==( const StepsID &other ) const
 	{

stepmania/src/Trail.cpp

@@ -32,6 +32,11 @@ bool TrailEntry::operator== ( const TrailEntry &rhs ) const
 
 Steps *TrailEntry::GetSteps() const
 {
+	// TODO:  There are a few bugs with the way we do trails right now.
+	//   - If a steps (or an entire song) is deleted, possibly through the editor,
+	//     this section will fail.
+	//   - If the editor changes a stepchart's stats, that should also be reflected
+	//     in the trail.
 	return m_StepsID.ToSteps( pSong, false );
 }
 

stepmania/src/song.h

@@ -58,7 +58,15 @@ public:
 	void Reset();
 	void DetachSteps();
 
-	bool LoadFromSongDir( RString sDir );
+	// WARNING: Do not use this DeepCopy in any situation with existing Steps*.  Those pointers
+	// will all point to garbage memory after this call.  The way to fix that is to change the
+	// pointers to StepsIDs or in some other way avoid keeping the Steps* pointers from before
+	// the call to DeepCopy.
+	void DeepCopy(const Song &song);
+
+	// LoadFromSongDir assumes the song is uninitialized.  Call ReloadFromSongDir if that is not true.
+	bool LoadFromSongDir( RString sDir, bool bIgnoreCache );
+	bool ReloadFromSongDir( RString sDir );
 
 	void TidyUpData();	// call after loading to clean up invalid data
 	void ReCalculateRadarValuesAndLastBeat();	// called by TidyUpData, and after saving
@@ -152,9 +160,8 @@ public:
 
 	typedef vector<BackgroundChange> 	VBackgroundChange;
 private:
-	// AutoPtr instead of raw pointer so that the auto gen'd copy constructor works correctly.
+	VBackgroundChange *			m_BackgroundChanges[NUM_BackgroundLayer];	// these must be sorted before gameplay
-	AutoPtrCopyOnWrite<VBackgroundChange>	m_BackgroundChanges[NUM_BackgroundLayer];	// these must be sorted before gameplay
+	VBackgroundChange *			m_ForegroundChanges;	// this must be sorted before gameplay
-	AutoPtrCopyOnWrite<VBackgroundChange>	m_ForegroundChanges;	// this must be sorted before gameplay
 public:
 	const vector<BackgroundChange>	&GetBackgroundChanges( BackgroundLayer bl ) const;
 	vector<BackgroundChange>	&GetBackgroundChanges( BackgroundLayer bl );
@@ -216,6 +223,9 @@ public:
 	void PushSelf( lua_State *L );
 
 private:
+	// DISALLOW operator= and Steps(Steps&)
+	Song(const Song&);
+	void operator=(const Song&);
 
 	vector<Steps*> m_vpSteps;
 	vector<Steps*> m_vpStepsByType[NUM_StepsType];