DGP - changes to compute fin tab dimensions when the centering rings are children of a subassembly such as a motor mount, rather than parented directly to the outer body tube; tweaks to algorithm for additional edge cases.

src/net/sf/openrocket/gui/configdialog/FinSetConfig.java

@@ -1,19 +1,5 @@
 package net.sf.openrocket.gui.configdialog;
 
-import java.awt.event.ActionEvent;
-import java.awt.event.ActionListener;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.List;
-
-import javax.swing.JButton;
-import javax.swing.JComboBox;
-import javax.swing.JLabel;
-import javax.swing.JPanel;
-import javax.swing.JSpinner;
-import javax.swing.SwingUtilities;
-
 import net.miginfocom.swing.MigLayout;
 import net.sf.openrocket.document.OpenRocketDocument;
 import net.sf.openrocket.gui.SpinnerEditor;
@@ -34,6 +20,15 @@ import net.sf.openrocket.rocketcomponent.RocketComponent;
 import net.sf.openrocket.startup.Application;
 import net.sf.openrocket.unit.UnitGroup;
 
+import javax.swing.*;
+import java.awt.event.ActionEvent;
+import java.awt.event.ActionListener;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.List;
+
 
 public abstract class FinSetConfig extends RocketComponentConfig {
 	private static final LogHelper log = Application.getLogger();
@@ -226,11 +221,11 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 					try {
 						document.startUndo("Compute fin tabs");
 						
-						List<RocketComponent> children = parent.getChildren();
 						List<CenteringRing> rings = new ArrayList<CenteringRing>();
-						
+                        //Do deep recursive iteration
-						for (int i = 0; i < children.size(); i++) {
+                        Iterator<RocketComponent> iter = parent.iterator(false);
-							RocketComponent rocketComponent = children.get(i);
+                        while (iter.hasNext()) {
+                            RocketComponent rocketComponent =  iter.next();
 							if (rocketComponent instanceof InnerTube) {
 								InnerTube it = (InnerTube) rocketComponent;
 								if (it.isMotorMount()) {
@@ -249,8 +244,8 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 						if (!rings.isEmpty()) {
 							FinSet.TabRelativePosition temp = (FinSet.TabRelativePosition) em.getSelectedItem();
 							em.setSelectedItem(FinSet.TabRelativePosition.FRONT);
-							double len = computeFinTabLength(rings, component.asPositionValue(RocketComponent.Position.TOP),
+							double len = computeFinTabLength(rings, component.asPositionValue(RocketComponent.Position.TOP, parent),
-										component.getLength(), mts);
+										component.getLength(), mts, parent);
 							mtl.setValue(len);
 							//Be nice to the user and set the tab relative position enum back the way they had it.
 							em.setSelectedItem(temp);
@@ -281,9 +276,12 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 	 * @param finPositionFromTop the position from the top of the parent of the start of the fin set root
 	 * @param finLength          the length of the root chord
 	 * @param mts                the model for the tab shift (position); the model's value is modified as a result of this method call
+     * @param relativeTo         the parent component of the finset
+     * 
 	 * @return the length of the fin tab
 	 */
-	private static double computeFinTabLength(List<CenteringRing> rings, Double finPositionFromTop, Double finLength, DoubleModel mts) {
+	private static double computeFinTabLength(List<CenteringRing> rings, Double finPositionFromTop, Double finLength, DoubleModel mts,
+                                              final RocketComponent relativeTo) {
 		List<SortableRing> positionsFromTop = new ArrayList<SortableRing>();
 		
 		//Fin tabs will be computed between the last two rings that meet the criteria, represented by top and bottom here.
@@ -295,8 +293,8 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 			Collections.sort(rings, new Comparator<CenteringRing>() {
 				@Override
 				public int compare(CenteringRing centeringRing, CenteringRing centeringRing1) {
-					return (int) (1000d * (centeringRing.asPositionValue(RocketComponent.Position.TOP) -
+					return (int) (1000d * (centeringRing.asPositionValue(RocketComponent.Position.TOP, relativeTo) -
-							centeringRing1.asPositionValue(RocketComponent.Position.TOP)));
+							centeringRing1.asPositionValue(RocketComponent.Position.TOP, relativeTo)));
 						}
 			});
 			
@@ -304,11 +302,12 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 				CenteringRing centeringRing = rings.get(i);
 				//Handle centering rings that overlap or are adjacent by synthetically merging them into one virtual ring.
 				if (!positionsFromTop.isEmpty() &&
-						positionsFromTop.get(positionsFromTop.size() - 1).bottomSidePositionFromTop() >= centeringRing.asPositionValue(RocketComponent.Position.TOP)) {
+						positionsFromTop.get(positionsFromTop.size() - 1).bottomSidePositionFromTop() >=
+                                centeringRing.asPositionValue(RocketComponent.Position.TOP, relativeTo)) {
 					SortableRing adjacent = positionsFromTop.get(positionsFromTop.size() - 1);
-					adjacent.merge(centeringRing);
+					adjacent.merge(centeringRing, relativeTo);
 				} else {
-					positionsFromTop.add(new SortableRing(centeringRing));
+					positionsFromTop.add(new SortableRing(centeringRing, relativeTo));
 				}
 			}
 			
@@ -342,35 +341,62 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 			}
 		}
 
+        double resultFinTabLength = 0d;
+
 		// Edge case where there are no centering rings or for some odd reason top and bottom are identical.
 		if (top == null || top == bottom) {
 			mts.setValue(0);
-			return finLength;
+			resultFinTabLength = finLength;
-		}
+		} else if (bottom == null) {
-		
-		if (bottom == null) {
 			// If there is no bottom ring and the top ring's bottom edge is within the span of the root chord, then
 			// set the position of the fin tab starting at the bottom side of the top ring.
 			if (top.bottomSidePositionFromTop() >= finPositionFromTop) {
 				mts.setValue(top.bottomSidePositionFromTop() - finPositionFromTop);
-				return (finPositionFromTop + finLength - top.bottomSidePositionFromTop());
+				resultFinTabLength = (finPositionFromTop + finLength - top.bottomSidePositionFromTop());
 			} else {
-				// Otherwise the top ring is outside the span of the root chord so set the tab length to be the entire
-				// root chord.
 				mts.setValue(0);
-				return finLength;
+                double diffLen = top.positionFromTop() - finPositionFromTop;
-			}
+                if (diffLen < 0) {
+                // Otherwise the top ring is outside the span of the root chord so set the tab length to be the entire
+                // root chord.
+                    resultFinTabLength = finLength;
+                }
+                else {
+                    // Otherwise there is one ring within the span. Return the length from the start of the fin to the top
+                    // side of the ring.
+                    resultFinTabLength = diffLen;
+                }
+            }
 		}
 		// If the bottom edge of the top centering ring is above the start of the fin's root chord, then make the
 		// fin tab align with the start of the root chord.
-		if (top.bottomSidePositionFromTop() < finPositionFromTop) {
+		else if (top.bottomSidePositionFromTop() < finPositionFromTop) {
 			mts.setValue(0);
-			return bottom.positionFromTop - finPositionFromTop;
+
+            double lenToBottomRing = bottom.positionFromTop - finPositionFromTop;
+            // If the bottom ring lies farther back (down) than the trailing edge of the fin, then the tab should
+            // only be as long as the fin.
+            if (lenToBottomRing > finLength) {
+                resultFinTabLength = finLength;
+            }
+            else {
+                resultFinTabLength = lenToBottomRing;
+            }
 		} else {
-			// Otherwise the rings are within the span of the root chord.  Place the tab between them.
 			mts.setValue(top.bottomSidePositionFromTop() - finPositionFromTop);
-			return (bottom.positionFromTop() - top.bottomSidePositionFromTop());
+            // The bottom ring is beyond the trailing edge of the fin.
-		}
+            if (bottom.positionFromTop() > finLength + finPositionFromTop) {
+                resultFinTabLength = (finLength + finPositionFromTop - top.bottomSidePositionFromTop());
+            }
+			// The rings are within the span of the root chord.  Place the tab between them.
+            else {
+			    resultFinTabLength = (bottom.positionFromTop() - top.bottomSidePositionFromTop());
+            }
+        }
+        if (resultFinTabLength < 0) {
+            resultFinTabLength = 0d;
+        }
+        return resultFinTabLength;
 	}
 	
 	@Override
@@ -400,9 +426,9 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 		 *
 		 * @param r the source centering ring
 		 */
-		SortableRing(CenteringRing r) {
+		SortableRing(CenteringRing r, RocketComponent relativeTo) {
 			thickness = r.getLength();
-			positionFromTop = r.asPositionValue(RocketComponent.Position.TOP);
+			positionFromTop = r.asPositionValue(RocketComponent.Position.TOP, relativeTo);
 		}
 		
 		/**
@@ -410,8 +436,8 @@ public abstract class FinSetConfig extends RocketComponentConfig {
 		 *
 		 * @param adjacent the adjacent ring
 		 */
-		public void merge(CenteringRing adjacent) {
+		public void merge(CenteringRing adjacent, RocketComponent relativeTo) {
-			double v = adjacent.asPositionValue(RocketComponent.Position.TOP);
+			double v = adjacent.asPositionValue(RocketComponent.Position.TOP, relativeTo);
 			if (positionFromTop < v) {
 				thickness = (v + adjacent.getLength()) - positionFromTop;
 			} else {

src/net/sf/openrocket/rocketcomponent/RocketComponent.java

@@ -842,19 +842,17 @@ public abstract class RocketComponent implements ChangeSource, Cloneable, Iterab
 
     /**
      * Determine position relative to given position argument.  Note: This is a side-effect free method.  No state
-     * is modified.  It's exactly like setRelativePosition without the 'set'.
+     * is modified.
      *
      * @param thePosition the relative position to be used as the basis for the computation
+     * @param relativeTo  the position is computed relative the the given component
      *
      * @return double position of the component relative to the parent, with respect to <code>position</code>
      */
-    public double asPositionValue (Position thePosition) {
+    public double asPositionValue (Position thePosition, RocketComponent relativeTo) {
-        if (this.relativePosition == thePosition) {
-            return this.position;
-        }
         double result = this.position;
-        if (this.parent != null) {
+        if (relativeTo != null) {
-            double thisPos = this.toRelative(Coordinate.NUL, this.parent)[0].x;
+            double thisPos = this.toRelative(Coordinate.NUL, relativeTo)[0].x;
 
             switch (thePosition) {
             case ABSOLUTE:
@@ -864,10 +862,10 @@ public abstract class RocketComponent implements ChangeSource, Cloneable, Iterab
                 result = thisPos;
                 break;
             case MIDDLE:
-                result = thisPos - (this.parent.length - this.length) / 2;
+                result = thisPos - (relativeTo.length - this.length) / 2;
                 break;
             case BOTTOM:
-                result = thisPos - (this.parent.length - this.length);
+                result = thisPos - (relativeTo.length - this.length);
                 break;
             default:
                 throw new BugException("Unknown position type: " + thePosition);

test/net/sf/openrocket/gui/configdialog/FinSetConfigTest.java

@@ -1,7 +1,9 @@
 package net.sf.openrocket.gui.configdialog;
 
 import net.sf.openrocket.gui.adaptors.DoubleModel;
+import net.sf.openrocket.rocketcomponent.BodyTube;
 import net.sf.openrocket.rocketcomponent.CenteringRing;
+import net.sf.openrocket.rocketcomponent.RocketComponent;
 import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -16,7 +18,7 @@ public class FinSetConfigTest {
 
     @BeforeClass
     public static void classSetup() throws Exception {
-        method = FinSetConfig.class.getDeclaredMethod("computeFinTabLength", List.class, Double.class, Double.class, DoubleModel.class);
+        method = FinSetConfig.class.getDeclaredMethod("computeFinTabLength", List.class, Double.class, Double.class, DoubleModel.class, RocketComponent.class);
         Assert.assertNotNull(method);
         method.setAccessible(true);
     }
@@ -31,9 +33,11 @@ public class FinSetConfigTest {
         DoubleModel dm = new DoubleModel(1d);
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
-        Double result = (Double)method.invoke(null, rings, 10d, 11d, dm);
+        RocketComponent parent = new BodyTube();
+
+        Double result = (Double)method.invoke(null, rings, 10d, 11d, dm, parent);
         Assert.assertEquals(0.0001, 11d, result.doubleValue());
-        result = (Double)method.invoke(null, null, 10d, 11d, dm);
+        result = (Double)method.invoke(null, null, 10d, 11d, dm, parent);
         Assert.assertEquals(11d, result.doubleValue(), 0.0001);
     }
 
@@ -45,16 +49,22 @@ public class FinSetConfigTest {
         DoubleModel dm = new DoubleModel(1d);
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
+        RocketComponent parent = new BodyTube();
+
         CenteringRing ring1 = new CenteringRing();
         ring1.setLength(0.004);
+        ring1.setRelativePosition(RocketComponent.Position.TOP);
         ring1.setPositionValue(0.43);
         CenteringRing ring2 = new CenteringRing();
         ring2.setLength(0.004);
+        ring2.setRelativePosition(RocketComponent.Position.TOP);
         ring2.setPositionValue(0.45);
         rings.add(ring1);
         rings.add(ring2);
+        parent.addChild(ring1);
+        parent.addChild(ring2);
 
-        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm);
+        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm, parent);
         Assert.assertEquals(0.01, result.doubleValue(), 0.0001);
         
     }
@@ -69,10 +79,14 @@ public class FinSetConfigTest {
 
         CenteringRing ring1 = new CenteringRing();
         ring1.setLength(0.004);
+        ring1.setRelativePosition(RocketComponent.Position.TOP);
         ring1.setPositionValue(0.43);
         rings.add(ring1);
 
-        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm);
+        RocketComponent parent = new BodyTube();
+        parent.addChild(ring1);
+
+        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm, parent);
         Assert.assertEquals(0.01, result.doubleValue(), 0.0001);
     }
 
@@ -85,15 +99,21 @@ public class FinSetConfigTest {
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
         CenteringRing ring1 = new CenteringRing();
+        ring1.setRelativePosition(RocketComponent.Position.TOP);
         ring1.setLength(0.004);
         ring1.setPositionValue(0.43);
         CenteringRing ring2 = new CenteringRing();
+        ring2.setRelativePosition(RocketComponent.Position.TOP);
         ring2.setLength(0.004);
         ring2.setPositionValue(0.45);
         rings.add(ring1);
         rings.add(ring2);
 
-        Double result = (Double)method.invoke(null, rings, 0.45d, 0.01, dm);
+        RocketComponent parent = new BodyTube(1d, 0.01);
+        parent.addChild(ring1);
+        parent.addChild(ring2);
+
+        Double result = (Double)method.invoke(null, rings, 0.45d, 0.01, dm, parent);
         Assert.assertEquals(0.01 - 0.004, result.doubleValue(), 0.0001);
     }
 
@@ -106,15 +126,18 @@ public class FinSetConfigTest {
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
         CenteringRing ring1 = new CenteringRing();
+        ring1.setRelativePosition(RocketComponent.Position.TOP);
         ring1.setLength(0.004);
         ring1.setPositionValue(0.43);
         CenteringRing ring2 = new CenteringRing();
+        ring2.setRelativePosition(RocketComponent.Position.TOP);
         ring2.setLength(0.004);
         ring2.setPositionValue(0.48);
         rings.add(ring1);
         rings.add(ring2);
 
-        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm);
+        RocketComponent parent = new BodyTube();
+        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm, ring1);
         Assert.assertEquals(0.01, result.doubleValue(), 0.0001);
     }
 
@@ -127,15 +150,19 @@ public class FinSetConfigTest {
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
         CenteringRing ring1 = new CenteringRing();
+        ring1.setRelativePosition(RocketComponent.Position.TOP);
         ring1.setLength(0.004);
         ring1.setPositionValue(0.4701);
         CenteringRing ring2 = new CenteringRing();
         ring2.setLength(0.004);
+        ring2.setRelativePosition(RocketComponent.Position.TOP);
         ring2.setPositionValue(0.48);
         rings.add(ring1);
         rings.add(ring2);
-
+        RocketComponent parent = new BodyTube(1.0d, 0.1d);
-        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm);
+        parent.addChild(ring1);
+        parent.addChild(ring2);
+        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm, parent);
         Assert.assertEquals(0.0059, result.doubleValue(), 0.0001);
     }
     
@@ -147,17 +174,22 @@ public class FinSetConfigTest {
         DoubleModel dm = new DoubleModel(1d);
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
+        RocketComponent parent = new BodyTube(1.0000d, 0.1d);
         CenteringRing ring1 = new CenteringRing();
+        ring1.setRelativePosition(RocketComponent.Position.TOP);
         ring1.setLength(0.004);
         ring1.setPositionValue(0.4701);
+        parent.addChild(ring1);
         CenteringRing ring2 = new CenteringRing();
         ring2.setLength(0.004);
+        ring2.setRelativePosition(RocketComponent.Position.TOP);
         ring2.setPositionValue(0.4750);
+        parent.addChild(ring2);
         rings.add(ring1);
         rings.add(ring2);
 
-        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm);
+        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm, parent);
-        Assert.assertEquals(0.0009, result.doubleValue(), 0.0001);
+        Assert.assertEquals(0.0009, result.doubleValue(), 0.0002);
     }
 
 
@@ -170,16 +202,21 @@ public class FinSetConfigTest {
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
         CenteringRing ring1 = new CenteringRing();
+        ring1.setRelativePosition(RocketComponent.Position.TOP);
         ring1.setLength(0.004);
         ring1.setPositionValue(0.48);
         CenteringRing ring2 = new CenteringRing();
+        ring2.setRelativePosition(RocketComponent.Position.TOP);
         ring2.setLength(0.004);
         ring2.setPositionValue(0.49);
         rings.add(ring1);
         rings.add(ring2);
+        RocketComponent parent = new BodyTube(1.0d, 0.1d);
+        parent.addChild(ring1);
+        parent.addChild(ring2);
 
-        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm);
+        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm, parent);
-        Assert.assertEquals(0.006, result.doubleValue(), 0.0001);
+        Assert.assertEquals(0.0, result.doubleValue(), 0.0001);
     }
 
     /**
@@ -191,19 +228,27 @@ public class FinSetConfigTest {
         List<CenteringRing> rings = new ArrayList<CenteringRing>();
 
         CenteringRing ring1 = new CenteringRing();
+        ring1.setRelativePosition(RocketComponent.Position.ABSOLUTE);
         ring1.setLength(0.004);
         ring1.setPositionValue(0.47);
         CenteringRing ring2 = new CenteringRing();
+        ring2.setRelativePosition(RocketComponent.Position.ABSOLUTE);
         ring2.setLength(0.004);
         ring2.setPositionValue(0.4702);
         CenteringRing ring3 = new CenteringRing();
+        ring3.setRelativePosition(RocketComponent.Position.ABSOLUTE);
         ring3.setLength(0.004);
         ring3.setPositionValue(0.4770);
         rings.add(ring1);
         rings.add(ring2);
         rings.add(ring3);
+        BodyTube parent = new BodyTube(1.0d, 0.1d);
+        parent.setPositionValue(0);
+        parent.addChild(ring1);
+        parent.addChild(ring2);
+        parent.addChild(ring3);
 
-        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm);
+        Double result = (Double)method.invoke(null, rings, 0.47d, 0.01, dm, parent);
         Assert.assertEquals(0.0028, result.doubleValue(), 0.0001);
     }