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

Conversation

@eggrobin
Copy link
Member

commented Sep 20, 2014

No description provided.

int const parent,
Displacement<AliceSun> const& from_parent_position,
Velocity<AliceSun> const& from_parent_velocity) {
CHECK(celestials_.find(parent) != celestials_.end()) << "No body at index "

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Put the entire message (including the <<) on the next line. (Several places in this file.)

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Not specified by the styleguide,
inconsistent with the conventions used it glog:
https://code.google.com/p/google-glog/source/browse/trunk/src/logging.cc#1230

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

Velocity<AliceSun> const& from_parent_velocity) {
CHECK(celestials_.find(parent) != celestials_.end()) << "No body at index "
<< parent;
Celestial* const parent_body = celestials_[parent].get();

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

You could save a lookup by first getting an iterator, checking that it's not end(), and dereferencing it here.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done everywhere. Also allows consting of some members.

if (trajectories.front()->last_time() != t) {
// TODO(egg): This is bound to happen given how the integrator works now...
LOG(ERROR) << "Integration went "
<< trajectories.front()->last_time() - t << " too far.";

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Align the << on the one above.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

CHECK(vessels_.find(guid) != vessels_.end()) << "No vessel with GUID "
<< guid;
CHECK(vessels_[guid]->history != nullptr) << "Vessel with GUID " << guid
<< "was not given an initial state";

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

s/"/" /

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

<< "was not given an initial state";
Velocity<Barycentre> const barycentric_result =
vessels_[guid]->history->last_velocity() -
vessels_[guid]->parent->history->last_velocity();

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

As mentioned above it would be nice to avoid all these lookups that may not be optimized. Something like:

auto const vessel = vessels_.find(guid);

and then use vessel instead of vessels_[guid]. It's a bit more readable too.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

yeah.

namespace principia {
namespace ksp_plugin_adapter {

[KSPAddon(KSPAddon.Startup.EveryScene, false)]
public class PluginAdapter : MonoBehaviour {
[KSPAddon(KSPAddon.Startup.Flight, false)]

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Comment the false.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Will use named arguments, it's called "once".
It has been documented by @Anatid (aka The_Duck) here.

private static extern bool InsertOrKeepVessel(
IntPtr plugin,
[MarshalAs(UnmanagedType.LPStr)]
String guid,

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Preference for having the String immediately after the [MarshalAs(...)]. It will make it easier to see that the latter applies to the former.

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Agreed and done.

}
}

#region Unity Lifecycle

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

Some comments would be welcome in the code below. At the moment it's totally mystifying.

This comment has been minimized.

GUILayout.TextArea(text : Marshal.PtrToStringAnsi(hello_ptr));
GUILayout.EndVertical();
UnityEngine.GUILayout.TextArea(text : Marshal.PtrToStringAnsi(hello_ptr));
if (UnityEngine.GUILayout.Button(plugin_running_? "Stop plugin"

This comment has been minimized.

Copy link
@pleroy

pleroy Sep 21, 2014

Member

s/?/ ?/

This comment has been minimized.

Copy link
@eggrobin

eggrobin Sep 21, 2014

Author Member

Done.

@pleroy pleroy closed this Sep 21, 2014

@eggrobin eggrobin reopened this Sep 21, 2014

@eggrobin eggrobin force-pushed the eggrobin:the-return-of-the-plugin branch from 1b72c60 to e4351da Sep 21, 2014

@eggrobin eggrobin force-pushed the eggrobin:the-return-of-the-plugin branch from e4351da to 3ce8341 Sep 21, 2014

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/interface.cpp in 3ce8341 Sep 21, 2014

CHECK_NOTNULL(plugin); as the first line in the function.

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

Done and documented, as discussed, on the second line, so that the logs are more informative.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/interface.cpp in 3ce8341 Sep 21, 2014

s/the/The/

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

Done.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/interface.cpp in 3ce8341 Sep 21, 2014

I would just write constexpr, it's not an identifier or other expression.

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

Done.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/interface.hpp in 3ce8341 Sep 21, 2014

s/dll/DLL/

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

Done.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/plugin.cpp in 3ce8341 Sep 21, 2014

Celestial const& parent_body = *it->second;

to express that you don't expect the pointer to be null. This applies to many places in this file (of course, not to the places that are not const).

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

Done in many places.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/plugin.cpp in 3ce8341 Sep 21, 2014

A similar identifier is called parent_body above. Pick one and be consistent.

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

parent.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/plugin.cpp in 3ce8341 Sep 21, 2014

Would this fit on the previous line?

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

It would and does.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/plugin.cpp in 3ce8341 Sep 21, 2014

Would this fit on the previous line?

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

No, a semicolon too long.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/plugin.hpp in 3ce8341 Sep 21, 2014

Inserts.

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

Done.

@pleroy

This comment has been minimized.

Copy link

commented on ksp_plugin/plugin.hpp in 3ce8341 Sep 21, 2014

I don't think that "one parameter per line" extends to the comments ;-> You can pack them here.

This comment has been minimized.

Copy link
Owner Author

replied Sep 21, 2014

But I will not, because it's unreadable. While adequate syntax highlighting makes the arguments stand out in the code, there is no (C#!) syntax highlighting for the comments.
This is arguably more important while writing code in the C# adapter than the actual list of arguments, so let's leave it unpacked.

@pleroy pleroy closed this Sep 21, 2014

@eggrobin eggrobin reopened this Sep 21, 2014

@pleroy pleroy added the LGTM label Sep 21, 2014

pleroy added a commit that referenced this pull request Sep 21, 2014

@pleroy pleroy merged commit 80c4a92 into mockingbirdnest:master Sep 21, 2014

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.