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

An interface that can actually be called #2089

Merged
merged 14 commits into from
Mar 10, 2019

Conversation

eggrobin
Copy link
Member

@eggrobin eggrobin commented Mar 8, 2019

The ExternalInterface abstracts away the fetching and passing of the plugin, and converts statuses into more idiomatic exceptions; it turns out parameters into return values, since the former are annoying to use by reflection.
By wrapping the Interface.ExternalMeow calls, it works around the calling convention issues described below.

See the draft documentation at https://github.com/mockingbirdnest/Principia/wiki/Interface-for-other-KSP-mods.

The API added in #2079 for #2074 is cumbersome to call directly (the caller has to look for a PrincipiaPluginAdapter, fetch its private plugin_ field, and check that the private static field Loader.loaded_principia_dll_ is true), and calls to functions with parameters fail hard anyway, because mono somehow messes up the calling convention when calling a P/Invoke function directly by reflection: an attempt to call

 internal static extern Status ExternalGeopotentialGetCoefficient(
     this IntPtr plugin,
     int body_index,
     int degree,
     int order,
     out XY coefficient);
extern "C" PRINCIPIA_DLL
Status CDECL principia__ExternalGeopotentialGetCoefficient(
    Plugin const* const plugin,
    int const body_index,
    int const degree,
    int const order,
    XY* const coefficient);

with

plugin = 0x5AFEA730,
body_index = 1,
degree = 2,
order = 0

led to, on the C++ side,

plugin = 1,
body_index = 2,
order = 0x3F3B58D0
coefficient = 18

and a subsequent access violation when dereferencing 1.

ksp_plugin_adapter/external_interface.cs Outdated Show resolved Hide resolved
ksp_plugin_adapter/external_interface.cs Outdated Show resolved Hide resolved
ksp_plugin_adapter/ksp_plugin_adapter.cs Outdated Show resolved Hide resolved
tools/journal_proto_processor.cpp Outdated Show resolved Hide resolved
tools/journal_proto_processor.cpp Outdated Show resolved Hide resolved
@pleroy pleroy added the LGTM label Mar 9, 2019
@pleroy
Copy link
Member

pleroy commented Mar 10, 2019

retest this please

@pleroy pleroy merged commit ab556f7 into mockingbirdnest:master Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants