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

Multiple instance support (broken out from #13132) #13169

Merged
merged 6 commits into from
Jul 20, 2020

Conversation

hrydgard
Copy link
Owner

I decided to attempt breaking apart #13132 , first up the multi-instance support. Just took the relevant-looking commits as-is. Might do some cleanup before merge.

After this is merged, #13132 should be rebased on top of master (which should be mostly trivial), then we'll extract the next part.

I'm not comfortable merging the whole thing in one chunk, it's too wide-ranging and hard to bisect errors, so this is what we'll do.

I don't actually really know how to test this, so some instructions would be nice :) @anr2me ?

@anr2me
Copy link
Collaborator

anr2me commented Jul 19, 2020

I decided to attempt breaking apart #13132 , first up the multi-instance support. Just took the relevant-looking commits as-is. Might do some cleanup before merge.

After this is merged, #13132 should be rebased on top of master (which should be mostly trivial), then we'll extract the next part.

I'm not comfortable merging the whole thing in one chunk, it's too wide-ranging and hard to bisect errors, so this is what we'll do.

I don't actually really know how to test this, so some instructions would be nice :) @anr2me ?

Awesome!
To test this you just need to put localhost on both Adhoc Server IP address and enable built-in adhoc server on at least one of the instance (enabling built-in adhoc server on both instance are okay too, doesn't seems to have any issue).
May be @mojojojodojo and friends could help tests this using the game list they tested with, to see if there are any regression when compared to current official release :)

Usually i tested it by running PPSSPP exe twice from the same folder so i only need to configure networks settings once (both built-in adhoc server ended being enabled). But if i do this sometimes there will be settings corruption, probably happened when i closed down multiple PPSSPP instance too fast and they got mixed up one another while trying to save the settings may be?
May be later we could have a separate settings based on the instance number? copied from existing setting if settings for current instance doesn't exist yet, so players doesn't need to reconfigure it from scratch.

Another issue with multiple instance from the same folder is that sometimes one of the window size and position is random when PPSSPP started (not sure if this also caused by settings corruption or not, may be i ran both instance too fast? ). But it seems to only happened with Debug builds, Release builds never had this issue as i remembered.

Btw, when testing multiplayer it's not recommended to load it from savestate, because it seems there are games that takes MAC address early during boot time and save that MAC somewhere in memory, so if you ever changed your MAC and load savestate the game will try to use the old MAC instead of current MAC and ended causing unknown peer/MAC issue during multiplayer, although i fixed it by tampering the local MAC given by the game during sceNet* calls in one of the game fixes, but it's not part of multiple instance.

PS: @hrydgard I think the commits saying "Fix local IP detection on non-Windows" also have something todo with multiple instance 45ed8cd

__ResetInitNetLib();
}

void __NetShutdown() {
__ResetInitNetLib();
//net::Shutdown();
#ifdef _WIN32
WSACleanup();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want this, or the WSAStartup? We already start networking on Windows, so in theory this is just doing nothing. At worst, it might make other parts of PPSSPP that use networking crash, if something gets out of sync. Doesn't seem like there's a good reason to have it.

-[Unknown]

Copy link
Collaborator

@anr2me anr2me Jul 20, 2020

Choose a reason for hiding this comment

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

Yes it should be safe to remove, actually it's no longer there on my final code. (probably removed within one of the commits, i forgot which one)

PS: The only issue i had with WSAStartup/Shutdown was with UPnP fallback cleanup (within PortManager class destructor) where i need to use another WSAStartup/Shutdown within that class, but it should never reach that fallback if UPnP is being cleaned up properly

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah I see it was removed in "Added UPnP support". I will do the same here, so hopefully the conflict will be none or very easy once you rebase.

@hrydgard
Copy link
Owner Author

The "Fix local IP detection on non-Windows" has a lot of conflicts if I try to apply it directly on top of this, there's been too many changes in between. We'll have to apply it later.

@hrydgard
Copy link
Owner Author

hrydgard commented Jul 20, 2020

Also later, I'm going to generalize this multi-instance stuff, so it starts counting directly when PPSSPP is started, and any secondary instances will not save their config. That should solve the settings corruption.

Could also show the instance number in the title bar.

@hrydgard
Copy link
Owner Author

OK, I got this to work with Tekken 6. Pretty cool!

I'm gonna go ahead and merge this, and do the cleanup (and generalization, so we can do things like turn off audio in the second instance) after we have merged more of your PR, to minimize conflicts.

@hrydgard hrydgard merged commit dadffa3 into master Jul 20, 2020
@hrydgard hrydgard deleted the multiple-instance-ANR2ME branch July 20, 2020 09:04
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.

None yet

3 participants