Skip to content

Conversation

@chowell1337
Copy link

@chowell1337 chowell1337 commented Aug 19, 2023

Prompts the user for a directory (would prefer if this could be auto-detected) Auto-detects the directory of the currently opened chart and automatically assigns files within that directory to the instrument associated iwth their name


public void GetInstrumentAudioByFileNames()
{
// TODO: there HAS to be a way to just detect the current folder, right???
Copy link
Owner

@FireFox2000000 FireFox2000000 Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could get the path of the currently loaded chart file from ChartEditor.Instance.lastLoadedFile (a getter will need to be written to make this public)

Copy link
Author

@chowell1337 chowell1337 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the information! added in commit fd986a3:
image

default:
// would like to use a switch expression, but since this enum is non-nullable,
// gotta use a continue to skip unwanted files
continue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better to have a list of the files that were skipped over, and then communicated to the user of which files were invalid and what the correct filenames should be (can send a message via ChartEditor.Instance.errorManager.QueueErrorMessage)

Copy link
Author

@chowell1337 chowell1337 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good suggestion, added in commit 61218e5

@FireFox2000000 are there any other filenames that should be considered non-error-worthy, but not be tied to an instrument? e.g. i know "preview.ogg" is used for separate song preview audio files in Clone Hero

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I'm aware of.

StartCoroutine(_RefreshAllAudioStreams());
}

public void GetInstrumentAudioByFileNames()
Copy link
Owner

@FireFox2000000 FireFox2000000 Dec 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function name should be more specific, would suggest something like "LoadCloneHeroAudioFromDirectory" as the list of audio files are CH-specific.

/// <summary>
/// null value indicates a valid filename that doesn't tie to a specific instrument (e.g. the song preview)
/// </summary>
static readonly Dictionary<string, Song.AudioInstrument?> validFilenames = new Dictionary<string, Song.AudioInstrument?>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs more specific naming, as this list of audio files is specific to Rock Band/Clone Hero. Regular guitar hero custom songs are not specific to these audio names.

m_HorizontalOverflow: 0
m_VerticalOverflow: 0
m_LineSpacing: 1
m_Text: Detect Files
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to "Auto-Detect Clone Hero Files", needs to be specific about the target game.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FireFox2000000 it seems that the image for the button isn't wide enough to fit that all on one line:
image

would you be ok with the text being "Auto-Detect CH Files"? if i shrink the font to 12, it fits on one line:
image

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be moved so that it's above the first "Load" and "X" buttons, as this really shouldn't be in line with the General/Audio/Advanced tabs in any case.

m_Script: {fileID: 11500000, guid: 635727e25c735994fb6f858716ba8247, type: 3}
m_Name:
m_EditorClassIdentifier:
message: Detects audio files from selected folder based on their filenames
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message needs updating as it does not completely reflect the current behaviour after changes.

if (instrument.HasValue)
{
LoadInstrumentAudioFromPath(instrument.Value, filePath);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self, add comment here explaining that the lack of else is a purposeful ignore (see validRBCloneHeroFilenames summary)

@FireFox2000000
Copy link
Owner

Code side looks good, will need to pull down the branch and evaluate the layout next.

@FireFox2000000
Copy link
Owner

Bug found-

Auto-detect CH Audio fails to load audio when user is prompted to select a folder via file explorer.

  1. Open Moonscraper in Unity Editor (Windows)
  2. New Song
  3. Audio tab
  4. Select "Auto-Detect CH Files"
  5. Select a song folder containing multiple audio files (Black Window of La Porte was used in testing)
  6. Note audio does not load.
    Upon some investigation, line 356 of SongPropertiesPanelController is stripping out 1 level of directories in the call Path.GetDirectoryName(currentDir). For example if the path retrieved is E:/GH3 Customs/11.E John 5 Featuring Jim Root - Black Widow of La Porte, the system tries to load from E:/GH3 Customs instead.

Fix requested

@FireFox2000000
Copy link
Owner

Bug found-

Multiple assert failures are logged when attempting to use the "Auto-Detect CH Files" button.

  1. Open Moonscraper in Unity Editor (Windows)
  2. Load a song containing multiple audio files (Black Widow of La Porte was used for testing purposes)
  3. Song Properties
  4. Audio tab
  5. Manually clear each stem by clicking the "X" button next to each loaded audio file
  6. Select "Auto-Detect CH Files"
  7. Note assertion failure in console log of Unity Editor for each audio file loaded.
Assertion failed
UnityEngine.Debug:Assert(Boolean)
<KickTasks>d__6:MoveNext() (at Assets/Scripts/Engine/LoadingTasksManager.cs:35)
System.Runtime.CompilerServices.AsyncVoidMethodBuilder:Start(<KickTasks>d__6&)
LoadingTasksManager:KickTasks(IList`1)
<SetAudio>d__51:MoveNext() (at Assets/Scripts/Game/UI/Menus/SongPropertiesPanelController.cs:410)
UnityEngine.MonoBehaviour:StartCoroutine(IEnumerator)
SongPropertiesPanelController:LoadInstrumentAudioFromPath(AudioInstrument, String) (at Assets/Scripts/Game/UI/Menus/SongPropertiesPanelController.cs:332)
SongPropertiesPanelController:LoadCloneHeroAudioFromDirectory() (at Assets/Scripts/Game/UI/Menus/SongPropertiesPanelController.cs:366)
UnityEngine.EventSystems.EventSystem:Update() (at C:/buildslave/unity/build/Extensions/guisystem/UnityEngine.UI/EventSystem/EventSystem.cs:377)

Fix requested.

var currentDir = ChartEditor.Instance.lastLoadedFile;
if (string.IsNullOrWhiteSpace(currentDir))
{
FileExplorer.OpenFolderPanel(out currentDir);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to handle the case of the user cancelling the open folder panel prompt. This should early return upon FileExplorer.OpenFolderPanel returning false.

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