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

Merge upstream changes. #35

Closed
wants to merge 125 commits into from
Closed

Merge upstream changes. #35

wants to merge 125 commits into from

Conversation

orbea
Copy link

@orbea orbea commented Dec 11, 2017

This merge was not very smooth, there were no conflicts, but here are the issues I encountered. Please review the last two commits for my changes.

  1. gl_lost_manager was removed upstream and this required removing it from libretro.cpp too. As this is part of the previously broken context_reset code I opted to comment it out. When someone more capable is able of fixing this issue the comments can be removed. Currently its as broken as it was before...
    See hrydgard@7d0fc9c#diff-c186d1e5c210507a5c29e758cecef602 and https://github.com/libretro/ppsspp/issues/27
  2. Upstream changed some code around which required adding glslang and spirv-cross files to Makefile.common to fix many undefined references.
  3. After commit hrydgard@9f000dd the libretro core will just close the content if there is a prexisting shader cache. I found that removing the following line in GPU/GLES/GPU_GLES.cpp worked around it.
shaderManagerGL_->Load(shaderCachePath_);

But as that is not a solution I found it also worked if this line in libretro.cpp is removed.

environ_cb(RETRO_ENVIRONMENT_SHUTDOWN, nullptr);

Which is part of.

if(!PSP_Init(coreParam, &error_string))
{
	 if (log_cb)
		log_cb(RETRO_LOG_ERROR, "PSP_Init() failed: %s.\n", error_string.c_str());
	environ_cb(RETRO_ENVIRONMENT_SHUTDOWN, nullptr);
}

Given that I made changed it to.

if(!PSP_Init(coreParam, &error_string) && !GPU_IsReady())

This works, but I am not sure for the right reasons?

unknownbrackets and others added 30 commits December 2, 2017 11:55
If it has no sendMessage(), it probably still makes sense to send to the
parent (especially cpu change, clear cache, etc.)

This fixes the ability to change CPU cores ingame, and also fixes other
settings which weren't properly clearing caches.
Most of it can just be handled by the common parent classes.
In case someone doesn't realize they're on settings, or etc.
UI: Handle messages even when dialog is top
Fixes the Android part of hrydgard#10217.

Not sure if the line height calculation is ideal but it's visually fine.
Should only matter if vertex cache is enabled.
Before, if you went to the GPU backend prompt, and canceled, it would
never call the callback, and so the g_Config value never got reset.
Fix missing Maintain() for some hashmaps
UI: Send prompt results even on back/cancel
…hard limit on the total number of allocs and it's unnecessary to have the UI put pressure on that.
Use a VulkanDeviceAllocator for thin3d textures. Saves on allocations.
… Stats: Invent some sort of usage metric for device memory allocators.
In fact, it may even be wrong to show the new data icon in this case...

Also fixes crashes when save title is 128 characters long.

Should improve hrydgard#9632.
Small boost - 5.66x instead of 5x steps.
Since we scale by 0.5x for small window, we need to adjust the half pixel
offset to match real pixels.
This applies to all platforms, but falls back to a compat profile if
core is not available.
@unknownbrackets
Copy link

unknownbrackets commented Dec 11, 2017

You need to keep calling GPU_IsReady() until it returns true. Depending on how many shaders there are to precompile, it may require calling it for several seconds (because of how slow GPU drivers can be at compiling.)

This was done to allow rendering of loading messages and prevent ANRs while precompiling shaders.

-[Unknown]

@orbea
Copy link
Author

orbea commented Dec 11, 2017

Thanks for the advice, but with all due honesty I don't know what I'm doing... I could really use help from someone more capable.

@inactive123
Copy link

inactive123 commented Dec 11, 2017

@unknownbrackets @hrydgard is it perhaps an option that we receive some upstream support and the libretro port becomes an officially supported target? I think @orbea and others would be glad to see that happen since most of you are more familiar with the inner workings of your emulator than we are. A lot of the users would be glad to see this happen and I don't think it would take away from the base emulator, in fact the opposite. Ever since we managed to reach an equilibrium with byuu and Higan it has been nothing but a positive experience, the same could apply for PPSSPP.

If not, the alternative is having to constantly play catch up whenever some internal API changes get made again. I'd really hope we can find some kind of willingness here to give a potential collaboration/cooperation a shot. I'd be willing to help out too.

@orbea
Copy link
Author

orbea commented Dec 11, 2017

As far as the libretro code goes, I think it should all be in the cmake files, the libretro directory and the inclusion of glew_libretro.c.

I personally think the ppsspp core would make a great asset to upstream and would reduce a lot of complications in maintaining the core. That said there are a few glaring issues such as Tekken 6 not booting or https://github.com/libretro/ppsspp/issues/27 which are not absent in upstream, but they might actually get solved if someone more capable and knowledgeable about ppsspp could help...

On the other hand, the libretro core entirely avoids countless micro stutters in upstream which make ppsspp rather unplayable.

@orbea
Copy link
Author

orbea commented Dec 12, 2017

