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

Adhoc fix + UPnP + Multiple Instance #13132

Closed
wants to merge 7 commits into from
Closed

Conversation

anr2me
Copy link
Collaborator

@anr2me anr2me commented Jul 12, 2020

These might be able to fix more games, but it's experimental (based on trial-and-error method without confirming it on real hardware), dirty, and badly written.

Win32&64 Builds: https://www.dropbox.com/s/i6699cyf1tzu29b/PPSSPP_1.10.3-adhocfix_Win32x64.zip?dl=0
Android ARMv7 Build: https://www.dropbox.com/s/dmz6jvesmm3uwp2/PPSSPP_1.10.3-adhocfix_ARMv7.apk?dl=0

I haven't been able to continue working on this for months and probably not in the future either.
Since i won't be updating this anymore (other than making sure it can be build successfully), here are the TODO list i remembered and was planning to do:
1). Grouping related functions into classes (AdhocControl, AdhocMatching, AdhocPDP, AdhocPTP,etc like what JPCSP did) for cleaner and easier to work with future improvements.
2). Replacing some real threads with fake PSP threads (ie. AdhocMatchingEvent, AdhocMatchingInput, etc) since some of them are using sceNet functions (like PdpSend/PdpRecv) which may cause issue when blocking-socket implementation are being done in the future (ie. using hle functions)
3). Implements blocking-socket in similar way to sceIo's Read/Write.
4). Replaces state polling inside sceNet function (to solve issues related to high latency over the internet) with hleDelayResult+CoreTiming::ScheduleEvent (similar to blocking-socket implementation) instead of polling it with sleep_ms which cause stutters and emulation performance degradations.
5). Add an option to show all players latency (current & highest) and packet lost rate (similar to FPS rate)

Hopefully someone can make a cleaner and easier to understand adhoc code and contribute it to official PPSSPP so that new/other contributors/devs can continue working on adhoc code without getting a headache too much LOL

@boy13epic
Copy link

are You AdamN?

@hrydgard
Copy link
Owner

Quite an impressive set of changes! I wish I knew enough about AdHoc to properly review this code and work on it...

I do agree that we should finally get the change that allows multi instance on one PC in.

I see that there are details of fixed games in the commits, but do you have a more organized list of what games are actually expected to work with this change in? Are you aware of any regressions?

The timing changes (from PSP time to real time in a couple of places) do make sense in a networking context but also do feel a little scary.

@ghost
Copy link

ghost commented Jul 12, 2020

Me and some friends made a list of games that work on both single PC and over the internet :
https://docs.google.com/spreadsheets/d/1KJz8NBitTcwM9Kvt2bQrE0uCbAYPn4ve5ZeAV5OaQnU
If someone wanna know the changes and regressions (some of them are based on an older build though).
I didn't test every game with adhoc thought but I think it's enough.
I have a better list but its not ready and currently private until further investigation.

@ghost
Copy link

ghost commented Jul 12, 2020

@anr2me @hrydgard
Hi,it is me again.I have been wondering,if code was to say rewritten,would it mean it can break some games that worked before?I'm curious at this point because it would be lot of trouble to have games that worked,fix if they would be broken upon merging this.I would double test this because lot of games that work could be broken with this.I don't need theory,we need proof.

TIA

@adenovan
Copy link
Contributor

when its merged make sure be ready to host #12340 as well

@@ -76,7 +76,7 @@ bool isLocalServer = false;
uint8_t PPSSPP_ID = 0;
sockaddr LocalhostIP;
sockaddr LocalIP;
int defaultWlanChannel = 1;
int defaultWlanChannel = PSP_SYSTEMPARAM_ADHOC_CHANNEL_1; // Don't put 0(Auto) here, it needed to be a valid/actual channel number
Copy link
Contributor

Choose a reason for hiding this comment

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

better use 11 on this one to match psp behavior #10698

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i was using 11 when i find out JPCSP using 11 as default, but later changed it back to 1 just to be different LOL

Copy link
Contributor

Choose a reason for hiding this comment

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

hahaha its fun to find the reason of default behind that 1 and 11 , the test has spoken the truth behind that java code

LOL

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

It looks like the only timeout changes are network related, right?

-[Unknown]

Common/Log.h Outdated
@@ -91,7 +91,7 @@ void GenericLog(LogTypes::LOG_LEVELS level, LogTypes::LOG_TYPE type,
bool GenericLogEnabled(LogTypes::LOG_LEVELS level, LogTypes::LOG_TYPE type);

#if defined(LOGGING) || defined(_DEBUG) || defined(DEBUGFAST) || defined(_WIN32)
#define MAX_LOGLEVEL DEBUG_LEVEL
#define MAX_LOGLEVEL VERBOSE_LEVEL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we want to change this?

-[Unknown]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't see any verbose log when debugging without it, any better way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

btw @hrydgard / @unknownbrackets is there anyway we can see Verbose logs in PPSSPP Debug Console Log without changing the source code like this?

Because changing the Logging Channel of SCENET to "Verb" from PPSSPP Settings doesn't seems to be working :( (was it a bug?)
And changing the source code + rebuild whenever we want to see verbose log is kinda troublesome isn't?

@@ -885,15 +1065,23 @@ static int sceNetAdhocctlScan() {
return ERROR_NET_ADHOCCTL_DISCONNECTED; // ERROR_NET_ADHOCCTL_BUSY
}

// Does Connected Event's mipscall need be executed after returning from sceNetAdhocctlScan ?
Copy link
Contributor

Choose a reason for hiding this comment

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

yep we can only do scan before updating Adhocctlhandler into connected state , scanning on CONNECT STATE lead to crash on real psp

freeFriendsRecursive(friends, &peercount);
INFO_LOG(SCENET, "Cleared Peer List (%i)", peercount);
// Delete Peer Reference
friends = NULL;
//May also need to clear Handlers
adhocctlHandlers.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

will be my next note on testing with auto test does term really removing the adhocctl handler?


// Success
return 0;
}

// Timeout occured
return ERROR_NET_ADHOC_TIMEOUT;
return ERROR_NET_ADHOC_CONNECTION_REFUSED; // ERROR_NET_ADHOC_TIMEOUT;
Copy link
Contributor

Choose a reason for hiding this comment

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

this one looks scary and probably can be regression over internet and real psp using aemu plugin , note this one if some game compability broken or not in high latency

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As i remembered i took that return code from JPCSP, it seems they only use a few error codes instead of the whole set like we do.

Copy link
Contributor

Choose a reason for hiding this comment

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

we have a whole set of error code but its not precise like real console return so its broke some register often , im trying to increase the precision of the error by messing with the control and especially this api on digimon games , due to aemu code often make the game timeout to reduce latency to trick some games this is need a proper testing from the community that use upnp to play.

Copy link
Contributor

Choose a reason for hiding this comment

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

embarking quest on monster hunter does relying a lot in this api before the game decide to drop a peer.

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 12, 2020

Quite an impressive set of changes! I wish I knew enough about AdHoc to properly review this code and work on it...

I do agree that we should finally get the change that allows multi instance on one PC in.

I see that there are details of fixed games in the commits, but do you have a more organized list of what games are actually expected to work with this change in? Are you aware of any regressions?

The timing changes (from PSP time to real time in a couple of places) do make sense in a networking context but also do feel a little scary.

There might be some regression over the internet i think, as i remembered someone mentioning Monster Hunter games, since i couldn't test it over the internet i never know what happened.
I can only tested it on LAN and localhost and found no problem, while some people on discord are testing it over the internet.

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 12, 2020

For the rest of the compilation error,
on Android seems to be related to CMakeLists.txt which i'm not familiar with :( i guess i won't be able to fix it.

on Windows it seems to be related to Windows SDK version, since i don't have the latest one i can't change it either, i'll let you guys change it.

PS: I've reuploaded the prebuild binaries (Windows & Android) because the previous one were having connection issue (clearly seen on GTA VCS where it can't see any game room)

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 12, 2020

It looks like the only timeout changes are network related, right?

-[Unknown]

yes, only timeout used in adhoc codes.


// Connect Socket to Peer (Nonblocking)
// NOTE: Based on what i read at stackoverflow, The First Non-blocking POSIX connect will always returns EAGAIN/EWOULDBLOCK because it returns without waiting for ACK/handshake, But GvG Next Plus is treating non-blocking PtpConnect just like blocking connect, May be on a real PSP the first non-blocking sceNetAdhocPtpConnect can be successfull?
Copy link
Collaborator Author

@anr2me anr2me Jul 12, 2020

Choose a reason for hiding this comment

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

@adenovan you might want to confirm this on real PSP.
Based on how GvG Next Plus is using non-blocking PtpConnect like a blocking one making me to have suspicion, that a non-blocking PtpConnect might have the possibility to be successfully connected on the first attempt.

While a non-blocking POSIX connect are not possible to be successful on the first attempt due to returning without waiting for ACK/handshake (at least based on what i read at stackoverflow, never tested this my self)

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 13, 2020

Okay i'm in a mess now :( i solve a simple conflict on proAdhoc.h through github directly, which apparently creating a merge commit instead of rebase, and ended making a lot of conflicts with my local branch, so i deleted my local branch and re-pull from adhoc_fix at github, and now i don't know how to rebase it to remove that merge and make my commits to be on top :( without deleting the whole adhoc_fix branch on github that is

Update1: i'm able to remove that merge commit and rebase it on my local repo, but i don't know how to remove that merge commit from github branch :( trying to push it got rejected

! [rejected] adhoc_fix -> adhoc_fix (non-fast-forward)
error: failed to push some refs to 'https://github.com/ANR2ME/ppsspp.git'
hint: Updates were rejected because the tip of your current branch is behind

Update2: forced push seems to do the trick :)

@unknownbrackets
Copy link
Collaborator

Even in the worst case, you can always use git reflog to get back to your old commits.

All you need to do though is the following:

git fetch upstream
git rebase upstream/master
git push origin -f adhoc_fix

This assumes "upstream" is the name of the hrydgard remote and "origin" is the ANR2ME remote, which is the common convention. The important thing here is -f to tell git it's okay to overwrite the branch head even though it no longer has the merge in it.

-[Unknown]

@ghost
Copy link

ghost commented Jul 13, 2020

@anr2me
Hi,me again.If i understand correctly Mon Hun games won't work like they did before the fix?If so,then what is point of this fix if it broke Mon Hun games,which run best on Ad-Hoc;tested myself every game for PSP.Can we talk about this,how it can really break something.

TIA

@ghost
Copy link

ghost commented Jul 13, 2020

@anr2me
Hi,me again.If i understand correctly Mon Hun games won't work like they did before the fix?If so,then what is point of this fix if it broke Mon Hun games,which run best on Ad-Hoc;tested myself every game for PSP.Can we talk about this,how it can really break something.

TIA

I think that monster hunter might work on the current build because I tried Worms Open Warfare 2 and it works and it uses the same adhoc functions as MH.
Maybe try and see.
It's a shame that the most requested game to work online Kingdom Hearts BBS is still getting issues...
Maybe one day.

@ghost
Copy link

ghost commented Jul 13, 2020

Ad-Hoc is my temporary to-do project anyways,i just want to play the best games and finish them,same what i did with GBA,might take lot of months,but i hope i will do all i plan without any bugs in the way.Anyways,thank you.

@ghost
Copy link

ghost commented Jul 13, 2020

@mojojojodojo
Hi,me again.I don't want to sound rude but comparing what emu-mp.net and GuenosNoLife,you just scratched tip of an iceberg regarding testing.I won't say 78 is not much,but comparing what people did with emulator itself,i think games are based on interest of play,not to be sure what is good and what not.

@boy13epic
Copy link

boy13epic commented Jul 13, 2020

@anr2me @hrydgard
Hi,it is me again.I have been wondering,if code was to say rewritten,would it mean it can break some games that worked before?I'm curious at this point because it would be lot of trouble to have games that worked,fix if they would be broken upon merging this.I would double test this because lot of games that work could be broken with this.I don't need theory,we need proof.

TIA

stop trashing MojoJojoDojo’s list.

@ghost
Copy link

ghost commented Jul 13, 2020

@boy13epic
Hi.Who said i was trashing MojoJojoDojo's list,do you get that out of context?I said that his list can be better if he goes by testing everything he wants,not he finds interest in.Please if you found it offensive i will delete my message,but if it doesn't don't say something offensive just because you read between lines or because you don't like my suggestion,and i believe we should be free to express and tell what we want.

TIA

@hrydgard
Copy link
Owner

Let's stop talking about lists.

I'm not quite sure where to start here - I'm thinking about pulling out pieces of this and merging them one by one, so we can reduce any breakage to a minimum through careful testing of each part.

For example, the UPNP stuff should be mergable mostly on its own.

Does that sound resonable?

@ghost
Copy link

ghost commented Jul 13, 2020

@hrydgard
I'm not developer by any means,but does that mean we would have and fixes if we found MH games broken?That would be good as it sounds,but that is going to be lot of work.

@boy13epic
Copy link

this is resonable

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 13, 2020

Let's stop talking about lists.

I'm not quite sure where to start here - I'm thinking about pulling out pieces of this and merging them one by one, so we can reduce any breakage to a minimum through careful testing of each part.

For example, the UPNP stuff should be mergable mostly on its own.

Does that sound resonable?

That would be great! I realized it too late for making a mistake by mixing bug fix with feature addition on the same branch until it's getting to large :(

Regarding UPnP, i tried to make it general purpose without importing any adhoc specific header, so you guys can use it for whatever feature that need port forwarding in the future. But currently UPnP initialization and cleanup are done within __NetInit() and __NetShutdown() which requires a game to run to be triggered, so you might want to move it somewhere else if it need to be usable without running a game(ie. for remote disc), preferably UPnP stuff to be running asynchronously on a separate thread to prevent delays/stutters from communications with the router (due to blocking socket), which could get worse for user experience if UPnP was enabled while not having a UPnP supported router (which resulting a few seconds delay due to getting timeout, 2 seconds is the default from miniUPnPc example probably for a very slow & overloaded router)

Btw, some of the commits might includes minor changes that might not have anything todo with what's written in the comment, so you might want to remove those unrelated parts when cherry picking it. For example in UPnP commits if you found files related to adhoc it's probably unrelated to UPnP (other than for UPnP usage)

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 13, 2020

@anr2me
Hi,me again.If i understand correctly Mon Hun games won't work like they did before the fix?If so,then what is point of this fix if it broke Mon Hun games,which run best on Ad-Hoc;tested myself every game for PSP.Can we talk about this,how it can really break something.

TIA

Actually you just need to test out the builds i provides on top of this page to see whether it really breaks any previously working games or not? if you're really curious, and don't forget to let us know the result :-)

If you're able to play multiplayer games for more than 5 minutes without problem, it will most likely have no issue (unless if single player was also having an issue)

@ghost
Copy link

ghost commented Jul 13, 2020

@anr2me
Issue is,UPnP kinda pulls issues on MHFU?Could you see why it is like that?

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 13, 2020

@anr2me
Issue is,UPnP kinda pulls issues on MHFU?Could you see why it is like that?

What kind of issue?
If you see a red message on screen saying "Failed/Unable to detected UPnP device" (or something like that), it means you don't have UPnP capable router (or haven't enable UPnP on your router)

If you're playing over the internet with other people using a different PPSSPP build than this build, you need to disable the "UPnP use original port" setting, it's enabled by default to maintain compatibility with real PSP and other/non-PPSSPP emulators not supporting port offset (only effective if you have port offset > 0), so that real PSP or non-PPSSPP emulators will see it as their own over the internet (external port doesn't use port offset, but locally still mapped to ports using port offset).

anr2me added 3 commits July 23, 2020 07:30
…oining a group. (Fix GTA VCS failed to join a group and unable to see any room)
…ing multiplayer game due to blocking socket behavior on miniUPnP
@anr2me
Copy link
Collaborator Author

anr2me commented Jul 23, 2020

I'll removes some of these commits that are no longer needed

@Lemoncak3
Copy link

Hello may i ask about dynasty warrior strikeforce? The adhoc works fine and we can enter online city together but whenever we send item trade, friend request, mission request other players got nothing so we only can wander in the city.

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 23, 2020

Hello may i ask about dynasty warrior strikeforce? The adhoc works fine and we can enter online city together but whenever we send item trade, friend request, mission request other players got nothing so we only can wander in the city.

Is this a regression that was working before? or it's been like that on older version too?

@Lemoncak3
Copy link

I habe tried this almost a year ago with same issue though. Because i really want to play with my brpther and sis. Today i tried it again with the latest build of ppsspp.

@anr2me
Copy link
Collaborator Author

anr2me commented Jul 23, 2020

I habe tried this almost a year ago with same issue though. Because i really want to play with my brpther and sis. Today i tried it again with the latest build of ppsspp.

Well if it's not working properly since last year you should create a new issue/bug report (if it's not already existed yet) and wait until someone could investigate it.

…tform and forcing the definition to what windows use might be a bad idea.
@anr2me
Copy link
Collaborator Author

anr2me commented Jul 23, 2020

Hmm.. when i tried to run Patapon 3 or DBZ:SB2 using Debug build i'm getting this crash: (Release build doesn't have this issue)
image

With this in the Output logs: (duplicated probably because i tried to press the Continue button several times)

D3D11 WARNING: ID3D11DeviceContext::OMSetRenderTargets: Resource being set to OM RenderTarget slot 0 is still bound on input! [ STATE_SETTING WARNING #9: DEVICE_OMSETRENDERTARGETS_HAZARD]
D3D11: **BREAK** enabled for the previous message, which was: [ WARNING STATE_SETTING #9: DEVICE_OMSETRENDERTARGETS_HAZARD ]
Exception thrown at 0x00007FF802833E49 (KernelBase.dll) in PPSSPPDebug64.exe: 0x0000087A (parameters: 0x0000000000000002, 0x0000007C034FAC90, 0x0000007C034FCA60).
Unhandled exception at 0x00007FF802833E49 (KernelBase.dll) in PPSSPPDebug64.exe: 0x0000087A (parameters: 0x0000000000000002, 0x0000007C034FAC90, 0x0000007C034FCA60).

D3D11 WARNING: ID3D11DeviceContext::OMSetRenderTargets[AndUnorderedAccessViews]: Forcing PS shader resource slot 1 to NULL. [ STATE_SETTING WARNING #7: DEVICE_PSSETSHADERRESOURCES_HAZARD]
D3D11: **BREAK** enabled for the previous message, which was: [ WARNING STATE_SETTING #7: DEVICE_PSSETSHADERRESOURCES_HAZARD ]
Exception thrown at 0x00007FF802833E49 (KernelBase.dll) in PPSSPPDebug64.exe: 0x0000087A (parameters: 0x0000000000000002, 0x0000007C034FAC90, 0x0000007C034FCA60).
Unhandled exception at 0x00007FF802833E49 (KernelBase.dll) in PPSSPPDebug64.exe: 0x0000087A (parameters: 0x0000000000000002, 0x0000007C034FAC90, 0x0000007C034FCA60).

The thread 0x34bc has exited with code 0 (0x0).
D3D11 WARNING: ID3D11DeviceContext::OMSetRenderTargets: Resource being set to OM RenderTarget slot 0 is still bound on input! [ STATE_SETTING WARNING #9: DEVICE_OMSETRENDERTARGETS_HAZARD]
D3D11: **BREAK** enabled for the previous message, which was: [ WARNING STATE_SETTING #9: DEVICE_OMSETRENDERTARGETS_HAZARD ]
Exception thrown at 0x00007FF802833E49 (KernelBase.dll) in PPSSPPDebug64.exe: 0x0000087A (parameters: 0x0000000000000002, 0x0000007C034FAC90, 0x0000007C034FCA60).
Unhandled exception at 0x00007FF802833E49 (KernelBase.dll) in PPSSPPDebug64.exe: 0x0000087A (parameters: 0x0000000000000002, 0x0000007C034FAC90, 0x0000007C034FCA60).

@hrydgard
Copy link
Owner

That would seem to be a bug in the D3D11 backend that should be reported separately - that can't really have anything directly to do with this PR.

hrydgard added a commit that referenced this pull request Jul 23, 2020
AdHoc Reconnect fixes, extracted from #13132 by ANR2ME
hrydgard added a commit that referenced this pull request Jul 23, 2020
Do UPnP on a thread to avoid stutter. Extracted from #13132 by ANR2ME
@hrydgard
Copy link
Owner

It is done!

This can be closed now. I hope we didn't break too many things - but at least now we can narrow down the range a lot easier by directing people to buildbot builds.

@hrydgard hrydgard closed this Jul 23, 2020
@hrydgard hrydgard added this to the v1.11.0 milestone Jul 23, 2020
@anr2me
Copy link
Collaborator Author

anr2me commented Jul 23, 2020

Awesome! Thanks for the hard work in splitting this up :D

PS: Some players probably want to have a separated controller config for each instance in the future

@hrydgard
Copy link
Owner

hrydgard commented Jul 23, 2020

Oh by the way I think you're only seeing it in a debug build because D3D11 validation is only active in debug. Likely drivers don't really care even if we accidentally trigger some invalid usage.

hrydgard added a commit that referenced this pull request Jul 23, 2020
@hrydgard
Copy link
Owner

@anr2me fix submitted to master.

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

Successfully merging this pull request may close these issues.

9 participants