diff --git a/stepmania/src/ScreenEdit.cpp b/stepmania/src/ScreenEdit.cpp index 44c8e16c69..fff167d7c9 100644 --- a/stepmania/src/ScreenEdit.cpp +++ b/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. - // If the user created them in the edit menu and never bothered - // to save them, then they aren't wanted. - Steps* pSteps = GAMESTATE->m_pCurSteps[PLAYER_1]; - if( !pSteps->GetSavedToDisk() ) + // We have to scroll through the entire Steps array because the user + // may have changed to a different stepchart after starting the edit. + vector vStepsToDelete; + for( vector::const_iterator it = GAMESTATE->m_pCurSong->GetAllSteps().begin(); + it != GAMESTATE->m_pCurSong->GetAllSteps().end(); ++it ) { - Song* pSong = GAMESTATE->m_pCurSong; - pSong->DeleteSteps( pSteps ); - m_pSteps = NULL; - GAMESTATE->m_pCurSteps[PLAYER_1].Set( NULL ); - } + Steps *pSteps = *it; + if( !pSteps->GetSavedToDisk() && !pSteps->IsAutogen() ) + { + 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; - if( GAMESTATE->m_pCurSteps[PLAYER_1] ) - m_stepsLastSave = *GAMESTATE->m_pCurSteps[PLAYER_1]; + m_SongLastSave.DeepCopy( *GAMESTATE->m_pCurSong ); } void ScreenEdit::CopyFromLastSave() { - *GAMESTATE->m_pCurSong = m_songLastSave; - if( GAMESTATE->m_pCurSteps[PLAYER_1] ) - *GAMESTATE->m_pCurSteps[PLAYER_1] = m_stepsLastSave; + // We have to make sure the new m_pCurSteps pointer points to the correct Steps + // object. The Song copy destroys all of the old pointers. + 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; - if( GAMESTATE->m_pCurSteps[PLAYER_1] ) - m_stepsLastSave = *GAMESTATE->m_pCurSteps[PLAYER_1]; + CopyToLastSave(); SetDirty(false); } diff --git a/stepmania/src/ScreenEdit.h b/stepmania/src/ScreenEdit.h index 38dc1640a8..6b65f3f994 100644 --- a/stepmania/src/ScreenEdit.h +++ b/stepmania/src/ScreenEdit.h @@ -253,9 +253,8 @@ protected: void CopyFromLastSave(); void RevertFromDisk(); - Song m_songLastSave; - Steps m_stepsLastSave; - + Song m_SongLastSave; + // for MODE_RECORD diff --git a/stepmania/src/ScreenGameplay.cpp b/stepmania/src/ScreenGameplay.cpp index a6f8de9852..71c5549251 100644 --- a/stepmania/src/ScreenGameplay.cpp +++ b/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; diff --git a/stepmania/src/Song.cpp b/stepmania/src/Song.cpp index 74b3512dbf..c9c93c6fd5 100644 --- a/stepmania/src/Song.cpp +++ b/stepmania/src/Song.cpp @@ -42,9 +42,8 @@ static Preference g_fMarathonVerSongSeconds( "MarathonVerSongSeconds", 60 Song::Song() { FOREACH_BackgroundLayer( i ) - m_BackgroundChanges[i] = AutoPtrCopyOnWrite(new VBackgroundChange); - m_ForegroundChanges = AutoPtrCopyOnWrite(new VBackgroundChange); - + m_BackgroundChanges[i] = new VBackgroundChange; + m_ForegroundChanges = new VBackgroundChange; m_LoadedFromProfile = ProfileSlot_Invalid; m_fMusicSampleStartSeconds = -1; @@ -68,34 +67,96 @@ 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() -{ - m_vpSteps.clear(); - FOREACH_StepsType( st ) - m_vpStepsByType[st].clear(); -} - -/* Reset to an empty song. */ +// 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::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 ) { // Delete old background change at this start beat, if any. @@ -173,7 +234,7 @@ static set 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 &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 &Song::GetBackgroundChanges( BackgroundLayer bl ) } vector &Song::GetBackgroundChanges( BackgroundLayer bl ) { - return *(m_BackgroundChanges[bl].Get()); + return *(m_BackgroundChanges[bl]); } const vector &Song::GetForegroundChanges() const @@ -972,7 +1047,7 @@ const vector &Song::GetForegroundChanges() const } vector &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 &vpSteps = m_vpStepsByType[pSteps->m_StepsType]; for( int j=vpSteps.size()-1; j>=0; j-- ) { diff --git a/stepmania/src/SongManager.cpp b/stepmania/src/SongManager.cpp index a0bd6a24c2..ad7874230c 100644 --- a/stepmania/src/SongManager.cpp +++ b/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¶m...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 ); diff --git a/stepmania/src/StepsUtil.h b/stepmania/src/StepsUtil.h index 4cf8f90613..a28bac3c40 100644 --- a/stepmania/src/StepsUtil.h +++ b/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 { diff --git a/stepmania/src/Trail.cpp b/stepmania/src/Trail.cpp index 4b9e5abcb0..a7ef607539 100644 --- a/stepmania/src/Trail.cpp +++ b/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 ); } diff --git a/stepmania/src/song.h b/stepmania/src/song.h index 09618fa07d..7dfa2e6bbe 100644 --- a/stepmania/src/song.h +++ b/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 VBackgroundChange; private: - // AutoPtr instead of raw pointer so that the auto gen'd copy constructor works correctly. - AutoPtrCopyOnWrite m_BackgroundChanges[NUM_BackgroundLayer]; // these must be sorted before gameplay - AutoPtrCopyOnWrite m_ForegroundChanges; // this must be sorted before gameplay + VBackgroundChange * m_BackgroundChanges[NUM_BackgroundLayer]; // these must be sorted before gameplay + VBackgroundChange * m_ForegroundChanges; // this must be sorted before gameplay public: const vector &GetBackgroundChanges( BackgroundLayer bl ) const; vector &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 m_vpSteps; vector m_vpStepsByType[NUM_StepsType];