I've updated the last commit with another possible solution. I'm still not really sure about this, any advice would be appreciated!

diff --git a/libretro/libretro.cpp b/libretro/libretro.cpp
index b6780235e6..ac4b97850a 100644
--- a/libretro/libretro.cpp
+++ b/libretro/libretro.cpp
@@ -1194,11 +1194,17 @@ void retro_run(void)
 	      bool success = libretro_draw->CreatePresets();
 	      assert(success);
 
-	      if(!PSP_Init(coreParam, &error_string))
+	      bool bootPending_ = !PSP_Init(coreParam, &error_string);
+
+	      if(!bootPending_)
 	      {
-		      if (log_cb)
-			      log_cb(RETRO_LOG_ERROR, "PSP_Init() failed: %s.\n", error_string.c_str());
-		      environ_cb(RETRO_ENVIRONMENT_SHUTDOWN, nullptr);
+		      bool invalid_ = !PSP_IsInited();
+		      if (invalid_)
+		      {
+			      if (log_cb)
+				      log_cb(RETRO_LOG_ERROR, "PSP_Init() failed: %s.\n", error_string.c_str());
+			      environ_cb(RETRO_ENVIRONMENT_SHUTDOWN, nullptr);
+		      }
 	      }
 
 	      host->BootDone();

@unknownbrackets
Copy link

unknownbrackets commented Dec 12, 2017

Well, I think what I'd suggest is to set _initialized = to the result of PSP_IsInited(), and have an else that checks GPU_IsReady() && PSP_IsInited().

Basically, the flow is:

  • PSP_InitStart() - if true, continue, if false, bail (false = failure case.)
  • Loop (possibly rendering a loading screen):
    • PSP_IsIniting() - if true, PSP not inited yet. Don't try to run emulation.
    • PSP_InitUpdate() - if true, init completed (may have failed or succeeded.)
    • PSP_IsInited() - if true, ready to emulate. If false, and PSP_InitUpdate() was true (or PSP_IsIniting() is now false), bail = failure case.
  • PSP_RunLoopFor() - safe to call after init completes.

This API hasn't really changed in 4 years (and wasn't changed by that PR/commit) and can be seen used here:
https://github.com/hrydgard/ppsspp/blob/46ea88e0962ec6dd19e96b3e078718cc87b20fe4/UI/EmuScreen.cpp#L135-L149

Personally, I haven't got a lot of personal interest in RetroArch. Last time I tried to use it:

  • It was very non obvious how to get PS1 emulation working, with limited error messaging telling me what I was doing wrong, so I lost interest after trying for around 15 minutes.
  • The PPSSPP core seemed to strip away various settings I cared about.
  • The UI wasn't my cup of tea, but I think it's changed drastically since then.
  • It seemed to suffer from the "too many options" problem. If I'm going to bother to research what core I want, I'm just going to download that emulator. RetroArch is only wasting my time by providing me 47 SNES emulation options.
  • Nothing seemed to add to or improve my experience versus using other emulators. It just seemed harder to get running with some shoe-horned settings.

Don't take this as criticism - I'm probably exactly the wrong target audience for RetroArch.

As a rule, I'm not interested in maintaining or developing something in my free time that I'm not actually interested in using. I'm happy to help with issues, though. That's just me.

-[Unknown]

@inactive123
Copy link

inactive123 commented Dec 12, 2017

This is not about RetroArch though, this is about every libretro frontend that is capable of running this core. RetroArch is called a reference frontend for libretro for a reason. So having the libretro core in upstream and maintained by upstream would ensure that the port is in line with whatever options and features the standalone build has. There are some highprofile programs getting a libretro frontend in mainline soon. I would encourage you to see libretro as a platform, as an alternative distribution method of your emulator, etc. It is not just about getting it to work in RetroArch.

All of the potential issues you cite could be fixed through teamwork and cooperation. If you find the core to be missing some options, that is something we could fix together, etc.

Anyway, this should be about what the users want. A lot of your users want your emulator to run inside RetroArch or any other libretro frontend, so why not try to meet the demand and have more direct control over this port? Why should it be treated any differently from say the Windows or Linux or Mac version?

@inactive123
Copy link

inactive123 commented Dec 12, 2017

I invite @hrydgard to this thread too -

I'd argue that if we worked together, stuff like this wouldn't happen all the time where they can walk over each and every single one of us in the emulation scene individually -

https://www.reddit.com/r/emulation/comments/7ivii4/damonps2_pro_fastest_ps2_emulator_for_mobile_scam/

https://forums.ppsspp.org/showthread.php?tid=23757&pid=129336#pid129336

I have also had my fair share of experiences like that in the past with RetroArch and entrepreneurs, and each and every single time we were on our own, with barely anybody trying to help. This is a vicious cycle where it's now happening to other developers in this emulation scene, because the desire to make money at all costs and not even be bound to the most trivial of rules tied to that source is just too strong for some of these more insidious elements. How good are our open source licenses if this stuff can keep happening without consequence? Power comes through mass organization, not through the barrel of a gun. None of us have guns here - the only perceivable power we COULD have is through organizing. Yet for some reason, nobody in this scene wants to do that. Well, don't be surprised if this stuff keeps happening then and you are powerless to stop it individually. Because nobody has each other's backs, everybody eventually gets screwed over by somebody else. This is why they have trade unions to begin with in other industries, because no man is an island, and no single man can deal with all the problems and nefarious elements that happen in the real world.

This only can happen because they know there are no consequences and because they know that they can easily pick each and every single one of us against each other. This opensource emulation scene is weak and ineffectual when it comes to dealing with these kind of situations in its current state. If we worked together, did collaborations, etc., there'd be more of a collective bloc and it wouldn't be as easy for them to get away with these things. This whole 'one man is his island' mentality is the undoing of this entire scene I'm afraid. It cannot work like this and you are already seeing these guys showing you it cannot work like this. If we want opensource to remain something fair where it's a win-win situation and the people using it conform to the basic wishes and terms of the license, we will have to fight for it and stand up for one another. It cannot work like little isolated silos that all just want to work on what they like and not have each other's backs - that is bound for failure. Each and every single big company has the common smarts about them to collaborate and work together even if it means expanding a little effort. It's about seeing the bigger picture and recognizing that it makes sense to forge alliances and relationships that could then be used later down the line.

Recognize that my intentions are sincere here. I am not ranting to anybody, this is an impassioned plea to want to explore ways we could find some common ground. I want to collaborate, I want to work together, and I think through this we can eventually create a more fair and inclusive scene this way. There is no reason why we have to all remain isolated from each other. It is helping none of us right now in the long run. And if a libretro core that is being maintained by upstream and is treated like a general release would be one way to forge an alliance, then why not? What is there to lose? There are still bugs remaining in this PPSSPP port that we cannot fix ourselves, we need your help here and for you to show the way. The users that want to use your core from within the confines of RetroArch and/or any other libretro frontend are also YOUR users, they are not ours. Please try to separate RetroArch from the libretro project and try to see the bigger picture here. I am hoping you guys will eventually come to see the benefit and value in what I am proposing.

@orbea
Copy link
Author

orbea commented Dec 12, 2017

@unknownbrackets By all means I really appreciate the advice given, but you have to consider that I have minimal c/c++ experience and understanding at best and I'm not really sure how to implement your suggestion yet... Do you think this is better?

diff --git a/libretro/libretro.cpp b/libretro/libretro.cpp
index b6780235e..5239098dc 100644
--- a/libretro/libretro.cpp
+++ b/libretro/libretro.cpp
@@ -1008,7 +1008,7 @@ bool retro_load_game(const struct retro_game_info *game)
    g_Config.iGlobalVolume = VOLUME_MAX - 1;
    g_Config.bEnableSound  = true;
    g_Config.bAudioResampler = false;
-   _initialized = false;
+   _initialized = PSP_IsInited();
    check_variables();
 
 #if 0
@@ -1170,12 +1170,10 @@ void retro_run(void)
                   }
           }
    }
-  
 
    if (should_reset)
       PSP_Shutdown();
 
-   
    if (!_initialized || should_reset)
    {
       should_reset = false;
@@ -1194,11 +1192,16 @@ void retro_run(void)
              bool success = libretro_draw->CreatePresets();
              assert(success);
 
-             if(!PSP_Init(coreParam, &error_string))
+             bool bootPending_ = !PSP_Init(coreParam, &error_string);
+
+             if(!bootPending_)
              {
-                     if (log_cb)
-                             log_cb(RETRO_LOG_ERROR, "PSP_Init() failed: %s.\n", error_string.c_str());
-                     environ_cb(RETRO_ENVIRONMENT_SHUTDOWN, nullptr);
+                     if (!GPU_IsReady() || !PSP_IsInited())
+                     {
+                             if (log_cb)
+                                     log_cb(RETRO_LOG_ERROR, "PSP_Init() failed: %s.\n", error_string.c_str());
+                             environ_cb(RETRO_ENVIRONMENT_SHUTDOWN, nullptr);
+                     }
              }
 
              host->BootDone();

The problem I am experiencing is neither GPU_IsReady() or PSP_IsInited() are true till after _bootpending so I'm not sure how to make it an else here? My primary concern right now is not seeing the ppsssp core fall behind upstream like last time making it impossible to update.

I'm not really here to debate who should or should not help maintain the libretro core, but I just want it to be clear that your concern about the ppsspp core missing options is valid, but also unfortunately difficult to fix when no one that knows what they are doing is willing to get their hands dirty. :) As for RetroArch, it should not be viewed as anything more than a reference frontend to the libretro api and while I can see how its aimed at power users, these concerns may have been improved since you last tried it and I doubt anyone would be opposed to cooperating with you or anyone else willing to point them out. Of course libretro means more than RetroArch, gnome games and kodi are two other examples.

@orbea
Copy link
Author

orbea commented Jan 23, 2018

@orbea orbea closed this Jan 23, 2018
@orbea orbea deleted the merge2 branch January 23, 2018 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants