Skip to content

Conversation

@alafi
Copy link

@alafi alafi commented May 26, 2018

The changes are:

  1. Add pluginPath param to Initialize method in Simulator class with default to null.
  2. Deleted the local copy of MengeCore_d.dll and enhanced the pre-build events to copy the dll that matches the build configuration from the external folder for both MengeCS and MengeCSExe projects.

This should also close issue #7


This change is Reviewable

@curds01
Copy link
Collaborator

curds01 commented May 27, 2018

I really appreciate you doing this. It's great to have you give back to the community. I particularly like the refactoring of sourcing the dlls directly from the menge directory.

However, this feels like two different PRs fixing two different issues:

  • The first addresses the issue you had in Failed in build #7.

    • This is flawed; you've copied over the debug version of the dll, but MengeWrapper.cs doesn't reference it. So, even if I build this in Debug mode, the project will still access the release version of MengeCore.dll.
  • The second adds the plug-in path to the CS bindings.

    • This feels incomplete. As you found, the plugins have lots of dependencies (including MengeVis.dll and even some of the SDL, opengl, etc. dlls). Exposing the plugin path through the CS API without documenting this need will produce a lot of confusion and frustration.
    • I'm working on a fix for the plug-in path which is going to include:
      • Adding a build target to the plugins that excludes the MengeVis dependencies
      • Documents the Menge build, the CS build, and how to use the plugins
      • Add documentation to MengeUnity and MengeCS which indicates the sha of the upstream repository it depends on so that they are kept clearly in sync.

So, I have the following recommendations:

  • Split this PR into two separate PRs
  • You can finish off the first PR (where the dll you use depends on the build).
  • I'd say let me do the PR for the plug-in path so it links across all of the menge libraries.

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