-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed Temple music errors #2582
Fixed Temple music errors #2582
Conversation
…wo different temples in the same city. Fixed issue where Fighter Trainers halls would throw an error in the SongManager (playing the Knight song like in classic instead)
We had a similar issue with dungeons: we're using a song player dedicated to dungeons, so it didn't notice when dungeon changed. I solved the dungeons case by listening to entering event, but that felt like an ad hoc hack, I was not satisfied at the time... #1632 |
I ran into the problem of teleporting from one interior type to another not changing the music in my Dynamic Music mod. My solution was to store a PlayerGPS.CurrentLocationIndex and check if it changed since the last frame. Not sure if it's the prettiest solution but it seems to work as a catch-all for determining if the player went to a different place. |
…consistent with how classic handles day transitions
…lly goes into two different dungeons in the same day
I've changed my approach from the original review. The basic logic of the song manager was:
For most playlists, the song selection is deterministically random: if you pick randomly from the same playlist on the same day, you get the same song (dungeons are based on location). For these playlists, refreshing songs will give you the same result, and the song manager will not restart the song if it's currently playing. The only exception is the Mages Guild, which is always random. This tracks with my tests in classic (just keep reentering, it will swap between the two randomly). Still, we want to avoid constantly refreshing in general, if we can avoid it. So, my change now:
Remember that if only the location changes and we refresh the song for playlists that don't change based on the location, the same song should be chosen. Again, Mages Guild is an exception, which is notable since the Teleportation service lets you change location index, which may change the song. That's not too much of an issue, since we did change location. I've also refactored some code. I've removed some redundancy without (hopefully) changing the behavior, and added some comments for why different playlists have different behavior. So, ultimately, this PR does the following:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I appreciate the extra refactos :)
Temple music is handled differently from other playlists in SongManager. The selected song is based on the faction of the building (the manager checks for both the "Temple" faction id and the "God" faction id). The songs are as follows:
I found two errors in how we handle this.
First, we use separate SongPlayer objects for Exterior and Interior. This means that when moving from an Interior to outside, the Interior SongPlayer is not updated until we enter a new interior. Therefore, if we move from one temple to outside to another temple, from the perspective of the Interior SongPlayer, the context never changed, and the music is not updated, even if the FactionID would cause a different song.
This can be tested in the Daggerfall region's Chesterwick Hamlet (not Chesterwick), which has both a Julianos and Kynareth temple. Both temples should have a different music, but if you enter one then directly enter the other, then the music is preserved from the first temple. The music is properly updated if you enter a non-temple interior then go to the temple.
So, I added "factionID" to the music context, which is only updated for temples since only that playlist actually checks it. When the playlist is the same as previously but the factionID changes, we update the song only if the playlist is Temples.
The second issue is what I initially investigated. DFU was throwing an exception if a Temple location did not have a proper Temple/God faction id, which occurs in DF data in those Fighter Trainers halls in Hammerfell. I checked in classic DF, and the music that plays is actually the "unused" Knight Order music - yes, that music is actually used. This can be checked in the Abibon-Gora capital.
So, I fixed both the error handling in the Temple music selection, and added a new music context for FighterTrainers so its classic DF music plays.