Skip to content

Conversation

@rapha-tech
Copy link
Contributor

The PR aims to add support for simple OSDs on retro-go during game emulation

@ducalex
Copy link
Owner

ducalex commented Mar 17, 2025

Can you add back rg_display_get_width/height? For a long time I've tried to convince myself that doing rg_display_get_info()->screen.width is fine, but in reality it's just annoying... That's why I've recently added rg_display_get_width/height.

Inside rg_display.c you can access display.screen.width directly, that's no problem and you can leave those changes if you want.

Edit: I've also fixed the printf in snes on the dev branch, you can remove this unrelated change.

@rapha-tech
Copy link
Contributor Author

rapha-tech commented Mar 18, 2025

Okay,
Also I forgot to answer your older message : I'd like to make things the right way even if it means spending a lot of time. If it can improve the rendering system it will be worth!
You also made me realise that font shadow will be an absolute need even if it is a bit costly since it is usually unreadable.

On the other hand I'm wondering if it still a good Idea to add transparency? But I can't think of an other way to have my battery icon without a rectangle "behind" it.

Comment on lines 41 to 42
.width = rg_display_get_width(),
.height = rg_display_get_height(),
.width = rg_display_get_info()->screen.width,
.height = rg_display_get_info()->screen.height,
Copy link
Owner

Choose a reason for hiding this comment

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

Should be undone

Comment on lines 612 to 621
printf("Rom loaded: name: %s, id: %s, company: %s, size: %dKB\n", Memory.ROMName, Memory.ROMId, Memory.CompanyId, Memory.CalculatedSize / 1024);
Settings.ForceHeader = Settings.ForceHiROM = Settings.ForceLoROM = Settings.ForceInterleaved = Settings.ForceNoHeader = Settings.ForceNotInterleaved = Settings.ForceInterleaved2 = false;
Settings.ForceHeader = false;
Settings.ForceHiROM = false;
Settings.ForceLoROM = false;
Settings.ForceInterleaved = false;
Settings.ForceNoHeader = false;
Settings.ForceNotInterleaved = false;
Settings.ForceInterleaved2 = false;

printf("Rom loaded: name: %s, id: %s, company: %s, size: %dKB\n",
Memory.ROMName, Memory.ROMId, Memory.CompanyId, (int)(Memory.CalculatedSize / 1024));
Copy link
Owner

Choose a reason for hiding this comment

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

Should be undone.

Comment on lines +84 to +86
uint16_t *buffer = osd.surface->data; // Treat the buffer as 16-bit values
for (int i = 0; i < osd.surface->width * osd.surface->height; i++)
buffer[i] = C_TRANSPARENT; // Assign the C_TRANSPARENT color directly to the background
Copy link
Owner

Choose a reason for hiding this comment

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

I have finally implemented rg_surface_fill, you should now be able to do rg_surface_fill(osd.surface, NULL, C_TRANSPARENT);

Copy link
Owner

Choose a reason for hiding this comment

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

Although I'm not able to test my own code right now so maybe best don't waste your time...

@ducalex
Copy link
Owner

ducalex commented Mar 31, 2025

If we go ahead with transparency then I think we should reconsider how we handle things. Currently you rerender the background pixels which works fine but it bypasses the filtering system and it also makes the code more repetitive.

Bypassing the filtering for a few pixels in the case of a battery icon doesn't really matter, the pixels will be close enough. But it might cause visible weirdness if the OSD is bigger with more transparent zones.

One possibility I can think of is that in the for (int y = 0; y < draw_height;) { ... } loop we check if the lines we've just rendered intersect the OSD. if so, we draw the concerned OSD portion over it and set need_update to true.

The other option I can think of is to just ignore the filter problem. We keep your current code but I clean it up a bit to reuse more code and improve correctness.

On the other hand I'm wondering if it still a good Idea to add transparency? But I can't think of an other way to have my battery icon without a rectangle "behind" it.

If the goal is limited to drawing a battery icon then drawing the few rectangles of the icon directly to the screen with rg_display_clear_rect (or just manually setting pixels in the aforementioned draw loop) might be a lot simpler and more effective than building a flexible OSD system with transparency. I think it would be a reasonable option and I would accept such PR, because the low battery icon is probably the only OSD that will be needed by users anyway.

But if so, I suggest that we keep this current PR open for when one of us, or someone else, wants to finish the full-fledged OSD :) (or at least transfer your changes to a branch in my repo before you delete them on your side).

@ducalex ducalex force-pushed the dev branch 4 times, most recently from c72c92e to 3dbe19b Compare June 25, 2025 08:04
@rapha-tech
Copy link
Contributor Author

Hi, sorry for the really late reply! I haven't worked on retro-go for a long time but I'm getting back to it.

Anyway, about the OSD, first thanks for your review but I feel like it would be relatively hard for me to make good clean code for now, I'm relatively new to C programming and I don't want to waste your time so yes I think we should left this PR open for when the full implementation of the OSD will be necessary and someone will want to work on it.

I will create a new PR soon with just the battery icon!

@ducalex
Copy link
Owner

ducalex commented Aug 14, 2025

I've been experimenting with on-screen indicators: https://github.com/ducalex/retro-go/commits/on-screen-indicators/

In on_screen_indicators you can select a virtual LED that does what a real LED would do (except it's only updated when the game screen is updated...). Or just have a low battery icon.

With this current code the icon can only be drawn at the bottom (you can experiment, but you'll see there's too much flicker if moved elsewhere).

To have it on top we possibly could call on_screen_indicators from within write_update, right after we've drawn the top 10 lines or something. This will flicker somewhat, but might be tolerable.

Of course the real way of putting it anywhere we want without flicker is compositing the part that intersects with the game screen. I've also experimented with that but the added complexity doesn't seen worth it.

So let me know if you have thoughts about placement and style and blinking and all that!

@rapha-tech
Copy link
Contributor Author

rapha-tech commented Aug 31, 2025

Hey, sorry for the late answer!

First thanks for taking the time to do the implementation ;)

I tested the battery icon and your implementation is great, I also noticed the blinking, but I feel like it's not an issue worth the effort for an icon that is only meant to let people know the device is going to die soon.

I agree that the placement isn't the best since it doesn't correspond to the usual icon but it's a minor detail considering the added blinking when placed correctly.

So I'm happy with the current code!

Edit: if merged, maybe you should let the user choose wether they want this option or not in the settings (some might find it annoying)

@ducalex
Copy link
Owner

ducalex commented Aug 31, 2025

I've merged the battery icon on low power.

Best way to test it is to disconnect the battery or in rg_display.c replace rg_system_get_indicator(RG_INDICATOR_POWER_LOW) with true.

It was impossible to find colors that work well on all backgrounds, but making it flash seems to work around that by still drawing some attention. Let me know how you feel about the styling and behavior!

Edit: if merged, maybe you should let the user choose wether they want this option or not in the settings (some might find it annoying)

It only appears at 2% which is usually a few minutes left, so I think it's fair to say that the user would prefer to see it than lose state. But I agree giving control to the user is important and I intend to add a toggle!

ducalex added a commit that referenced this pull request Aug 31, 2025
#188)

As of now there is no way to disable it, but I will add a toggle later, likely in the LED menu (and rename the menu to Indicators or something)..
@rapha-tech
Copy link
Contributor Author

Yes, the blinking really helps it stand out from the game. Personally, I think the icon is great as it is!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants