[refactor] Refactored FlightConfiguration.calculateBounds to be clearer

core/src/net/sf/openrocket/rocketcomponent/BoxBounded.java

@@ -0,0 +1,15 @@
+package net.sf.openrocket.rocketcomponent;
+
+import net.sf.openrocket.util.BoundingBox;
+
+public interface BoxBounded {
+
+    /**
+     * Get a bounding box for a single instance of this component, from its own reference point.
+     * This is expected to be compbined with a InstanceContext for bounds in the global / rocket frame.
+     *
+     * @return BoundingBox from the instance's reference point.
+     */
+    BoundingBox getInstanceBoundingBox();
+
+}

core/src/net/sf/openrocket/rocketcomponent/FinSet.java

@@ -25,12 +25,8 @@ import net.sf.openrocket.util.Coordinate;
 import net.sf.openrocket.util.MathUtil;
 import net.sf.openrocket.util.Transformation;
 
-public abstract class FinSet extends ExternalComponent implements RingInstanceable, AxialPositionable {
+public abstract class FinSet extends ExternalComponent implements AxialPositionable, BoxBounded, RingInstanceable {
 	private static final Translator trans = Application.getTranslator();
-	
-	@SuppressWarnings("unused")
-	private static final Logger log = LoggerFactory.getLogger(FinSet.class);
-	
 
 	/**
 	 * Maximum allowed cant of fins.
@@ -735,13 +731,14 @@ public abstract class FinSet extends ExternalComponent implements RingInstanceab
 	}
 	
 
-	public BoundingBox getBoundingBox() {
+	public BoundingBox getInstanceBoundingBox(){
 		BoundingBox singleFinBounds= new BoundingBox().update(getFinPoints());
 		final double finLength = singleFinBounds.max.x;
 		final double finHeight = singleFinBounds.max.y;
 		
 		Coordinate[] locations = getInstanceLocations();
 		double[] angles = getInstanceAngles();
+
 		BoundingBox finSetBox = new BoundingBox();
 		
 		/*

core/src/net/sf/openrocket/rocketcomponent/FlightConfiguration.java

@@ -550,59 +550,77 @@ public class FlightConfiguration implements FlightConfigurableParameter<FlightCo
 	 * in the current configuration.
 	 */
 	private void calculateBounds(){
-		BoundingBox bounds = new BoundingBox();
+		BoundingBox rocketBounds = new BoundingBox();
 
 		InstanceMap map = getActiveInstances();
 		for (Map.Entry<RocketComponent, java.util.ArrayList<InstanceContext>>  entry : map.entrySet()) {
 			RocketComponent component = entry.getKey();
+			BoundingBox componentBounds = new BoundingBox();
 			List<InstanceContext> contexts = entry.getValue();
-						
-			Collection<Coordinate> coordinates = new ArrayList<Coordinate>();
+
 			/* FinSets already provide a bounding box, so let's use that.
 			 */
-			if (component instanceof FinSet) {
-				bounds.update(((FinSet) component).getBoundingBox());
-				continue;
-			} 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 BoxBounded) {
+				for (InstanceContext context : 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 ((component instanceof AxialStage) || (component instanceof BodyTube) ||
-						(component instanceof PodSet)) {
-						tc = c;
-					} else {
-						tc = ctxt.transform.transform(c);
+					if (!context.active) {
+						continue;
+					}
+
+					BoundingBox instanceBounds = ((BoxBounded) component).getInstanceBoundingBox();
+					if(instanceBounds.isEmpty()) {
+						// this component is probably non-physical (like an assembly) or has invalid bounds.  Skip.
+						// i.e. 'break' out of the per-instance-context loop, and let the per-component loop continue
+						break;
+					}
+					componentBounds = instanceBounds.transform(context.transform);
+				}
+			}else if ((component instanceof AxialStage) || (component instanceof BodyTube) || (component instanceof PodSet)) {
+				// Legacy Case #1:
+				// 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.
+				componentBounds.update(component.getComponentBounds());
+			} else {
+				// Legacy Case #2:
+				// These components do not implement
+				Collection<Coordinate> instanceCoordinates = component.getComponentBounds();
+
+				for (InstanceContext context : 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 (!context.active) {
+						continue;
+					}
+
+					Collection<Coordinate> transformedCoords = new ArrayList<>(instanceCoordinates);
+					// mutating.  Transforms coordinates in place.
+					context.transform.transform(instanceCoordinates);
+
+					for (Coordinate tc : transformedCoords) {
+						componentBounds.update(tc);
 					}
-					componentBox.update(tc);
-					transformedCoords.add(tc);
 				}
 			}
-			
-			bounds.update(componentBox);
+
+			rocketBounds.update(componentBounds);
 		}
 		
 		boundsModID = rocket.getModID();
-		cachedLength = bounds.span().x;
+		cachedLength = rocketBounds.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.  
@@ -610,7 +628,7 @@ public class FlightConfiguration implements FlightConfigurableParameter<FlightCo
 		if (getActiveStages().isEmpty()) {
 			cachedLength = 0;
 		}
-		cachedBounds.update( bounds );
+		cachedBounds.update( rocketBounds );
 	}
 	
 	/**

core/src/net/sf/openrocket/util/BoundingBox.java

@@ -7,7 +7,7 @@ import java.util.Collection;
 public class BoundingBox {
 	public Coordinate min;
 	public Coordinate max;
-	
+
 	public BoundingBox() {
 	    clear();
 	}
@@ -27,8 +27,25 @@ public class BoundingBox {
 	public BoundingBox clone() {
 		return new BoundingBox( this.min, this.max );
 	}
-	
-	
+
+	public boolean isEmpty(){
+		if( (min.x > max.x) ||
+			(min.y > max.y) ||
+			(min.z > max.z)){
+			return true;
+		}else{
+			return false;
+		}
+	}
+
+	/**
+	 * return a copy of this bounding box, transformed by the given transformation matrix
+ 	 * @return
+	 */
+	public BoundingBox transform(Transformation transform){
+		return new BoundingBox(transform.transform(this.min), transform.transform(this.max));
+	}
+
 	private void update_x_min( final double xVal) {
 		if( min.x > xVal)
 			min = min.setX( xVal );
@@ -104,6 +121,9 @@ public class BoundingBox {
     }
 	
 	public BoundingBox update( BoundingBox other ) {
+		if(other.isEmpty()){
+			return this;
+		}
 		update_x_min(other.min.x);
 		update_y_min(other.min.y);
 		update_z_min(other.min.y);
@@ -121,7 +141,7 @@ public class BoundingBox {
 	}
 	
 	public Collection<Coordinate> toCollection(){
-		Collection<Coordinate> toReturn = new ArrayList<Coordinate>();
+		Collection<Coordinate> toReturn = new ArrayList<>();
 		toReturn.add( this.max);
 		toReturn.add( this.min);
 		return toReturn;

swing/src/net/sf/openrocket/gui/figure3d/geometry/FinRenderer.java

@@ -19,7 +19,7 @@ public class FinRenderer {
 	
 	public void renderFinSet(final GL2 gl, FinSet finSet ) {
 		
-	    BoundingBox bounds = finSet.getBoundingBox();
+	    BoundingBox bounds = finSet.getInstanceBoundingBox();
 		gl.glMatrixMode(GL.GL_TEXTURE);
 		gl.glPushMatrix();
 		gl.glScaled(1 / (bounds.max.x - bounds.min.x), 1 / (bounds.max.y - bounds.min.y), 0);