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

The return of the plugin #151

Merged
merged 54 commits into from Sep 21, 2014
Merged
Changes from 52 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
ff2eb93
bikeshedding
eggrobin Sep 3, 2014
b348f44
Add some files.
eggrobin Sep 4, 2014
9828fc7
Merge branch 'master' into the-return-of-the-plugin
eggrobin Sep 4, 2014
2ad75f3
Manage the vessels and celestials.
eggrobin Sep 5, 2014
39c9067
Implement.
eggrobin Sep 5, 2014
01d5d3a
things
eggrobin Sep 6, 2014
55c0d21
moar headers
eggrobin Sep 6, 2014
02ea212
valse hésitation
eggrobin Sep 6, 2014
94a5616
This API is starting to make sense...
eggrobin Sep 6, 2014
ad0d6d2
Give the sun to the constructor, so we don't need to pass pointers
eggrobin Sep 6, 2014
2cf8c34
?
eggrobin Sep 7, 2014
e2b4f4f
Merge branch 'master' into the-return-of-the-plugin
eggrobin Sep 9, 2014
62bd7a0
things
eggrobin Sep 9, 2014
b3d1eb8
things
eggrobin Sep 9, 2014
623aec7
Merge remote-tracking branch 'la-vache/master' into the-return-of-the…
eggrobin Sep 9, 2014
9d11b86
waiting for a cl to be merged
eggrobin Sep 10, 2014
4302a4d
Merge remote-tracking branch 'la-vache/pr/148' into the-return-of-the…
eggrobin Sep 10, 2014
9ac0f93
resume working
eggrobin Sep 10, 2014
4cd1112
yet another method
eggrobin Sep 10, 2014
b1d0ced
Merge remote-tracking branch 'la-vache/master' into the-return-of-the…
eggrobin Sep 10, 2014
716c98a
It compiles *something*.
eggrobin Sep 11, 2014
8c376c3
There was a bug in Permutation.
eggrobin Sep 11, 2014
1c4c62e
state-setting implemented
eggrobin Sep 11, 2014
7e9d6ce
make it compile
eggrobin Sep 11, 2014
499bbbc
misc lint
eggrobin Sep 11, 2014
a4303cd
Integrate.
eggrobin Sep 11, 2014
8da9bbf
4 feedback methods.
eggrobin Sep 11, 2014
1aedcfa
preprocessor nonsense
eggrobin Sep 11, 2014
4a90f94
Typo.
eggrobin Sep 13, 2014
f0220e1
Typo!
eggrobin Sep 14, 2014
71779e8
Typo...
eggrobin Sep 15, 2014
c246dc0
Interface for a planet-pinning mod.
eggrobin Sep 16, 2014
b3434f3
It's been too long since the last commit...
eggrobin Sep 16, 2014
378e3ce
Rebooting!
eggrobin Sep 17, 2014
b503dd3
DFS
eggrobin Sep 18, 2014
b62d365
Document and assert.
eggrobin Sep 18, 2014
91e78fb
It compiles...
eggrobin Sep 19, 2014
3c829c9
Hooray segfaults!
eggrobin Sep 19, 2014
005ab0d
Initialise that pointer....
eggrobin Sep 19, 2014
d758ad7
ugly
eggrobin Sep 19, 2014
d51f345
blah
eggrobin Sep 19, 2014
540d124
more logging stuff
eggrobin Sep 19, 2014
d2e222d
reword comment
eggrobin Sep 19, 2014
2bc4a08
Follow the docs
eggrobin Sep 19, 2014
6b51ad4
LOG(INFO) << all_the_things;
eggrobin Sep 20, 2014
b63d3b3
Enough logging to find a bug
eggrobin Sep 20, 2014
aad7372
VLOG(1) << things_that_get_called_all_the_time
eggrobin Sep 20, 2014
70bb429
No Unity logging. Unity logging is slow.
eggrobin Sep 20, 2014
b6a81a5
lint
eggrobin Sep 20, 2014
83b67f1
CL time, merging master
eggrobin Sep 20, 2014
3e9f23a
more lint.
eggrobin Sep 20, 2014
1aa9a51
obsolete log
eggrobin Sep 20, 2014
3ce8341
After pleroy's review
eggrobin Sep 21, 2014
39fc5f8
After pleroy's second review
eggrobin Sep 21, 2014
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -51,7 +51,8 @@ Ignore any compiler or linker warnings. Close the solution.
`<root>\Google`. There should be a file at `<root>\Google\gmock-1.7.0\README`
if the unpacking was done correctly.
9. Make `<root>\Google\gmock-1.7.0` and its contents *not* readonly.
10. Open `<root>\Google\gmock-1.7.0\msvc\2010\gmock.sln` with Visual Studio 2013.
10. Open `<root>\Google\gmock-1.7.0\msvc\2010\gmock.sln` with Visual Studio
2013.
You will be prompted to upgrade, do so. Save and close the solution,
`git add -A`, commit.
11. Set the build configuration to Debug.
@@ -65,17 +66,20 @@ You will be prompted to upgrade, do so. Save and close the solution,
14. For every project, set the runtime library to Multi-threaded DLL
(`/MD`), similarly to step 12.
15. In both build configurations, add glog to the include path of gmock_main by
prepending `..\..\..\glog-0.3.3\src\windows;` to `Additional Include Directories`
prepending `..\..\..\glog-0.3.3\src\windows;` to `Additional Include
Directories`
in `gmock_main Properties -> C/C++ -> General`.
16. Save everything, close the solution and commit.
17. Copy `<root>\Principia\Documentation\Setup Files\gmock.patch` to
`<root>\Google\gmock.patch`, then, in `<root>\Google`, run `git am gmock.patch`.
18. Delete `<root>\Google\gmock.patch`.
19. Open `<root>\Google\gmock-1.7.0\msvc\2010\gmock.sln`.
20. Build for Debug and Release.
21. Unhide `<root>\Google\.git`, e.g. by running `attrib -h .git` in `<root>\Google`, then delete `<root>\Google\.git`.
21. Unhide `<root>\Google\.git`, e.g. by running `attrib -h .git` in
`<root>\Google`, then delete `<root>\Google\.git`.
22. In `<root>\Google`, run `git clone https://github.com/pleroy/benchmark.git`.
23. Open `<root>\Google\benchmark\msvc\google-benchmark.sln` with Visual Studio 2013.
23. Open `<root>\Google\benchmark\msvc\google-benchmark.sln` with Visual Studio
2013.
24. Build for Debug and Release.

You are now done with the setup of the dependencies.
@@ -152,7 +152,7 @@ Permutation<FromFrame, ToFrame> operator*(
{{Right::YXZ, Left::ZYX}, Result::ZXY},
{{Right::YXZ, Left::YXZ}, Result::XYZ}};
return Result(multiplication.at({right.coordinate_permutation_,
left.coordinate_permutation_}));
left.coordinate_permutation_}));

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Broken indent. Please undo.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

}

} // namespace geometry
@@ -0,0 +1,133 @@
#include "ksp_plugin/interface.hpp"

#include <string>

using principia::si::Degree;
using principia::si::Metre;

namespace principia {
namespace ksp_plugin {

void InitGoogleLogging() {
#ifdef _MSC_VER

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Add a comment explaining why you are doing this.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Explained in the header file.

FILE* file;
freopen_s(&file, "stderr.log", "w", stderr);
#else
std::freopen("stderr.log", "w", stderr);
#endif
google::SetStderrLogging(google::INFO);
google::SetLogDestination(google::FATAL, "glog/Principia/FATAL.");
google::SetLogDestination(google::ERROR, "glog/Principia/ERROR.");
google::SetLogDestination(google::WARNING, "glog/Principia/WARNING.");
google::SetLogDestination(google::INFO, "glog/Principia/INFO.");
google::InitGoogleLogging("Principia");
LOG(INFO) << "Initialized Google logging for Principia.";
}

void LOGINFO(char const* message) {
LOG(INFO) << message;
}

void LOGWARNING(char const* message) {
LOG(WARNING) << message;
}

void LOGERROR(char const* message) {
LOG(ERROR) << message;
}

void LOGFATAL(char const* message) {
LOG(FATAL) << message;
}

Plugin* CreatePlugin(double const initial_time, int const sun_index,
double const sun_gravitational_parameter,
double const planetarium_rotation_in_degrees) {
LOG(INFO) << "Creating Principia...";
return new Plugin(

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Methinks that the arguments might fit immediately after the (.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

sun_gravitational_parameter * SIUnit<GravitationalParameter>(), definitely does not.

Instant(initial_time * Second),
sun_index,
sun_gravitational_parameter * SIUnit<GravitationalParameter>(),
planetarium_rotation_in_degrees * Degree);
LOG(INFO) << "Plugin created.";
}

void DestroyPlugin(Plugin* plugin) {

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Log that you are about to destroy here?

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

delete plugin;
plugin = nullptr;

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

This statement, it doesn't mean what you think it means.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

changed to Plugin** const plugin, deletes *plugin and nulls it, called as ref from C#.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Also changed the other Plugin* to Plugin* const.

LOG(INFO) << "Destroyed Principia.";
}

void InsertCelestial(Plugin* plugin, int const index,
double const gravitational_parameter,
int const parent,
XYZ const from_parent_position,
XYZ const from_parent_velocity) {
plugin->InsertCelestial(

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

CHECK_NOTNULL(plugin)-> (everywhere).

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done and documented.

index,
gravitational_parameter * SIUnit<GravitationalParameter>(),
parent,
Displacement<AliceSun>({from_parent_position.x * Metre,
from_parent_position.y * Metre,
from_parent_position.z * Metre}),
Velocity<AliceSun>({from_parent_velocity.x * Metre / Second,

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Note that SIUnit<Speed>() would be a bit more efficient than Metre / Second. I'm not asking for a change though, but maybe a NOTE(egg) in case it become a performance issue at some point.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

It probably has a (tiny) influence right now, but if that were changed to

* (Metre / Second)

and whenever we get constexpr support this should be done at compile time. Will change the parentheses and add a NOTE(egg).

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

from_parent_velocity.y * Metre / Second,
from_parent_velocity.z * Metre / Second}));
}

void UpdateCelestialHierarchy(Plugin* plugin, int const index,
int const parent) {
plugin->UpdateCelestialHierarchy(index, parent);
}

void InsertOrKeepVessel(Plugin* plugin, char const* guid, int const parent) {
plugin->InsertOrKeepVessel(guid, parent);
}

void SetVesselStateOffset(Plugin* plugin, char const* guid,
XYZ const from_parent_position,
XYZ const from_parent_velocity) {
plugin->SetVesselStateOffset(
guid,
Displacement<AliceSun>({from_parent_position.x * Metre,
from_parent_position.y * Metre,
from_parent_position.z * Metre}),
Velocity<AliceSun>({from_parent_velocity.x * Metre / Second,
from_parent_velocity.y * Metre / Second,
from_parent_velocity.z * Metre / Second}));
}

XYZ VesselDisplacementFromParent(Plugin* plugin, char const* guid) {
R3Element<Length> const result =
plugin->VesselDisplacementFromParent(guid).coordinates();
return {result.x / Metre, result.y / Metre, result.z / Metre};
}

XYZ VesselParentRelativeVelocity(Plugin* plugin, char const* guid) {
R3Element<Speed> const result =
plugin->VesselParentRelativeVelocity(guid).coordinates();
return {result.x / (Metre / Second),
result.y / (Metre / Second),
result.z / (Metre / Second)};
}

XYZ CelestialDisplacementFromParent(Plugin* plugin, int const index) {
R3Element<Length> const result =
plugin->CelestialDisplacementFromParent(index).coordinates();
return {result.x / Metre, result.y / Metre, result.z / Metre};;
}

XYZ CelestialParentRelativeVelocity(Plugin* plugin, int const index) {
R3Element<Speed> const result =
plugin->CelestialParentRelativeVelocity(index).coordinates();
return {result.x / (Metre / Second),
result.y / (Metre / Second),
result.z / (Metre / Second)};
}

char const* SayHello() {
return "Hello from native C++!";
}

} // namespace ksp_plugin
} // namespace principia
@@ -0,0 +1,100 @@
#pragma once

#include <type_traits>

#include "ksp_plugin/plugin.hpp"

// DLL-exported functions for interfacing with Platform Invocation Services.

#if defined(_WIN32)
#define DLLEXPORT __declspec(dllexport)
#else
#define DLLEXPORT __attribute__((visibility("default")))
#endif

namespace principia {
namespace ksp_plugin {

extern "C"
struct XYZ { double x, y, z; };

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

\n after { and before }.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

static_assert(std::is_standard_layout<XYZ>::value,
"XYZ is used for interfacing");

// Sets stderr to log INFO, and redirects stderr, which Unity does not log, to
// "<KSP directory>/stderr.log". This provides an easily accessible file
// containing a sufficiently verbose log of the latest session, instead of
// requiring users to dig in the archive of all past logs at all severities.
// This archive is written to
// "<KSP directory>/glog/Principia/<SEVERITY>.<date>-<time>.<pid>",
// where date and time are in ISO 8601 basic format.
// TODO(egg): libglog should really be statically linked, what happens if two
// plugins use glog?
extern "C" DLLEXPORT
void InitGoogleLogging();

// Exports |LOG(SEVERITY) << message| for fast logging from the C# adapter.

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Comment that this evaluates the argument so it is less efficient than directly doing LOG(INFO).

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

extern "C" DLLEXPORT
void LOGINFO(char const* message);

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Should be LogInfo. LOGINFO should be reserved for macros, and it's not a macro.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Styleguide says that, but glog says https://code.google.com/p/google-glog/source/browse/trunk/src/windows/glog/log_severity.h#57, and styleguide says

Remember that consistency includes local consistency

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

As discussed, faut pas.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

As discussed, done.

extern "C" DLLEXPORT
void LOGWARNING(char const* message);
extern "C" DLLEXPORT
void LOGERROR(char const* message);
extern "C" DLLEXPORT
void LOGFATAL(char const* message);

// Returns a pointer to a plugin constructed with the arguments given.

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

"given arguments" (many places in this file).

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Googling "with the arguments given" yields

About 1,280,000 results

Googling "with the given arguments" yields

About 635,000 results

extern "C" DLLEXPORT
Plugin* CreatePlugin(double const initial_time, int const sun_index,

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

The usual naming for this is NewPlugin. Also, one parameter per line please.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

one parameter per line please.

That's not what we've been doing up to now, and that's not what the styleguide says.

ReturnType ClassName::FunctionName(Type par_name1, Type par_name2) {
  DoSomething();
  ...
}

If you have too much text to fit on one line:

ReturnType ClassName::ReallyLongFunctionName(Type par_name1, Type par_name2,
                                             Type par_name3) {
  DoSomething();
  ...
}

or if you cannot fit even the first parameter:

ReturnType LongClassName::ReallyReallyReallyLongFunctionName(
    Type par_name1,  // 4 space indent
    Type par_name2,
    Type par_name3) {
  DoSomething();  // 2 space indent
  ...
}

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

As discussed, abiding #152.

double const sun_gravitational_parameter,
double const planetarium_rotation_in_degrees);

// Deletes |plugin|.
extern "C" DLLEXPORT
void DestroyPlugin(Plugin* plugin);

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

DeletePlugin? Otherwise it's odd that that comment says "deletes".

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Well, deletes |plugin|, and thus calls the destructor of |*plugin|, which is a |Plugin|, dawg.

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

As discussed, "delete".

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.


// Calls |plugin->InsertCelestial| with the arguments given.
extern "C" DLLEXPORT
void InsertCelestial(Plugin* plugin, int const index,

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

One parameter per line (many places in this file).

Also please document ownership transfer (or lack thereof) for all pointers in this file.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

double const gravitational_parameter,
int const parent,

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Is parent an index? If so, it should probably be named parent_index for clarity.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

As discussed, using using instead.

XYZ const from_parent_position,
XYZ const from_parent_velocity);

// Calls |plugin->InsertCelestial| with the arguments given.

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Or is it UpdateCelestialHierarchy?

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done

extern "C" DLLEXPORT
void UpdateCelestialHierarchy(Plugin* plugin, int const index,
int const parent);

// Calls |plugin->InsertOrKeepVessel| with the arguments given.
extern "C" DLLEXPORT
void InsertOrKeepVessel(Plugin* plugin, char const* guid, int const parent);

// Calls |plugin->SetVesselStateOffset| with the arguments given.
extern "C" DLLEXPORT
void SetVesselStateOffset(Plugin* plugin, char const* guid,
XYZ const from_parent_position,
XYZ const from_parent_velocity);

// Calls |plugin->VesselDisplacementFromParent| with the arguments given.
extern "C" DLLEXPORT
XYZ VesselDisplacementFromParent(Plugin* plugin,
char const* guid);

// Calls |plugin->VesselParentRelativeVelocity| with the arguments given.
extern "C" DLLEXPORT
XYZ VesselParentRelativeVelocity(Plugin* plugin,
char const* guid);

// Calls |plugin->CelestialDisplacementFromParent| with the arguments given.
extern "C" DLLEXPORT
XYZ CelestialDisplacementFromParent(Plugin* plugin, int const index);

// Calls |plugin->CelestialParentRelativeVelocity| with the arguments given.
extern "C" DLLEXPORT
XYZ CelestialParentRelativeVelocity(Plugin* plugin, int const index);

extern "C" DLLEXPORT

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Comment why this is here.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

char const* SayHello();

} // namespace ksp_plugin
} // namespace principia

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Can we #undef DLLEXPORT here?

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done, together with a

#if defined(DLLEXPORT)
#error "DLLEXPORT already defined"
#else
// ...
#endif

at the beginning.

@@ -33,10 +33,12 @@
</ImportGroup>
<ImportGroup Label="PropertySheets" Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
<Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
<Import Project="..\google_test_framework.props" />
<Import Project="..\include_solution.props" />
</ImportGroup>
<ImportGroup Label="PropertySheets" Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
<Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
<Import Project="..\google_test_framework.props" />
<Import Project="..\include_solution.props" />
</ImportGroup>
<PropertyGroup Label="UserMacros" />
@@ -74,10 +76,12 @@
</Link>
</ItemDefinitionGroup>
<ItemGroup>
<ClInclude Include="test_plugin.hpp" />
<ClInclude Include="plugin.hpp" />
<ClInclude Include="interface.hpp" />
</ItemGroup>
<ItemGroup>
<ClCompile Include="test_plugin.cpp" />
<ClCompile Include="interface.cpp" />
<ClCompile Include="plugin.cpp" />
</ItemGroup>
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
<ImportGroup Label="ExtensionTargets">
@@ -11,12 +11,18 @@
</Filter>
</ItemGroup>
<ItemGroup>
<ClInclude Include="test_plugin.hpp">
<ClInclude Include="plugin.hpp">
<Filter>Header Files</Filter>
</ClInclude>
<ClInclude Include="interface.hpp">
<Filter>Header Files</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<ClCompile Include="test_plugin.cpp">
<ClCompile Include="interface.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="plugin.cpp">
<Filter>Source Files</Filter>
</ClCompile>
</ItemGroup>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.