I don't think I'll get through all the code, though I probably looked over all of it at some point. It does look noticeable cleaner now though!

I just realised (although it was always there) that Game now prints to the terminal every single time it's launched:

[S_API] SteamAPI_Init(): SteamAPI_IsSteamRunning() did not locate a running instance of Steam.
[S_API] SteamAPI_Init(): Loaded '/home/ralph/.local/share/Steam/linux32/steamclient.so' OK.
[S_API FAIL] SteamAPI_Init() failed; create pipe failed.

which is pretty annoying for non-Windows users like me who use the terminal, and we should try to find some way to fix this. Unfortunately I don't know if that's possible if steamworks must be initialized before the window is created -- but does it actually need to be? I think SteamAPI_RestartAppIfNecessary would avoid requiring it. Going to do some tests...
Even if it does need to be, we could figure out whether we're loading an .rpg immediately (from commandline or name of the executable) before initializing gfx backend.

I think you should use boolean as the translation of C/C++ bool in the translations of the API functions in steam.bas, especially when passing a pointer to dim bFailed as boolint. (My previous use of boolint was where I also used matching boolint in C, and was due to my misunderstanding of boolean. I should get rid of it).

I think maybe we technically should remove the comments in steam_internal.bi since they're probably copyrightable but I don't think the original C header was meant to be redistributed. But I know that public C# wrapper you linked just translation all the original headers and I don't think Valve care.

We have inconsistent indentation but I reserve the right to complain about indentation in new code regardless! Nowhere else do we indent code inside a namespace, and since it encompasses all of achievements.rbas I feel it's a bit pointless. Also, I get the impression you have your editor set to use 4-space tabs, but used spaces to indent rather than tabs. I think the rest of the codebase uses either tab, 1 space (which I oppose), or 2 space indentation, so it would be nice not to have a 4th way.

Should Steam.reward_achievement be called from Achievements.reward_achievement only for permanent achievements? Or is it intentional... do you get notifications from steam for those the first time you achieve them? Which if possible seems like a good thing to me. Also, suggest checking .steam_id is non-empty in reward_achievement before calling steam.

More tomorrow (maybe).

Whoops, I don't know what keycombo I pressed to submit a half-written comment. I edited it.

Wait, I'm not thinking straight. I forgot that aside from steamclient.so there's a second library, libsteam_api.so, which I had placed in the directory next to ohrrpgce-game, which is printing that. So users won't get those messages usually.

We may wish to add a Distribute Game option for Steam which adds the steam_api lib, and possibly the appid somehow too.

Steamworks initialization

I do believe we need to initialize Steamworks before we create the window, so it can hook into opengl/etc properly. Especially if we add the SteamAPI_RestartAppIfNecessary call, since otherwise the window would flash on the screen and then immediately vanish.

I wonder if we could supress the console messages somehow? Maybe by closing stdout?

I agree that we should add the libraries to the distribute game feeature. I've been thinking about how to do this properly (again, especialy for the SteamAPI_RestartAppIfNecessary call) but we can discuss that elsewhere.

<1/2/4 space indentation>

Agree on 1 space being bad. 4 spaces is pretty standard in all the code bases and languages that I usually work with, but I'm happy to switch it here (other than the labour of fixing it, but that's what scripting tools are for)

I would like to push back a little on this. Even in C++, it is common to indent within namespaces... right?

*googles*

oh my god it isn't. hm.

Well, either way, this is not C++ and not indenting namespaces reads wrong to me. It's not as though we are trying to save characters here. Also, there do exist things outside of namespaces in these files, which imo makes it more important.

If you insist, I will unindent, but let it be known that I don't like it!

<Steam.reward_achievement>

It should be called for all achievements; Steam will not display it multiple times. It should, however, check for the existence of a steam id before doing so!

<steam_internal.bi comments>

I think what you're really asking here is "how much of steam_internal.bi can be re-licensed by us?". IANAL, but my guess is that right now it is less than 100%. Though I used the steamworks SDK as reference, I wrote the types and function definitions by hand. I did not write the error constants by hand (which is why the constants came over).

Given that we do not use any of those constants at all for any reason, I'm happy to just remove that section. At that point, I think the rest is safe to do with as we need.

Unrelated to that, I did a bit more digging and I found this page which somehow we all missed at the start of this project:

https://partner.steamgames.com/doc/sdk/uploading/distributing_opensource

Keep in mind that according to the Steam Distribution Agreement you warrant and represent that you have all necessary rights to distribute the game via Steam. If your application contains third party open source code that is incompatible with the Steamworks SDK, then YOU MUST NOT DISTRIBUTE YOUR APPLICATION VIA STEAM.

Which Open Source Licenses are problematic for shipping on Steam?
Generally, any license that has a so-called “copyleft” element will be problematic when combining code with the Steamworks SDK. The best-known example is GPL.

(emphasis theirs)

I don't think there's anything on that page that is going to be a problem here. I think what they are saying is that if you are using the Steamworks headers (as we would be if this was a C or C++ project), those headers cannot be relicensed as GPL. However, since that's not what we're doing, it doesn't have any bearing on us or people using the OHR on Steam.

I tried initialising Steamworks after creating the window and it works normally, but the program crashes when you quit, inside the OpenGL driver. So I don't think we'll be doing that.

So, uh, sorry I wasn't clear, but I wasn't suggesting switching to two-space indents (not a fan personally). Because I believed you had your editor set to 4-space tabs I suggested switching to tabs as a minimal change which improves consistency. But really, you can use 4 spaces if you want, since each module uses its own code style (which is baffling). Likewise, indent inside the namespace if you have a strong preference.

I'm happy to have steam notifications for non-permanent achievements. But if the player previously received it in another game steam presumably won't display anything? If you select non-permanent achievements I think you expect the engine to always show a notification. Does that mean we should also show a slice-based notification in addition to the steam one? Or should we just remove the achievement and then immediately re-award it to force a notification?