Skip to content
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

BcatService: Return InternetRequestDenied for CmifCommand 10101 #6780

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Yohoki
Copy link
Contributor

@Yohoki Yohoki commented May 6, 2024

Implementation for BcatService.RequestSyncDeliveryCacheWithDirectoryName()

Instead of trying to connect to official servers, returns BcatResult.InternetRequestDenied.

This fixes #6769 , causing the game to be playable without "Ignore Missing Services". Skips the crash after Chapter 1-1. Also prompts an error message on trying to play online, alerting the user that internet access is not available.

@github-actions github-actions bot added the horizon Related to Ryujinx.HLE label May 6, 2024
@ryujinx-mako ryujinx-mako bot requested review from AcK77, gdkchan, TSRBerry and a team May 6, 2024 09:53
@Yohoki
Copy link
Contributor Author

Yohoki commented May 6, 2024

Quick note, some testing was done in Endless Ocean.

Returning Result.Success works to load main menu, but gives infinite loop on selecting online play.

Returning BcatResult.NotFound also woks. Tries to load stage on selecting online play mode, but fails quietly back to main menu.

Returning BcatResult.InternetRequestDenied gave the best results, allowing entry to main menu and prompting an error message before returning to main menu when online play is selected.

Copy link
Member

@gdkchan gdkchan left a comment

Choose a reason for hiding this comment

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

I think we should be consistent with what we do on RequestSyncDeliveryCache. It does not look like RequestSyncDeliveryCache returns an error.

We should probably implement both as if there was no internet connection available, but I'm not sure what it is supposed to return in this case. Also I'm not sure if that could have some undesirable side effect on a game that is currently working.

@@ -16,5 +16,14 @@ public Result RequestSyncDeliveryCache(out IDeliveryCacheProgressService deliver

return Result.Success;
}

[CmifCommand(10101)]
public Result RequestSyncDeliveryCacheWithDirectoryName(out IDeliveryCacheProgressService deliveryCacheProgressService)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would assume it would behave similarly if both return InternetRequestDenied. In Endless Ocean's case, it fails quietly in some cases and prompts an error message in others. I'm actually not sure why Result.Success is used here, since there is a BcatResult class that seems more appropriate.
image

I have updated the code now to include a directory parameter, although it was not failing without one. It's better to have it fleshed out completely for the future. Thanks!
public Result RequestSyncDeliveryCacheWithDirectoryName(out IDeliveryCacheProgressService deliveryCacheProgressService, LibHac.Bcat.DirectoryName arg1)

Copy link
Member

Choose a reason for hiding this comment

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

You should add a new DirectoryName type instead of using the LibHac type. On the branch that I linked you can find a DirectoryName struct (although you can also "port" the one from LibHac if you want). The struct should be located on src/Ryujinx.Horizon/Bcat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to quickly implement that, but it caused a lot of build errors. I'll have a better look at it when I get home.

'DirectoryName' is an ambiguous reference between 'Ryujinx.Horizon.Sdk.Bcat.DirectoryName' and 'LibHac.Bcat.DirectoryName'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I've set up the blank DirectoryName struct and pushed for that in the arguments. I also resolved the conflicts between Bcat.DirectoryName and LibHac.Bcat.DirectoryName . The old functions that may rely on LibHac still use it, so nothing should be broken there.

The LibHac version looks well made, but I don't think it's my business to touch something that complex. :)

@ryujinx-mako ryujinx-mako bot requested a review from a team May 6, 2024 19:44
@AcK77 AcK77 requested a review from gdkchan May 14, 2024 14:38
@gdkchan
Copy link
Member

gdkchan commented May 16, 2024

I think it's better to do the same as RequestSyncDeliveryCache and return Result.Success. As I said before, I think those functions should be consistent, and the change with lowest risk here is returning Success (as we don't need to change RequestSyncDeliveryCache implementation then).

@Yohoki
Copy link
Contributor Author

Yohoki commented May 16, 2024

I think it's better to do the same as RequestSyncDeliveryCache and return Result.Success. As I said before, I think those functions should be consistent, and the change with lowest risk here is returning Success (as we don't need to change RequestSyncDeliveryCache implementation then).

I will not change RequestSyncDeliveryCacheWithDirectoryName to return Result.Success. It causes infinite loop/loading screens in the game I have tested it with. If there are other games that use this call, I would test them, but I don't know any. The lowest risk here, in my opinion, is to assume that the game does not have internet connectivity and allow it to fail gracefully.

The options, at least for this game are as follows:

  • Result.Success
    Results in infinite loading screen.
  • BcatResult.NotFound
    Results in failed load, and return to main menu.
  • BcatResult.InternetRequestDenied || BcatResult.Success
    Results in failed load, a user friendly prompt that there was a connection error and returns to same menu. (Not main menu)

I can provide screenshots/recordings of these outcomes if requested.

-Edit- Confused BcatResult.Success with Result.Success. Fixed.

@gdkchan
Copy link
Member

gdkchan commented May 16, 2024

You originally said

Returning Result.Success works to load main menu, but gives infinite loop on selecting online play.

Which makes it sound like that returning Result.Success works, except for online play (which will not work anyway, so it's not a big deal).

@Yohoki
Copy link
Contributor Author

Yohoki commented May 17, 2024

Ah. You're right. It's been a week and I confused BcatResult.Success and Result.Success anyway. If you're good with it, I will change it to BcatResult.Success, because that mimics the no internet result. If not, I can change it to Result.Success and deal with the loop. At least the game boots now.

I've tried dumping another game that uses this method for testing, but it crashes before loading the menu (even on main branch) if lan/ldn is enabled. It ends up crashing trying to write to an invalid address. So can't really test that one and compare the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
horizon Related to Ryujinx.HLE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BcatService command ID: 10101 (RequestSyncDeliveryCacheWithDirectoryName) is not implemented
2 participants