From 25475a92fbdccb790199a1c1d235adb11742b6e6 Mon Sep 17 00:00:00 2001
From: Billy Olsen <billy.olsen@gmail.com>
Date: Sun, 5 Apr 2020 21:01:50 -0700
Subject: [PATCH] Rework the FlightConfiguration.calculateBounds() for
 instances

Use the information provided in the getActiveInstances() in order to
calculate the length of the rocket. This change takes each component
instance and uses the component bounds and the instance's transform
in order to determine the instance's actual bounds. The length is
then calculated as the difference between the min and max X values,
using the BoundingBox.

Note, this particular change special-cases a few of the components
in order to get the right length. It is preferred to revisit each
special case in subsequent patches in order to keep this patch set
minimal for the time being.

The length calculations are already stressed during unit tests, but
the results are a bit more accurate and thus the unit tests are
updated to reflect the new values.

Fixes #452

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
---
 .../rocketcomponent/FlightConfiguration.java  | 66 +++++++++++++++++--
 .../FlightConfigurationTest.java              |  4 +-
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/core/src/net/sf/openrocket/rocketcomponent/FlightConfiguration.java b/core/src/net/sf/openrocket/rocketcomponent/FlightConfiguration.java
index 803013df3..7a6257f46 100644
--- a/core/src/net/sf/openrocket/rocketcomponent/FlightConfiguration.java
+++ b/core/src/net/sf/openrocket/rocketcomponent/FlightConfiguration.java
@@ -1,6 +1,7 @@
 package net.sf.openrocket.rocketcomponent;
 
 import java.util.ArrayDeque;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
@@ -551,16 +552,71 @@ public class FlightConfiguration implements FlightConfigurableParameter<FlightCo
 		return cachedBounds;
 	}
 
+	/**
+	 * Calculates the bounds for all the active component instances
+	 * in the current configuration.
+	 */
 	private void calculateBounds(){
 		BoundingBox bounds = new BoundingBox();
-			
-		for (RocketComponent component : this.getActiveComponents()) {
-			BoundingBox componentBounds = new BoundingBox().update(component.getComponentBounds());
-			bounds.update( componentBounds );
-		}
 
+		InstanceMap map = getActiveInstances();
+		for (Map.Entry<RocketComponent, java.util.ArrayList<InstanceContext>>  entry : map.entrySet()) {
+			RocketComponent component = entry.getKey();
+			List<InstanceContext> contexts = entry.getValue();
+						
+			Collection<Coordinate> coordinates = new ArrayList<Coordinate>();
+			/* FinSets are a bit different in how they store their bounds,
+			 * so we'll use the fin points for calculations. 
+			 */
+			if (component instanceof FinSet) {
+				coordinates.addAll(Arrays.asList(((FinSet) component).getFinPoints()));
+			} else {
+				coordinates.addAll(component.getComponentBounds());
+			}
+			BoundingBox componentBox = new BoundingBox();
+			List<Coordinate> transformedCoords = new ArrayList<Coordinate>();
+			for (InstanceContext ctxt : contexts) {
+				/*
+				 * If the instance is not active in the current context, then
+				 * skip the bound calculations. This is mildly confusing since
+				 * getActiveInstances() implies that it will only return the
+				 * instances that are active, but it returns all instances and
+				 * the context indicates if it is active or not.
+				 */
+				if (!ctxt.active) {
+					continue;
+				}
+				for (Coordinate c : coordinates) {
+					Coordinate tc = null;
+					/* These components do not need the transform performed in
+					 * order to provide the proper coordinates for length calculation.
+					 * The transformation will cause the values to be calculated
+					 * incorrectly. This should be fixed in the appropriate places
+					 * not handled as one-offs in here.
+					 */
+					if ((component instanceof AxialStage) || (component instanceof BodyTube) ||
+						(component instanceof PodSet)) {
+						tc = c;
+					} else {
+						tc = ctxt.transform.transform(c);
+					}
+					componentBox.update(tc);
+					transformedCoords.add(tc);
+				}
+			}
+			
+			bounds.update(componentBox);
+		}
+		
 		boundsModID = rocket.getModID();
 		cachedLength = bounds.span().x;
+		/* Special case for the scenario that all of the stages are removed and are
+		 * inactive. Its possible that this shouldn't be allowed, but it is currently
+		 * so we'll just adjust the length here.  
+		 */
+		if (cachedLength == Double.POSITIVE_INFINITY || cachedLength == Double.NEGATIVE_INFINITY) {
+			cachedLength = 0;
+		}
 		cachedBounds.update( bounds );
 	}
 	
diff --git a/core/test/net/sf/openrocket/rocketcomponent/FlightConfigurationTest.java b/core/test/net/sf/openrocket/rocketcomponent/FlightConfigurationTest.java
index fe11df0dc..ebec46523 100644
--- a/core/test/net/sf/openrocket/rocketcomponent/FlightConfigurationTest.java
+++ b/core/test/net/sf/openrocket/rocketcomponent/FlightConfigurationTest.java
@@ -44,7 +44,7 @@ public class FlightConfigurationTest extends BaseTestCase {
 		// preconditions
 		assertThat("active stage count doesn't match", config.getActiveStageCount(), equalTo(2));
 
-		final double expectedLength = 0.33;
+		final double expectedLength = 0.335;
 		final double calculatedLength = config.getLength();
 		assertEquals("source config length doesn't match: ", expectedLength, calculatedLength, EPSILON);
 
@@ -70,7 +70,7 @@ public class FlightConfigurationTest extends BaseTestCase {
 		int expectedMotorCount = 2;
 		int actualMotorCount = config1.getActiveMotors().size();
 		assertThat("active motor count doesn't match", actualMotorCount, equalTo(expectedMotorCount));
-		double expectedLength = 0.33;
+		double expectedLength = 0.335;
 		assertEquals("source config length doesn't match: ", expectedLength, config1.getLength(), EPSILON);
 		double expectedReferenceLength = 0.024;
 		assertEquals("source config reference length doesn't match: ", expectedReferenceLength, config1.getReferenceLength(), EPSILON);