Adding new config option to load the songs in the background (similar to covers) to allow the user to start using the program earlier#813
Conversation
… to covers) to allow the user to start using the program earlier
iakona
left a comment
There was a problem hiding this comment.
i've done testing on my end and with a friend and this generally seems to work. but there may still be some edge cases i haven't handled properly yet. in theory though rolling this out with nightly/prerelease would get more eyes to help fix an edge cases i have created
| if ((_IntroOutPlayed || _SkipIntro) && CSettings.ProgramState == EProgramState.Start && songsReady && coversReady) | ||
| { | ||
| CSettings.ProgramState = EProgramState.Normal; | ||
| CGraphics.FadeTo(EScreen.Main); |
There was a problem hiding this comment.
the FadeTo call will cause the song loading count text to disappear and then reappear when dynamic is enabled. not sure if it's necessarily worth going down the rabbit hole to try to fix
| lock (_Covers) | ||
| { | ||
| _Covers.Add(text, texture); | ||
| _Covers[text] = texture; |
There was a problem hiding this comment.
i encountered a crash when closing sometimes where we'd try to add a cover that already existed, so switching this to an upsert operation fixed that
|
|
||
| public void ForceRefresh() | ||
| { | ||
| _SetChanged(); |
There was a problem hiding this comment.
this will cause the visible song list to update
| foreach (CSong song in CSongs.Songs) | ||
| for (int i = 0; i < CSongs.NumAllSongs; i++) | ||
| { | ||
| CSong song = CSongs.Songs[i]; |
There was a problem hiding this comment.
i encountered a modification of enumerable while enumerating, so changing this to be index based fixed that
| bool profileOK = CProfiles.NumProfiles > 0; | ||
| _Statics[_StaticWarningProfiles].Visible = !profileOK; | ||
| _Texts[_TextWarningProfiles].Visible = !profileOK; | ||
| if (CConfig.Config.Theme.SongLoading == ESongLoading.TR_CONFIG_SONGLOADING_DYNAMIC) |
There was a problem hiding this comment.
when dynamic is enabled, figured it'd be a good idea to still show the user how they are progressing towards loading songs. but i can always remove this if you'd like
| _TimerShortInfoText.AutoReset = false; | ||
| _TimerShortInfoText.Elapsed += OnTimedEventShortInfoText; | ||
|
|
||
| _TimerLoadSongs = new System.Timers.Timer(5000); |
There was a problem hiding this comment.
i used 5 seconds as a value since other timers (like above) run on that interval. i'm open to suggestions to other refresh rates
| [DefaultValue(EPlayerInfo.TR_CONFIG_PLAYERINFO_BOTH)] public EPlayerInfo PlayerInfo; | ||
| [DefaultValue(EFadePlayerInfo.TR_CONFIG_FADEPLAYERINFO_OFF)] public EFadePlayerInfo FadePlayerInfo; | ||
| [DefaultValue(ECoverLoading.TR_CONFIG_COVERLOADING_DYNAMIC)] public ECoverLoading CoverLoading; | ||
| [DefaultValue(ESongLoading.TR_CONFIG_SONGLOADING_ATSTART)] public ESongLoading SongLoading; |
There was a problem hiding this comment.
defaulting this to do the song loading upfront to preserve existing functionality, but this also means there's less eyes on the feature. not sure if it's better to enable this feature by default in nightly/prerelease and then default it to up front on the stable release (whenever this feature goes out)
| <string name="TR_CONFIG_COVERLOADING_ONDEMAND">On demand</string> | ||
| <string name="TR_CONFIG_COVERLOADING_ATSTART">At startup</string> | ||
| <string name="TR_CONFIG_COVERLOADING_DYNAMIC">Dynamic</string> | ||
| <string name="TR_CONFIG_SONGLOADING_ATSTART">At startup</string> |
There was a problem hiding this comment.
i should probably just use the coverloading strings here instead right? since they are already translated? or should i copy paste those translations into the other languages?
|
Thank you for your contribution! I'm not sure what you're trying to achieve with this pull request? Cover loading on demand is used to only load cover into the memory if they are needed, so safe some memory as pictured tend to be bigger. Song information are in contrast to that necessary to display the list of songs, so lazy loading wouldn't be that helpful? Plus song information aren't that big. |
what i'm trying to accomplish is getting into the singing faster. currently all songs must be loaded before you're able to interact with anything on the main menu, and for folks with large song libraries and/or on low spec computers that could be 5+ minutes of waiting. so the idea is to have them be able to jump into using the program, setting up the microphone and singers, tweaking settings, or even looking at the song list some while things load in the background. instead of having to wait a period of time first before doing things |
|
Got it! As you've already stated in you description: there could be a lot of edge cases happening and stuff will become quite complex. Have you tried to identify bottlenecks of the song loading? Maybe there is room for improvement? If I remember correctly, you can activate a benchmark within the config and check the benchmark log afterwards. |
my local setup isn't really in a position to benchmark, i've only got a pretty small library and a nice computer. my friend has a large library with a not so nice computer, but also doesn't have development environment setup. is there a way to enable the benchmark on a build i can send them and then they send me back the data? |
|
Cannot remember where it's located. Could you check the Vocaluxe.log for benchmark information? Or/and set DebugLevel to TR_CONFIG_ON within config.xml? |
No description provided.