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

Osd manager #1431

Merged
merged 5 commits into from Jan 3, 2017

Conversation

Projects
None yet
@JasonBrownDeveloper
Contributor

JasonBrownDeveloper commented Jun 23, 2016

This is no where near ready to merge, but I wanted to get eyes on what I have. Most of the base functionality is done. I'm still going to add some gettext support and add a settings page.

@@ -25,6 +25,7 @@
#include "GSTexture.h"
#include "GSVertex.h"
#include "GSAlignedClass.h"
#include "GSOsdManager.h"
enum ShaderConvert {

This comment has been minimized.

@gregory38

gregory38 Jun 23, 2016

Contributor

Could you extand this enum and uses it instead of 18/19 value for the program object

@gregory38

View changes

plugins/GSdx/GSDevice.cpp Outdated
@@ -118,6 +144,19 @@ void GSDevice::Present(const GSVector4i& r, int shader)
ShaderConvert_COMPLEX_FILTER}; // FIXME
Present(m_current, m_backbuffer, GSVector4(r), s_shader[shader]);
static std::chrono::system_clock::time_point timer;

This comment has been minimized.

@gregory38

gregory38 Jun 23, 2016

Contributor

indentation please.

This comment has been minimized.

@gregory38

gregory38 Jun 23, 2016

Contributor

And maybe you could put an ifdef ENABLE_OSD_TEST (here and above). Then add the define in config.h

This comment has been minimized.

@JasonBrownDeveloper

JasonBrownDeveloper Jun 23, 2016

Contributor

This is just some test code to exercise the new code and will be removed in the final pull request. I intentionally didn't indent those two lines to call out the fact that they are static and will not re-initialize between function calls (i.e. global variables).

@gregory38

View changes

plugins/GSdx/GSDeviceOGL.cpp Outdated
{
BeginScene();
m_shader->BindPipeline(m_convert.ps[18]);

This comment has been minimized.

@gregory38

gregory38 Jun 23, 2016

Contributor

As said previously, use a an enum constant here

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 23, 2016

Hum, I think you might need update cmake to add the libfreetype library. You need to add the dep in travis.sh and debian-packager/control. For windows it is slightly more complicated, so it can wait (but in short, freetype must be populated in 3rdparty)

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jun 23, 2016

From a comment on your original gsdx-osd-ogl branch:

"Note 1: based on freetype library (not a real dependency because wx already pull it)"

I believe wx requires freetype, but I can add it as an explicit dependency if you like.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 23, 2016

Oh. It misses the link option at least.

/usr/bin/ld: CMakeFiles/pcsx2_GSReplayLoader.dir/GSOsdManager.cpp.o: undefined reference to symbol 'FT_New_Face'

//usr/lib/i386-linux-gnu/libfreetype.so.6: error adding symbols: DSO missing from command line

collect2: error: ld returned 1 exit status

make[2]: *** [plugins/GSdx/pcsx2_GSReplayLoader] Error 1

make[1]: *** [plugins/GSdx/CMakeFiles/pcsx2_GSReplayLoader.dir/all] Error 2
@ramapcsx2

This comment has been minimized.

Member

ramapcsx2 commented Jun 23, 2016

Could you show a screenshot of this? I'd like to see what we're dealing with.
It'd be great to have an OSD :)

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jun 23, 2016

I'll see about putting a video up somewhere this evening. It's more of a dynamic thing a screenshot wouldn't capture.

EDIT: Video link https://drive.google.com/file/d/0B5GOz6BWltXBYWtIeWJ5Ml85VkU/view?usp=sharing

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jun 23, 2016

I didn't have any problems linking with the replay loader
`[ 95%] Building CXX object plugins/GSdx/CMakeFiles/pcsx2_GSReplayLoader.dir/GSOsdManager.cpp.o

...

Linking CXX executable pcsx2_GSReplayLoader

[100%] Built target pcsx2_GSReplayLoader`

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 24, 2016

It is not portable to link through another lib. It won't always work. It depends on linker and options.

@ramapcsx2

This comment has been minimized.

Member

ramapcsx2 commented Jun 24, 2016

Thanks for the video. This is indeed good stuff! :)

@ssakash

View changes

plugins/GSdx/GS.cpp Outdated
EXPORT_C_(void) GSosdLog(const char *utf8)
{
if(s_gs) if(s_gs->m_dev) s_gs->m_dev->m_osd.Log(utf8);
}

This comment has been minimized.

@ssakash

ssakash Jun 29, 2016

Member

if(s_gs && s_gs->m_dev) ?

This comment has been minimized.

@JasonBrownDeveloper

JasonBrownDeveloper Jun 29, 2016

Contributor

yes thats what i meant, i've been distracted while doing this and have to keep taking breaks and coming back. I'll fix it in the next push.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jun 29, 2016

Honestly I'm not so sure that it worth to have an UTF/translation-compatible OSD. I mean 99% of people want the fps and load/save state feedback. Remaining usage will be debug stuff.
I'm afraid it will only bring extra bug/complexity due to UTF8 (linux)/UTF16(windows) difference.

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jun 29, 2016

It's not just for the translation part. I need to be able to look up the codepoint in the font reliably and wchar_t is different depending on the system and compiler. On linux it will usually be a 32 bit codepoint and on windows it will usually be 16 bits which may or may not be UTF. I need to boil them both down to something common, and wxString has a utf8_str() member.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jul 1, 2016

Yes, that why basic C string will be easier (and enough). There is a macro to extract the C string from wxString (close of c_str() )

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jul 1, 2016

Refer to http://docs.wxwidgets.org/trunk/classwx_string.html

ToAscii, c_str and mb_str are all potentially destructive if the string can't be converted to a single byte per character. utf8_str is always safe.

@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jul 2, 2016

It would be annoying if you want to display non-ASCII character. I'm not against it, I'm just asking if it worths it to support non-ASCII character in the OSD. Or if plain-english will be enough. In the latter case, you don't need to bother with UTF/wchar mess. By the way, so far it is really a good job, I would like to merge it as soon as possible. What are the current change on the pipe ?

Honestly I don't know what people want to print on the screen. Let's ask other opinions.

@Sarania @ramapcsx2 @turtleli @ssakash @refractionpcsx2
Feel free to give your opinion,

  • Plain english OSD
  • Special Char OSD (allow translation)
@ssakash

This comment has been minimized.

Member

ssakash commented Jul 2, 2016

The ideal choice would be to support non-ASCII characters but personally I think it would compromise the code readability and also add extra work for maintenance. so my vote is for plain English OSD. ;)

@ramapcsx2

This comment has been minimized.

Member

ramapcsx2 commented Jul 2, 2016

Plain English is preferred. The OSD would be used for status messages that are universal for emulators. Translation wouldn't be necessary for those.

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jul 5, 2016

I'm currently working on a settings page for the OSD. I also need to export the Monitor feature back to the main app. I need to figure out how to support the indicator feature. For the demo I just used font glyphs, but I would like to have a nice API to use a passed in glyph. I also want to do a cleanup pass of naming and style once I'm completely done.

As far as the ASCII vs Unicode support. I'm going to add a a wrapper similar to pxGetTranslation and a macro _u8 so that it will be comparable to other gettext and c++ string literal usage.

@willkuer

This comment has been minimized.

Contributor

willkuer commented Jul 6, 2016

It could be awkward for hotkeys if the strings are not translatable. Messages for saving savestates or fps counter probably don't need to be translated. But hotkeys that switch settings explicitly named in the configuration can make problems.

E.g. if I press F4 the OSD should probably say something like 'Disable framelimiting' in the German localization however it is called 'Deaktiviere FPS Limitierung' (I would like to emphasize that frame!=FPS) . 'Aspect ratio' becomes 'Seitenverhältnis'.

Probably it's not that bad with English/German as both languages belong to the same family but I guess for some languages there will be a large deviation between English setting string and localized setting string.

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Jul 6, 2016

Has there been a discussion yet about what should be on the OSD and what it should look like?

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jul 6, 2016

No, for the final PR, I was just going to log statesave/load and put the FPS counter on the monitor. But the interface should be super easy to drop anywhere in main app you want to log something.

@gregory38

View changes

plugins/GSdx/GSOsdManager.cpp Outdated
uint8 alpha = 0xFF;
if(!pair.second.on) {
alpha /= 4;
if(theApp.GetConfigB("indicator_enabled")) {

This comment has been minimized.

@gregory38

gregory38 Jul 11, 2016

Contributor

FYI, GetConfig call are quite slow, it is often to cache them in a local/member variables

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jul 12, 2016

I need some help with the windows and mac settings page, I don't have a windows or mac development machine. Also, I think a couple more commits and I should be done.

@Sarania

This comment has been minimized.

Contributor

Sarania commented Jul 13, 2016

I'm really glad to see this getting done! We've needed it for a long time.

Just throwing this out here as it's a random thing I thought of. As far as things that people would want displayed, there are the normal things like FPS, speed, save/load stating, changes caused by hotkeys etc. But I also would request being notified when PCSX2 writes out to the memory card. A lot of other emulators post an on screen message when the emulated system writes out a save and I find it "comforting" just to know it did in fact save :P

@ssakash

This comment has been minimized.

Member

ssakash commented Jul 18, 2016

need some help with the windows and mac settings page,

GUI elements for Windows/Mac could probably be added later, there's no hurry in adding it along with this PR.

Unless I'm wrong it could still be customized through the INI file. FWIW, do even need a settings page for OSD configuration ? I don't think any other emulators have anything like that.

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Dec 15, 2016

The freezing problem is not related to the OSD. I can reproduce it in the master branch. I can cause a freeze by pressing to quickly or holding down one of th Fn keys. F4 frame limiting seems to work particularly well for me.

@Sarania

This comment has been minimized.

Contributor

Sarania commented Dec 15, 2016

The freezing problem is not related to the OSD. I can reproduce it in the master branch. I can cause a freeze by pressing to quickly or holding down one of th Fn keys. F4 frame limiting seems to work particularly well for me.

It's likely the same issue that I reported a LONG way back(I could specifically cause it by rapidly toggling software mode with F9). Also more recently issues with some of the hotkeys were reported by I think @FlatOutPS2 who noted that the behavior of TAB/F4 can get screwy depending on the order they are pressed in.

Also thanks for all the hard work on this guys, especially @JasonBrownDeveloper

@JasonBrownDeveloper JasonBrownDeveloper force-pushed the JasonBrownDeveloper:OsdManager branch Dec 16, 2016

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Dec 16, 2016

I think the artifacting was caused by the OpenGL buffer being flushed past the used part. I changed the logic around a bit so that I calculate the log size exactly and so alway reserve the exact right amount. I also split the frame skipping log across two lines and fixed a few other warnings and minor details.

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Dec 16, 2016

The flickering issue seems completely gone 👍 , but the console message garbage I mentioned is still there.

@JasonBrownDeveloper JasonBrownDeveloper force-pushed the JasonBrownDeveloper:OsdManager branch Dec 16, 2016

@turtleli

This comment has been minimized.

Member

turtleli commented Dec 17, 2016

Console message garbage on Windows has been fixed.

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Dec 17, 2016

Console message garbage on Windows has been fixed.

Did you test Linux behaviour as well(including the GUI features)?

@turtleli

This comment has been minimized.

Member

turtleli commented Dec 17, 2016

Did you test Linux behaviour as well (including the GUI features)?

Nope, not yet.

@turtleli

Brief Linux test done. Seems to work ok. Transparency works if the typo is fixed.

plugins/GSdx/GSOsdManager.cpp Outdated
m_log_speed = m_log_speed < 2 ? 2 : m_log_speed > 10 ? 10 : m_log_speed;
m_monitor_enabled = theApp.GetConfigB("osd_monitor_enabled");
m_indicator_enabled = theApp.GetConfigB("osd_indicator_enabled");
m_osd_transparency = theApp.GetConfigI("osd_osd_transparency");

This comment has been minimized.

@turtleli

turtleli Dec 17, 2016

Member

osd_osd_transparency -> osd_transparency

plugins/GSdx/GSOsdManager.h Outdated
float StringSize(const std::u32string msg);
bool m_log_enabled;
int m_log_speed;

This comment has been minimized.

@turtleli

turtleli Dec 17, 2016

Member

File is indented with tabs, but this line is indented with spaces.

plugins/GSdx/GSOsdManager.cpp Outdated
FT_Error error = FT_New_Face(m_library, theApp.GetConfigS("osd_fontname").c_str(), 0, &m_face);
if (error == FT_Err_Unknown_File_Format) {
fprintf(stderr, "Failed to init the freetype face: unknwon format\n");
m_face = NULL;

This comment has been minimized.

@turtleli

turtleli Dec 17, 2016

Member

File is indented with tabs, but this line is indented with spaces. There's a few more instances of this happening in this file.

pcsx2/gui/ConsoleLogger.cpp Outdated
GSosdLog(str.c_str(), wxGetApp().GetProgramLog()->GetRGBA(color));
if (console)
Console.WriteLn(color, str.c_str());

This comment has been minimized.

@turtleli

turtleli Dec 17, 2016

Member

Indentation needs fixed.

@JasonBrownDeveloper JasonBrownDeveloper force-pushed the JasonBrownDeveloper:OsdManager branch Dec 18, 2016

@ssakash

Just some minor nitpicks related to coding style, would be nice if most of the redundant braces are removed though.

@@ -173,26 +174,26 @@ namespace Implementations
void GSwindow_CycleAspectRatio()
{
AspectRatioType& art = g_Conf->GSWindow.AspectRatio;
wxString arts(L"Not modified");
const char *arts = "Not modified";

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

Didn't we have a discussion earlier that the asterisk should be placed near the data type instead of the variable name?

This comment has been minimized.

@turtleli

turtleli Dec 22, 2016

Member

The conclusion was next to the variable name (clang-format is also set up to do so, though we currently haven't formatted pcsx2 and GSdx with it).

pcsx2/gui/GlobalCommands.cpp Outdated
OSDlog( Color_StrongRed, true, "(FrameSkipping) Enabled." );
OSDlog( Color_StrongRed, true, " FrameDraws=%d, FrameSkips=%d", g_Conf->EmuOptions.GS.FramesToDraw, g_Conf->EmuOptions.GS.FramesToSkip );
} else
OSDlog( Color_StrongRed, true, "(FrameSkipping) Disabled." );

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

else should also have braces here since the previous if() has it.

pcsx2/gui/GlobalCommands.cpp Outdated
OSDlog( Color_StrongBlue, true, "(GSwindow) Zoom: 0 (auto, no black bars)");
} else {
OSDlog( Color_StrongBlue, true, "(GSwindow) Zoom: %f", zoom);
}

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

Not sure why you added braces here when they're not necessary? doesn't also make the code more readable in my opinion.

This comment has been minimized.

@JasonBrownDeveloper

JasonBrownDeveloper Dec 23, 2016

Contributor

I added them when OSD and Console logging were two separate API calls. I can remove them now, since it's one call again.

pcsx2/gui/MemoryCardFile.cpp Outdated
int status = mcfp.Write( m_currentdata.GetPtr(), size );
if( 0 != status ) {

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

Maybe if(status) instead? having a constant before the variable on relational operators doesn't really look nice. We had similar discussions before.

This comment has been minimized.

@JasonBrownDeveloper

JasonBrownDeveloper Dec 23, 2016

Contributor

I don't mind either way. It's just a defensive programming style to protect against accidentally using an assignment instead of a comparison. The compiler will throw an error on (const = var) because you can't assign a value to a const but it will happily accept (var = const), because sometimes you really want to do that in more complex statements like (var1 == (var2 = func())

EXPORT_C_(void) GSosdMonitor(const char *key, const char *value, uint32 color)
{
if(s_gs && s_gs->m_dev) s_gs->m_dev->m_osd.Monitor(key, value, color);

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

Would be better to separate this into two lines, your wish though.

plugins/GSdx/GSOsdManager.cpp Outdated
void GSOsdManager::LoadFont() {
FT_Error error = FT_New_Face(m_library, theApp.GetConfigS("osd_fontname").c_str(), 0, &m_face);
if (error == FT_Err_Unknown_File_Format) {
fprintf(stderr, "Failed to init the freetype face: unknwon format\n");

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

unknown is misspelled here.

plugins/GSdx/GSOsdManager.cpp Outdated
m_face = NULL;
fprintf(stderr, "Failed to init the freetype face\n");
return;
}

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

I think something like this is more readable, any opinion?

if(error)
{
     m_face = NULL;
     if (error == FT_Err_Unknown_File_Format)
          fprintf(stderr, "Failed to init the freetype face: unknown format\n");
     else
          fprintf(stderr, "Failed to init the freetype face\n");

     return;
}
}
}
if(size) *utf32 = *utf8; // Copy NUL char

This comment has been minimized.

@ssakash

ssakash Dec 22, 2016

Member

I assume you meant NULL?

This comment has been minimized.

@JasonBrownDeveloper

JasonBrownDeveloper Dec 23, 2016

Contributor

When talking about characters it is NUL https://en.wikipedia.org/wiki/Null_character, when talking about pointers it is NULL.

JasonBrownDeveloper added some commits Sep 18, 2016

Changed the GSBufferOGL interface from map and upload to map and unma…
…p. This allows rendering directly into the OGL buffer instead of having to do copy at some point.
Added callbacks for OSD Log and Monitor. Added wrappers in PCSX2 main…
… for callbacks. Added some basic info calls (e.g. Saving loading FPS)

@JasonBrownDeveloper JasonBrownDeveloper force-pushed the JasonBrownDeveloper:OsdManager branch to 961cc28 Dec 23, 2016

@gregory38

Could you put this commit before the previous one ? (you just need to invert the 2 commit lines in the interactive rebase)
Previous commit seem to use the GetRGBA so it won't compile.

@turtleli

This comment has been minimized.

Member

turtleli commented Jan 2, 2017

Could you put this commit before the previous one ? (you just need to invert the 2 commit lines in the interactive rebase)
Previous commit seem to use the GetRGBA so it won't compile.

The commits seem to be in the correct order - Github is displaying it wrong.

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Jan 2, 2017

So what's left that needs to be done before this can get merged?

@gregory38 gregory38 merged commit fce2814 into PCSX2:master Jan 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gregory38

This comment has been minimized.

Contributor

gregory38 commented Jan 3, 2017

Thanks you for your contribution. Sorry that it took so long to review and to merge your work.

@JasonBrownDeveloper

This comment has been minimized.

Contributor

JasonBrownDeveloper commented Jan 3, 2017

Not a problem. I understand being meticulous when managing a large project with multiple contributors. I'm just happy I can contribute.

@lightningterror

This comment has been minimized.

Member

lightningterror commented Jan 3, 2017

Question. How to activate this feature on the latest build since it's merged now ?

@FlatOutPS2

This comment has been minimized.

Member

FlatOutPS2 commented Jan 3, 2017

Question. How to activate this feature on the latest build since it's merged now ?

Use the OpenGL renderer and make a savesate. :p

@lightningterror

This comment has been minimized.

Member

lightningterror commented Jan 3, 2017

Ah so it doesn't work with dx. I tried earlier on dx only so I thought I needed to edit something to make it work.

@ramapcsx2

This comment has been minimized.

Member

ramapcsx2 commented Jan 6, 2017

Very useful feature. Many thanks!

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