HLE: Refactoring of ApplicationLoader #4480

AcK77 · 2023-02-25T12:51:39Z

This PR attempt to refactor ApplicationLoader in a better way for avoid duplicated code and helping in a potential multi-processes support in the future.

In that way, some things are a bit "wrong" but should works as expected since we can only run one Application at a time.
I've done some loading testing but it could be nice to test different loading too, like Homebrew, NSP Homebrew, games using jit services, and more...

github-actions · 2023-02-25T13:17:47Z

Download the artifacts for this pull request:

Experimental GUI (Avalonia)

GUI-less (SDL2)

Only for Developers

gdkchan

Took a quick look, I will likely submit another review after testing it a bit and checking out the changes locally.

Ryujinx.Ava/AppHost.cs

Ryujinx.HLE/HOS/ModLoader.cs

Ryujinx.HLE/Loaders/Processes/Extensions/NcaExtensions.cs

Ryujinx.HLE/Loaders/Processes/ProcessInfo.cs

Ryujinx.Ui.Common/App/ApplicationLibrary.cs

Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs

Ryujinx.HLE/Loaders/Processes/ProcessResult.cs

TSRBerry

Just found a few typos

Ryujinx.HLE/Loaders/Processes/Extensions/PartitionFileSystemExtensions.cs

Ryujinx.Ui.Common/App/ApplicationLibrary.cs

gdkchan

lgtm, I tested it a bit here, but did not test it extensively, so it might benefit from more testing in different scenarios.

TSRBerry · 2023-03-12T14:06:16Z

I'm not able to start homebrew applications with this PR.

The crash below was produced using the hello-world example from switchbrew/switch-examples.

crash snippet:

00:00:00.362 |I| Application LoadGuestApplication: Loading as homebrew.
00:00:00.376 |W| Loader GetNpdm: NPDM file not found, using default values!
00:00:00.395 |E| Application : Unhandled exception caught: LibHac.Common.HorizonResultException: ResultLoaderInvalidMeta (2009-0004)
   at LibHac.Common.ThrowHelper.ThrowResult(Result result)
   at LibHac.Result.ThrowIfFailure()
   at Ryujinx.HLE.Loaders.Processes.Extensions.MetaLoaderExtensions.LoadDefault(MetaLoader metaLoader) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/Extensions/MetaLoaderExtensions.cs:line 38
   at Ryujinx.HLE.Loaders.Processes.Extensions.FileSystemExtensions.GetNpdm(IFileSystem fileSystem) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/Extensions/FileSystemExtensions.cs:line 26
   at Ryujinx.HLE.Loaders.Processes.ProcessLoader.LoadNxo(String path) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs:line 178
   at Ryujinx.HLE.Switch.LoadProgram(String fileName) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Switch.cs:line 90
   at Ryujinx.Ava.AppHost.LoadGuestApplication() in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.Ava/AppHost.cs:line 605
   at Ryujinx.Ava.UI.ViewModels.MainWindowViewModel.<>c__DisplayClass369_0.<<LoadApplication>g__Action|0>d.MoveNext() in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.Ava/UI/ViewModels/MainWindowViewModel.cs:line 1694
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state)
   at Avalonia.Threading.AvaloniaSynchronizationContext.<>c__DisplayClass5_0.<Post>b__0() in /_/src/Avalonia.Base/Threading/AvaloniaSynchronizationContext.cs:line 33
   at Avalonia.X11.X11PlatformThreading.CheckSignaled() in /_/src/Avalonia.X11/X11PlatformThreading.cs:line 164
   at Avalonia.X11.X11PlatformThreading.RunLoop(CancellationToken cancellationToken) in /_/src/Avalonia.X11/X11PlatformThreading.cs:line 244
   at Avalonia.Threading.Dispatcher.MainLoop(CancellationToken cancellationToken) in /_/src/Avalonia.Base/Threading/Dispatcher.cs:line 65
   at Avalonia.Controls.ApplicationLifetimes.ClassicDesktopStyleApplicationLifetime.Start(String[] args) in /_/src/Avalonia.Controls/ApplicationLifetimes/ClassicDesktopStyleApplicationLifetime.cs:line 120
   at Avalonia.ClassicDesktopStyleApplicationLifetimeExtensions.StartWithClassicDesktopLifetime[T](T builder, String[] args, ShutdownMode shutdownMode) in /_/src/Avalonia.Controls/ApplicationLifetimes/ClassicDesktopStyleApplicationLifetime.cs:line 209
   at Ryujinx.Ava.Program.Main(String[] args) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.Ava/Program.cs:line 51

TSRBerry

Lgtm!

I'll ask for some community tests, before we merge this.
I tested cheats and homebrew applications, but haven't tested a lot of other stuff.

MisterbALLIN · 2023-03-12T16:21:57Z

Tested Splatoon 3 with the Inkopolis DLC on Update 3.0.1. Everything seems good

LukeWarnut · 2023-03-12T19:16:04Z

Tested Splatoon 3 with the Flexlion mod and DLCs, Hollow Knight, Resident Evil 4, and Dead Cells + DLC; all using the latest updates. No regressions found.

With the amount of commits I'm adding, another review will be required.

TSRBerry · 2023-03-13T01:28:43Z

I found a few more issues:

If ProcessResult.Failed was returned Ryujinx would crash with a NRE.

Example crash

```
00:00:00.351 |I| Application LoadGuestApplication: Loading as NSP.
00:00:00.355 |E| Loader Load: Unable to load: Could not find Main NCA
00:00:00.361 |E| Application : Unhandled exception caught: System.NullReferenceException: Object reference not set to an instance of an object.
   at Ryujinx.HLE.Loaders.Processes.Extensions.MetaLoaderExtensions.GetProgramId(MetaLoader metaLoader) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/Extensions/MetaLoaderExtensions.cs:line 15
   at Ryujinx.HLE.Loaders.Processes.ProcessResult..ctor(MetaLoader metaLoader, ApplicationControlProperty applicationControlProperties, Boolean diskCacheEnabled, Boolean allowCodeMemoryForJit, IDiskCacheLoadState diskCacheLoadState, UInt64 pid, Byte mainThreadPriority, UInt32 mainThreadStackSize) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/ProcessResult.cs:line 48
   at Ryujinx.HLE.Loaders.Processes.ProcessResult.get_Failed() in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/ProcessResult.cs:line 12
   at Ryujinx.HLE.Loaders.Processes.Extensions.PartitionFileSystemExtensions.Load(PartitionFileSystem partitionFileSystem, Switch device, String path) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/Extensions/PartitionFileSystemExtensions.cs:line 163
   at Ryujinx.HLE.Loaders.Processes.ProcessLoader.LoadNsp(String path) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs:line 61
   at Ryujinx.HLE.Switch.LoadNsp(String nspFile) in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.HLE/Switch.cs:line 85
   at Ryujinx.Ava.AppHost.LoadGuestApplication() in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.Ava/AppHost.cs:line 595
   at Ryujinx.Ava.UI.ViewModels.MainWindowViewModel.<>c__DisplayClass369_0.<<LoadApplication>g__Action|0>d.MoveNext() in /mnt/Dev/git-repos/Nintendo/Ryujinx/Ryujinx.Ava/UI/ViewModels/MainWindowViewModel.cs:line 1694
```

Error logging was a little confusing when loading nx-hbloader, as it would first log an error and then continue to load without issues.
None of the GUIs were checking the results of the Load() calls.

This avoids logging "Could not find Main NCA" followed by "Loading main..." when trying to start hbl.

TSRBerry · 2023-03-17T14:16:29Z

Rebased, ready for another review!

Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs

Ryujinx.HLE/Loaders/Processes/Extensions/FileSystemExtensions.cs

Ryujinx.HLE/Loaders/Processes/Extensions/NcaExtensions.cs

Ryujinx.HLE/Loaders/Processes/Extensions/PartitionFileSystemExtensions.cs

Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs

gdkchan · 2023-03-18T13:53:41Z

Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs

+
+            (bool success, ProcessResult processResult) = partitionFileSystem.TryLoad(_device, path, out string errorMessage);
+
+            if (processResult.ProcessId == 0)


This seems a bit odd, is it using ProcessId == 0 as an indication that the load failed? Why does it not just check if success is false?

Most of the other methods are doing this too (since they don't have a success return value). So maybe it's better to keep it like this for the sake of consistency. Also I didn't check the case where it returns success, so it's possible that it wouldn't even work here, but that would be a bit misleading (you would assume that if success is true, everything loaded correctly).

Yeah, it will only return true if loading succeeded here.
I agree it's a bit confusing, but wasn't sure to change the check here if we are doing it this way for every other load method like this.

Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs

Ryujinx.HLE/Loaders/Processes/ProcessResult.cs

Ryujinx.HLE/Loaders/Processes/Extensions/FileSystemExtensions.cs

Ryujinx.HLE/Loaders/Processes/ProcessLoaderHelper.cs

Ryujinx.Ui.Common/App/ApplicationLibrary.cs

Ryujinx/Ui/MainWindow.cs

AcK77 added enhancement New feature or request horizon Related to Ryujinx.HLE labels Feb 25, 2023

AcK77 requested review from marysaka, gdkchan and TSRBerry February 25, 2023 12:51

gdkchan reviewed Feb 25, 2023

View reviewed changes

TSRBerry reviewed Feb 25, 2023

View reviewed changes

Ryujinx.HLE/Loaders/Processes/Extensions/PartitionFileSystemExtensions.cs Outdated Show resolved Hide resolved

Ryujinx.Ui.Common/App/ApplicationLibrary.cs Outdated Show resolved Hide resolved

AcK77 force-pushed the app-loader branch from 77c746e to e57bbb4 Compare March 4, 2023 14:31

gdkchan approved these changes Mar 4, 2023

View reviewed changes

TSRBerry previously approved these changes Mar 12, 2023

View reviewed changes

TSRBerry added the needs-feedback The needs feedbacks from the community label Mar 12, 2023

TSRBerry requested review from gdkchan, emmauss and riperiperi March 13, 2023 01:30

TSRBerry mentioned this pull request Mar 14, 2023

UI: Expose games build ID for cheat management #4340

Merged

marysaka approved these changes Mar 16, 2023

View reviewed changes

AcK77 added 8 commits March 17, 2023 14:18

HLE: Refactoring of ApplicationLoader

6363af2

Fix SDL2 Headless

54691d9

Addresses gdkchan feedback

efbed12

Fixes LoadUnpackedNca RomFS loading

be80ac1

remove useless casting

e5dbc61

Cleanup and fixe empty application name

46fed10

Remove ProcessInfo

6e4e1a3

Fixes typo

2dd0276

AcK77 and others added 10 commits March 17, 2023 14:19

ActiveProcess to ActiveApplication

4daf847

Update check

3084b71

Clean using.

82357e1

Use the correct filepath when loading Homebrew.npdm

f42142f

Fix NRE in ProcessResult if MetaLoader is null

43ea81b

Add more checks for valid processId & return success

60775a7

Add missing logging statement for npdm error

c81ee7c

Return result for LoadKip()

ec303f9

Move error logging out of PFS load extension method

6514305

This avoids logging "Could not find Main NCA" followed by "Loading main..." when trying to start hbl.

Fix GUIs not checking load results

e0782b0

TSRBerry force-pushed the app-loader branch from 837e36f to e0782b0 Compare March 17, 2023 14:15

gdkchan reviewed Mar 18, 2023

View reviewed changes

Ryujinx.HLE/Loaders/Processes/ProcessLoader.cs Outdated Show resolved Hide resolved

gdkchan mentioned this pull request Mar 18, 2023

[Bug] Fire Emblem Engage crashes on macOS Silicon #4548

Closed

gdkchan reviewed Mar 18, 2023

View reviewed changes

Fix style and formatting issues

b706867

TSRBerry force-pushed the app-loader branch from 03648fa to b706867 Compare March 18, 2023 16:34

TSRBerry requested a review from gdkchan March 18, 2023 16:34

gdkchan reviewed Mar 21, 2023

View reviewed changes

TSRBerry added 2 commits March 21, 2023 20:55

Fix formatting and wording

b1c7216

gtk: Refactor LoadApplication()

bde7b79

TSRBerry requested a review from gdkchan March 21, 2023 20:34

marysaka merged commit 4c2d9ff into Ryujinx:master Mar 31, 2023

TSRBerry mentioned this pull request Apr 5, 2023

HLE: Deal with empty title names properly #4643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLE: Refactoring of ApplicationLoader #4480

HLE: Refactoring of ApplicationLoader #4480

AcK77 commented Feb 25, 2023

github-actions bot commented Feb 25, 2023 •

edited

Loading

gdkchan left a comment

TSRBerry left a comment

gdkchan left a comment

TSRBerry commented Mar 12, 2023

TSRBerry left a comment

MisterbALLIN commented Mar 12, 2023

LukeWarnut commented Mar 12, 2023

TSRBerry commented Mar 13, 2023 •

edited

Loading

TSRBerry commented Mar 17, 2023

gdkchan Mar 18, 2023

gdkchan Mar 18, 2023 •

edited

Loading

TSRBerry Mar 18, 2023


		(bool success, ProcessResult processResult) = partitionFileSystem.TryLoad(_device, path, out string errorMessage);

		if (processResult.ProcessId == 0)

HLE: Refactoring of ApplicationLoader #4480

HLE: Refactoring of ApplicationLoader #4480

Conversation

AcK77 commented Feb 25, 2023

github-actions bot commented Feb 25, 2023 • edited Loading

gdkchan left a comment

Choose a reason for hiding this comment

TSRBerry left a comment

Choose a reason for hiding this comment

gdkchan left a comment

Choose a reason for hiding this comment

TSRBerry commented Mar 12, 2023

TSRBerry left a comment

Choose a reason for hiding this comment

MisterbALLIN commented Mar 12, 2023

LukeWarnut commented Mar 12, 2023

TSRBerry commented Mar 13, 2023 • edited Loading

TSRBerry commented Mar 17, 2023

gdkchan Mar 18, 2023

Choose a reason for hiding this comment

gdkchan Mar 18, 2023 • edited Loading

Choose a reason for hiding this comment

TSRBerry Mar 18, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 25, 2023 •

edited

Loading

TSRBerry commented Mar 13, 2023 •

edited

Loading

gdkchan Mar 18, 2023 •

edited

Loading