Skip to content

Conversation

@TheBlackParrot
Copy link

Was running into a System.InvalidOperationException every time I reloaded songs with both the current public version of SongCore and the version available in the dev branch on 1.40.4 and 1.40.3.

[INFO @ 23:12:45 | SongCore] Starting song refresh
[ERROR @ 23:12:45 | SongCore] RetrieveAllSongs failed:
[ERROR @ 23:12:45 | SongCore] System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
[ERROR @ 23:12:45 | SongCore]   at System.Collections.Generic.Dictionary`2+ValueCollection+Enumerator[TKey,TValue].MoveNext () [0x00013] in <51fded79cd284d4d911c5949aff4cb21>:0 
[ERROR @ 23:12:45 | SongCore]   at SongCore.Loader+<>c__DisplayClass93_0.<RetrieveAllSongs>b__1 () [0x003a7] in C:\Users\Meivyn\Downloads\SongCore\source\SongCore\Loader.cs:473 
[INFO @ 23:12:45 | SongCore] Loaded 2 new songs (2) in CustomLevels | 0 in separate folders) in 0.0375272 seconds

Been a thorn in my side so I patched around it, most of the stuff I was looking up mentioned using lock in some capacity, but was unsure how the lock() function works. I took the easier way out and moved the DeleteSingleSong call outside of the loop, using a collection that doesn't get modified.

Build was complaining about the added reference.

honestly terrified of submitting a PR to SongCore, the annoyance got to me enough lol

Meivyn added a commit that referenced this pull request Apr 12, 2025
@Meivyn
Copy link
Collaborator

Meivyn commented Apr 12, 2025

Thanks for the PR, but I was aware of that and fixed it locally already. Your PR is also not fixing the logic properly. I pushed the changes, could you tell me if that fixes your issue? Looking at it, my approach is less efficient but it should be safer.

Note that the inclusion of AdditionalContentModel.Interfaces is unnecessary, IDE might complain about it because of Harmony patches but it doesn't prevent build.

Also, don't be afraid of submitting a PR to SongCore. I'll review it if it needs to be, but PRs are welcome.

@TheBlackParrot
Copy link
Author

Changes fixed the issue, yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants