Part based pile ups in plugin #1220

eggrobin · 2017-03-03T18:42:31Z

No description provided.

This reverts commit 8412121.

pleroy · 2017-03-04T08:32:45Z

ksp_plugin/plugin.cpp

  CHECK(!initializing_);
  not_null<Celestial const*> parent =
      FindOrDie(celestials_, parent_index).get();
-  auto inserted =
+  auto pair =


I recommend to use std::tie for this kind of calls.

pleroy · 2017-03-04T08:36:10Z

ksp_plugin/plugin.cpp

+    if (vessel == current_vessel) {
+      vessel->KeepPart(part_id);
+    } else {
+      FindOrDie(part_id_to_vessel_, part_id) = vessel;


Why not it->second on the LHS to avoid a second lookup?

Done (with a reference, so we can have names)

pleroy · 2017-03-04T08:37:02Z

ksp_plugin/plugin.cpp

+      vessel->AddPart(current_vessel->ExtractPart(part_id));
+    }
+  } else {
+    auto const pair = part_id_to_vessel_.emplace(part_id, vessel);


std::tie.

pleroy · 2017-03-04T08:39:03Z

ksp_plugin/plugin.cpp

+    not_null<Vessel*> const vessel,
+    DegreesOfFreedom<Barycentric> const& degrees_of_freedom) {
+  auto it = part_id_to_vessel_.find(part_id);
+  bool const found = it != part_id_to_vessel_.end();


Maybe call this part_found to clarify what is found.

pleroy · 2017-03-04T08:43:08Z

ksp_plugin/plugin.cpp

+      it = vessels_.erase(it);
+    }
+  }
+  // Free old parts, and bind vessels.


No comma here, it's not an enumeration.

pleroy · 2017-03-04T08:53:14Z

ksp_plugin/plugin.hpp

+  // Calls |SetPartApparentDegreesOfFreedom| on the pile-up containing the
+  // relevant part.  This part must be in a loaded vessel.
+  virtual void SetPartApparentDegreesOfFreedom(
+      PartId const part_id,


No const.

pleroy · 2017-03-04T08:54:13Z

ksp_plugin/plugin.hpp

@@ -290,7 +308,7 @@ class Plugin {

  // Notifies |this| that the given vessels are touching, and should gravitate
  // as part of a single rigid body.
-  virtual void ReportCollision(GUID const& vessel1, GUID const& vessel2) const;
+  virtual void ReportCollision(PartId const& part1, PartId const& part2) const;


Pass PartIds by copy.

pleroy · 2017-03-04T08:55:24Z

ksp_plugin/plugin.hpp

  GUIDToOwnedVessel vessels_;
+  std::map<PartId, not_null<Vessel*>> part_id_to_vessel_;


Document this map: "For each part, the vessel that this part belongs to. The part is guaranteed to be part of the parts() map of the vessel, and owned by it."

pleroy · 2017-03-04T08:55:55Z

ksp_plugin/plugin.hpp

@@ -460,7 +476,7 @@ class Plugin {
  // Do not |erase| from this list, use |Vessel::clear_pile_up| instead.
  std::list<PileUp> pile_ups_;

-  std::list<std::list<PileUp>::iterator> pile_ups_in_bubble_;
+  std::set<not_null<Vessel*>> loaded_vessels_;


Comment: "The vessels that are currently loaded, i.e. in the physics bubble."

pleroy · 2017-03-04T08:58:47Z

ksp_plugin/vessel.hpp

@@ -83,9 +83,15 @@ class Vessel {
  // Returns the part with the given ID.  Such a part must have been added using
  // |AddPart|.
  virtual not_null<Part*> part(PartId id) const;
+  virtual std::map<PartId, not_null<std::unique_ptr<Part>>> const& parts()
+      const;


There is a bit of an abstraction leak here as we expose the data structure internal to the vessel. Maybe it would be better to have GetSomePart (returning the first one, but that's not part of the contract) and DoForAllParts (taking a lambda).

Done, ForAllParts, ForSomePart.

pleroy · 2017-03-04T12:03:59Z

ksp_plugin/plugin.hpp

@@ -419,6 +425,8 @@ class Plugin {
  bool is_loaded(not_null<Vessel*> vessel) const;

  GUIDToOwnedVessel vessels_;
+  // For each part, the vessel that this part belongs to. The part is guaranteed
+  // to be part of the parts() map of the vessel, and owned by it.


s/to be part of/to be in/

pleroy · 2017-03-04T12:05:11Z

ksp_plugin/plugin.cpp

-      Subset<Part>::Unite(Subset<Part>::Find(first_part),
-                          Subset<Part>::Find(part));
-    }
+    vessel->ForSomePart([vessel](Part& first_part) {


Would it work to make this const&? (Same question in other places.)

eggrobin added 11 commits March 3, 2017 00:36

Revert "don't change the plugin yet"

eee1255

This reverts commit 8412121.

Merge branch 'burst-more-bubble' into part-based-pile-ups-in-plugin

b40971d

some plugin work

6559ad4

misc compilation errors

a02ec21

resurrect a function

bce4324

moar plugin work

f7c51b5

it compiles (which means it must be correct, right?)

124fd5a

so now it's even more correct!

921616b

checks, comments, and missing clears

da5bf58

it compiles again

65e8a4e

remove header of removed function, add predicate

d7e775f

pleroy reviewed Mar 4, 2017

View reviewed changes

after pleroy's review

339a54b

pleroy reviewed Mar 4, 2017

View reviewed changes

pleroy added the LGTM label Mar 4, 2017

after pleroy's second review

3849e4f

eggrobin merged commit 5253a03 into mockingbirdnest:Cardano Mar 4, 2017

eggrobin mentioned this pull request Mar 4, 2017

Interface for part-based pile ups #1226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part based pile ups in plugin #1220

Part based pile ups in plugin #1220

eggrobin commented Mar 3, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

pleroy Mar 4, 2017

eggrobin Mar 4, 2017

		GUIDToOwnedVessel vessels_;
		std::map<PartId, not_null<Vessel*>> part_id_to_vessel_;

Part based pile ups in plugin #1220

Part based pile ups in plugin #1220

Conversation

eggrobin commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment