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

BridgeApi - RemixApi wrapper for x86 games #12

Closed

Conversation

xoxor4d
Copy link
Contributor

@xoxor4d xoxor4d commented Jul 14, 2024

This PR adds a intermediary api called BridgeApi to the bridge that allows x86 games to use parts of the x64 remix-runtime api.

Overview:

  • the client implements BridgeApi and sends the data of received api calls to the server client/bridge_api.cpp
  • the server assembles the necessary RemixApi structs and calls the appropriate RemixApi functions server/main.cpp

Behavior on init:

  • server/module_processing.cpp grab the device pointer used to initialize the RemixApi in CreateDevice / CreateDeviceEx
  • server/main.cpp initialize the RemixApi in InitializeD3D if client option getEnableBridgeApi() is true by calling remixapi_lib_loadRemixDllAndInitialize with the optained device pointer

Usage:

  • Using BridgeApi requires the user to be internal (dll/asi injection or by other means - requires game process access)
  • Client option client.exposeRemixApi true
  • Include bridge_c.h and a predefined macro:
#define BRIDGE_API_IMPORT
#include "bridge_c.h"

  • Initialize the api:
// make sure that the bridge had enough time to create a device before you call the following
const auto status = bridgeapi_initialize(&bridge);
if (status == BRIDGEAPI_ERROR_CODE_SUCCESS)
{
  if (bridge.initialized)
  {
    bridge.RegisterDevice();
  }
}

  • If no game render logic is hooked (triggered once per frame) or if the dll/asi has to be game agnostic, a callback can be registered that triggers when device->EndScene is called:
void on_endscene()
{
  bridge.DrawLightInstance(my_light_handle);
}

// somewhere after the api was initialized
bridge.RegisterEndSceneCallback(on_endscene);

Notes:

  • I've set client option client.exposeRemixApi to true by default (now false by default)

  • I'm using a mutex to get the device here. (I don't have much experience working with threads -> not sure if correct)

  • I've directly included remix_c.h from dxvk-remix (should be made a dependency that's pulled?)

  • There is no RemixApi version check nor does bridge_c.h expose any version

  • Sending const char* or wchar_t* via rpc can result in the string no longer having the null terminator in the correct location. This can lead to strings with "junk data" attached to them. The returned length/size is correct tho. This also happens for Bridge_DebugMessage. This leads to special handling in every api case in server/main.cpp that`s using strings:

    • Api_DebugPrint using std::string(data, length) to construct a new string with the proper length
    • Api_CreateOpaqueMaterial using NVPULL_PATH -> std::wstring(data, length) to construct a new wstring with the proper length

Example usage of almost every feature:

Copy link
Collaborator

@nv-nfreybler nv-nfreybler left a comment

Choose a reason for hiding this comment

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

Overall looks great, thanks for doing this! There is certainly a promising future for exposing RemixApi like this.

One overall comment: Please rename everything from BridgeApi to RemixApi and similar. We see this more so as exposing the RemixApi directly to the games/mods, as opposed to wrapping it through a layer, and we don't want to conflate the RemixApi with the bridge's actual API, which is basically just the d3d9 API.

src/client/bridge_api.h Outdated Show resolved Hide resolved
src/server/main.cpp Outdated Show resolved Hide resolved
bridge.conf Outdated Show resolved Hide resolved
src/server/module_processing.cpp Outdated Show resolved Hide resolved
src/server/main.cpp Outdated Show resolved Hide resolved
src/client/bridge_api.h Outdated Show resolved Hide resolved
src/server/module_processing.cpp Outdated Show resolved Hide resolved
src/server/bridge_api.h Outdated Show resolved Hide resolved
src/server/main.cpp Outdated Show resolved Hide resolved
src/client/client_options.h Outdated Show resolved Hide resolved
review fixes
@xoxor4d xoxor4d force-pushed the feature/bridge-api-squashed branch from a874b5d to e6c64ef Compare July 16, 2024 19:46
@xoxor4d
Copy link
Contributor Author

xoxor4d commented Jul 16, 2024

@nv-nfreybler Firstly, thanks for taking the time to review this! I've made the required changes and squashed everything into one commit. I've remove almost all traces of bridgeApi as requested with the exception to the bridge_c header file as I'm not sure if the contents will stay in a separate file or if they'll be included within the remix_c header?

@nv-nfreybler
Copy link
Collaborator

nv-nfreybler commented Jul 24, 2024

Thanks for making all these changes! A couple things to note:

  1. Part of the reason for my delay in getting back to you is because we've been implementing a versioning handshake.
  2. When we're ready, the next step of the process will involve taking in your changes so we can do some internal testing, and make further additions. Please be patient while we deliberate on this PR, in addition to our internal set of tasks.

I've remove almost all traces of bridgeApi as requested with the exception to the bridge_c header file as I'm not sure if the contents will stay in a separate file or if they'll be included within the remix_c header?

Thanks for doing that, I know it can be a long and tedious task to rename things. I appreciate your calling it out in particular, as we are actively discussing the bridge_c header file in particular. A decision will likely be reached in our own internal merge commit.

@nv-nfreybler
Copy link
Collaborator

Just wanted to post an update here that we're currently actively internally reviewing and working on this PR, for the better part of the last couple weeks, to be honest. We appreciate your patience, and want to assure you that it is not forgotten.

@xoxor4d
Copy link
Contributor Author

xoxor4d commented Aug 31, 2024

Thank you for the update, really appreciate it! I hope this hasn't caused too much trouble internally 🙂

svc-remix-github pushed a commit that referenced this pull request Sep 18, 2024
- #12

- Includes Remix API headers under new ext/remix/ top-level dir
  - remix_c.h and remix.h
  - Added an #ifndef to be able to turn off non-x64 compiler error at top of remix_c.h
- Add new root dir public/include/ for bridge-specific Remix API header
  - public/include/remixapi/bridge_remix_api.h
- Adds new util Serializable<T> helper class in util_serializable.h
  - Defines a uniform interface of functions to de-/serialize generic classes
- Adds new RemixApi util helpers in util_remixapi.h/.cpp
  - Defines Serializable<T>s for remixapi types, and adds extra helpers to handle their quirks
- Adds remix_api.h/.cpp to both Client and Server
  - Helpers for handling the x86 -> x64 conversion
svc-remix-github pushed a commit that referenced this pull request Sep 18, 2024
- #12

- [REMIX-3433]
- Includes Remix API headers under new `ext/remix/` top-level dir
  - `remix_c.h` and `remix.h`
  - Added an #ifndef to be able to turn off non-x64 compiler error at top of `remix_c.h`
- Add new root dir `public/include/` for bridge-specific Remix API header
  - `public/include/remixapi/bridge_remix_api.h`
- Adds new util `Serializable<T>` helper class in `util_serializable.h`
  - Defines a uniform interface of functions to de-/serialize generic classes
- Adds new RemixApi util helpers in `util_remixapi.h/.cpp`
  - Defines `Serializable<T>`s for remixapi types, and adds extra helpers to handle their quirks
- Adds `remix_api.h/.cpp` to both Client and Server\
  - Helpers for handling the x86 -> x64 conversion
@nv-nfreybler
Copy link
Collaborator

Please see commit 2dd0a0e.
Binaries

We overhauled quite a bit from the original PR, mostly to eliminate the code duplication between the original bridge_c.h and remix_c.h. We also wanted 1:1 parity with the standard Remix API, so we went with a singular CreateLight etc as opposed to extension-specific APIs. Additionally we found some memory leaks, and needed to refactor accordingly.

Thank you again for the PR. If you consider this sufficient, please go ahead and close this, or else we can.

@xoxor4d
Copy link
Contributor Author

xoxor4d commented Sep 25, 2024

I'll now close the PR because the latest builds are running great and everything seems to be ironed out and working as expected.
Communication was great and it was a blast working on this. Thanks again to all the people involved! I really appreciate you all and am looking forward to the next PR 🙂

@xoxor4d xoxor4d closed this Sep 25, 2024
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.

3 participants