Fix resizing an instance with aspectRatio activated to 0, causing it to break (#9163)

newIDE/app/src/InstancesEditor/CompactInstancePropertiesEditor/CompactInstancePropertiesSchema.js

@@ -50,7 +50,67 @@ const applyRatio = ({
   newReferenceValue: number,
   valueToApplyTo: number,
 |}) => {
-  return (newReferenceValue / oldReferenceValue) * valueToApplyTo;
+  // Rounding to 12 significant digits removes floating point errors
+  // (e.g. 104.01999999999998 instead of 104.02) that would otherwise
+  // accumulate when the size is changed step by step.
+  return Number(
+    ((newReferenceValue * valueToApplyTo) / oldReferenceValue).toPrecision(12)
+  );
+};
+
+type Size = [number, number, number];
+
+// Size of instances before it was set to 0 while keeping the ratio, to restore
+// their proportions when the size is increased again.
+const sizesBeforeCollapse: Map<number, Size> = new Map();
+
+const setInstanceSize = ({
+  instance,
+  dimension,
+  newValue,
+  getInstanceSize,
+  getInstanceDefaultSize,
+}: {|
+  instance: gdInitialInstance,
+  dimension: 0 | 1 | 2,
+  newValue: number,
+  getInstanceSize: gdInitialInstance => Size,
+  getInstanceDefaultSize: gdInitialInstance => Size,
+|}) => {
+  const shouldKeepRatio = instance.shouldKeepRatio();
+  const currentSize = getInstanceSize(instance);
+  const newDimensionValue = Math.max(newValue, 0);
+  const isCollapsed = currentSize.every(value => value === 0);
+  const referenceSize =
+    shouldKeepRatio && isCollapsed
+      ? sizesBeforeCollapse.get(instance.ptr) ||
+        getInstanceDefaultSize(instance)
+      : currentSize;
+  const referenceValue = referenceSize[dimension];
+  const newSize = referenceSize.map((value, index) =>
+    index === dimension
+      ? newDimensionValue
+      : !shouldKeepRatio || referenceValue === 0
+      ? currentSize[index]
+      : applyRatio({
+          oldReferenceValue: referenceValue,
+          newReferenceValue: newDimensionValue,
+          valueToApplyTo: value,
+        })
+  );
+
+  if (newSize.every(value => value === 0)) {
+    if (!isCollapsed) sizesBeforeCollapse.set(instance.ptr, currentSize);
+  } else {
+    sizesBeforeCollapse.delete(instance.ptr);
+  }
+
+  instance.setCustomWidth(newSize[0]);
+  instance.setCustomHeight(newSize[1]);
+  instance.setCustomDepth(newSize[2]);
+  // This must be done after reading the size.
+  instance.setHasCustomSize(true);
+  instance.setHasCustomDepth(true);
 };
 
 const getEditObjectButton = ({
@@ -253,12 +313,14 @@ const getWidthField = ({
   getInstanceWidth,
   getInstanceHeight,
   getInstanceDepth,
+  getInstanceDefaultSize,
   forceUpdate,
 }: {|
   i18n: I18nType,
   getInstanceWidth: gdInitialInstance => number,
   getInstanceHeight: gdInitialInstance => number,
   getInstanceDepth: gdInitialInstance => number,
+  getInstanceDefaultSize: gdInitialInstance => Size,
   forceUpdate: () => void,
 |}): Field => ({
   name: 'Width',
@@ -266,34 +328,17 @@ const getWidthField = ({
   valueType: 'number',
   getValue: getInstanceWidth,
   setValue: (instance: gdInitialInstance, newValue: number) => {
-    const shouldKeepRatio = instance.shouldKeepRatio();
-    const newWidth = Math.max(newValue, 0);
-    if (shouldKeepRatio) {
-      const initialWidth = getInstanceWidth(instance) || 1;
-      instance.setCustomWidth(newWidth);
-      instance.setCustomHeight(
-        applyRatio({
-          oldReferenceValue: initialWidth,
-          newReferenceValue: newWidth,
-          valueToApplyTo: getInstanceHeight(instance),
-        })
-      );
-      instance.setCustomDepth(
-        applyRatio({
-          oldReferenceValue: initialWidth,
-          newReferenceValue: newWidth,
-          valueToApplyTo: getInstanceDepth(instance),
-        })
-      );
-    } else {
-      instance.setCustomWidth(newWidth);
-      instance.setCustomHeight(getInstanceHeight(instance));
-      instance.setCustomDepth(getInstanceDepth(instance));
-    }
-
-    // This must be done after reading the size.
-    instance.setHasCustomSize(true);
-    instance.setHasCustomDepth(true);
+    setInstanceSize({
+      instance,
+      dimension: 0,
+      newValue,
+      getInstanceSize: instance => [
+        getInstanceWidth(instance),
+        getInstanceHeight(instance),
+        getInstanceDepth(instance),
+      ],
+      getInstanceDefaultSize,
+    });
     forceUpdate();
   },
   renderLeftIcon: className => <LetterW className={className} />,
@@ -314,12 +359,14 @@ const getHeightField = ({
   getInstanceWidth,
   getInstanceHeight,
   getInstanceDepth,
+  getInstanceDefaultSize,
   forceUpdate,
 }: {|
   i18n: I18nType,
   getInstanceWidth: gdInitialInstance => number,
   getInstanceHeight: gdInitialInstance => number,
   getInstanceDepth: gdInitialInstance => number,
+  getInstanceDefaultSize: gdInitialInstance => Size,
   forceUpdate: () => void,
 |}): Field => ({
   name: 'Height',
@@ -327,34 +374,17 @@ const getHeightField = ({
   valueType: 'number',
   getValue: getInstanceHeight,
   setValue: (instance: gdInitialInstance, newValue: number) => {
-    const shouldKeepRatio = instance.shouldKeepRatio();
-    const newHeight = Math.max(newValue, 0);
-    if (shouldKeepRatio) {
-      const initialHeight = getInstanceHeight(instance) || 1;
-      instance.setCustomWidth(
-        applyRatio({
-          oldReferenceValue: initialHeight,
-          newReferenceValue: newHeight,
-          valueToApplyTo: getInstanceWidth(instance),
-        })
-      );
-      instance.setCustomHeight(newHeight);
-      instance.setCustomDepth(
-        applyRatio({
-          oldReferenceValue: initialHeight,
-          newReferenceValue: newHeight,
-          valueToApplyTo: getInstanceDepth(instance),
-        })
-      );
-    } else {
-      instance.setCustomWidth(getInstanceWidth(instance));
-      instance.setCustomHeight(newHeight);
-      instance.setCustomDepth(getInstanceDepth(instance));
-    }
-
-    // This must be done after reading the size.
-    instance.setHasCustomSize(true);
-    instance.setHasCustomDepth(true);
+    setInstanceSize({
+      instance,
+      dimension: 1,
+      newValue,
+      getInstanceSize: instance => [
+        getInstanceWidth(instance),
+        getInstanceHeight(instance),
+        getInstanceDepth(instance),
+      ],
+      getInstanceDefaultSize,
+    });
     forceUpdate();
   },
   renderLeftIcon: className => <LetterH className={className} />,
@@ -375,12 +405,14 @@ const getDepthField = ({
   getInstanceWidth,
   getInstanceHeight,
   getInstanceDepth,
+  getInstanceDefaultSize,
   forceUpdate,
 }: {|
   i18n: I18nType,
   getInstanceWidth: gdInitialInstance => number,
   getInstanceHeight: gdInitialInstance => number,
   getInstanceDepth: gdInitialInstance => number,
+  getInstanceDefaultSize: gdInitialInstance => Size,
   forceUpdate: () => void,
 |}): Field => ({
   name: 'Depth',
@@ -388,34 +420,17 @@ const getDepthField = ({
   valueType: 'number',
   getValue: getInstanceDepth,
   setValue: (instance: gdInitialInstance, newValue: number) => {
-    const shouldKeepRatio = instance.shouldKeepRatio();
-    const newDepth = Math.max(newValue, 0);
-    if (shouldKeepRatio) {
-      const initialDepth = getInstanceDepth(instance) || 1;
-      instance.setCustomWidth(
-        applyRatio({
-          oldReferenceValue: initialDepth,
-          newReferenceValue: newDepth,
-          valueToApplyTo: getInstanceWidth(instance),
-        })
-      );
-      instance.setCustomHeight(
-        applyRatio({
-          oldReferenceValue: initialDepth,
-          newReferenceValue: newDepth,
-          valueToApplyTo: getInstanceHeight(instance),
-        })
-      );
-      instance.setCustomDepth(newDepth);
-    } else {
-      instance.setCustomWidth(getInstanceWidth(instance));
-      instance.setCustomHeight(getInstanceHeight(instance));
-      instance.setCustomDepth(newDepth);
-    }
-
-    // This must be done after reading the size.
-    instance.setHasCustomSize(true);
-    instance.setHasCustomDepth(true);
+    setInstanceSize({
+      instance,
+      dimension: 2,
+      newValue,
+      getInstanceSize: instance => [
+        getInstanceWidth(instance),
+        getInstanceHeight(instance),
+        getInstanceDepth(instance),
+      ],
+      getInstanceDefaultSize,
+    });
     forceUpdate();
   },
   renderLeftIcon: className => <LetterD className={className} />,
@@ -556,6 +571,17 @@ export const makeSchema = ({
       ? instance.getCustomDepth()
       : onGetInstanceSize(instance)[2];
 
+  const getInstanceDefaultSize = (instance: gdInitialInstance): Size => {
+    const hasCustomSize = instance.hasCustomSize();
+    const hasCustomDepth = instance.hasCustomDepth();
+    instance.setHasCustomSize(false);
+    instance.setHasCustomDepth(false);
+    const defaultSize = onGetInstanceSize(instance);
+    instance.setHasCustomSize(hasCustomSize);
+    instance.setHasCustomDepth(hasCustomDepth);
+    return defaultSize;
+  };
+
   if (is3DInstance) {
     // $FlowFixMe[incompatible-type]
     return [
@@ -582,6 +608,7 @@ export const makeSchema = ({
                 getInstanceWidth,
                 getInstanceHeight,
                 getInstanceDepth,
+                getInstanceDefaultSize,
                 forceUpdate,
               }),
               getHeightField({
@@ -589,6 +616,7 @@ export const makeSchema = ({
                 getInstanceWidth,
                 getInstanceHeight,
                 getInstanceDepth,
+                getInstanceDefaultSize,
                 forceUpdate,
               }),
               getDepthField({
@@ -596,6 +624,7 @@ export const makeSchema = ({
                 getInstanceWidth,
                 getInstanceHeight,
                 getInstanceDepth,
+                getInstanceDefaultSize,
                 forceUpdate,
               }),
             ],
@@ -672,6 +701,7 @@ export const makeSchema = ({
               getInstanceWidth,
               getInstanceHeight,
               getInstanceDepth,
+              getInstanceDefaultSize,
               forceUpdate,
             }),
             getHeightField({
@@ -679,6 +709,7 @@ export const makeSchema = ({
               getInstanceWidth,
               getInstanceHeight,
               getInstanceDepth,
+              getInstanceDefaultSize,
               forceUpdate,
             }),
           ],

newIDE/app/src/InstancesEditor/CompactInstancePropertiesEditor/CompactInstancePropertiesSchema.spec.js

@@ -0,0 +1,199 @@
+// @flow
+import { makeSchema } from './CompactInstancePropertiesSchema';
+const gd: libGDevelop = global.gd;
+
+// Avoid importing the whole properties panel (and its renderers).
+jest.mock('./index', () => ({ styles: { icon: {} } }));
+
+const fakeI18n: any = { _: () => '' };
+
+const findField = (schema: any, name: string): any => {
+  for (const field of schema) {
+    if (!field) continue;
+    if (field.name === name) return field;
+    const children = field.children || (field.child ? [field.child] : null);
+    if (children) {
+      const found = findField(children, name);
+      if (found) return found;
+    }
+  }
+  return null;
+};
+
+const makeSizeFields = ({
+  is3DInstance,
+  defaultSize,
+}: {|
+  is3DInstance: boolean,
+  defaultSize: [number, number, number],
+|}) => {
+  const layout = new gd.Layout();
+  const schema = makeSchema({
+    is3DInstance,
+    hasOpacity: true,
+    canBeFlippedXY: true,
+    canBeFlippedZ: true,
+    i18n: fakeI18n,
+    forceUpdate: () => {},
+    onEditObject: () => {},
+    onGetInstanceSize: (instance: gdInitialInstance) => [
+      instance.hasCustomSize() ? instance.getCustomWidth() : defaultSize[0],
+      instance.hasCustomSize() ? instance.getCustomHeight() : defaultSize[1],
+      instance.hasCustomDepth() ? instance.getCustomDepth() : defaultSize[2],
+    ],
+    layersContainer: layout.getLayers(),
+  });
+  return {
+    widthField: findField(schema, 'Width'),
+    heightField: findField(schema, 'Height'),
+    depthField: findField(schema, 'Depth'),
+    layout,
+  };
+};
+
+const getSize = (instance: gdInitialInstance) => [
+  instance.getCustomWidth(),
+  instance.getCustomHeight(),
+  instance.getCustomDepth(),
+];
+
+describe('CompactInstancePropertiesSchema size fields', () => {
+  it('keeps the ratio when a 3D instance is resized to 0 and back', () => {
+    const { widthField, layout } = makeSizeFields({
+      is3DInstance: true,
+      defaultSize: [40, 20, 10],
+    });
+    const instance = new gd.InitialInstance();
+    instance.setShouldKeepRatio(true);
+
+    widthField.setValue(instance, 80);
+    expect(getSize(instance)).toEqual([80, 40, 20]);
+
+    widthField.setValue(instance, 0);
+    expect(getSize(instance)).toEqual([0, 0, 0]);
+
+    widthField.setValue(instance, 8);
+    expect(getSize(instance)).toEqual([8, 4, 2]);
+
+    instance.delete();
+    layout.delete();
+  });
+
+  it('keeps the ratio when a 2D instance is resized to 0 and back with another field', () => {
+    const { widthField, heightField, layout } = makeSizeFields({
+      is3DInstance: false,
+      defaultSize: [40, 20, 0],
+    });
+    const instance = new gd.InitialInstance();
+    instance.setShouldKeepRatio(true);
+
+    heightField.setValue(instance, 0);
+    expect(getSize(instance)).toEqual([0, 0, 0]);
+
+    widthField.setValue(instance, 10);
+    expect(getSize(instance)).toEqual([10, 5, 0]);
+
+    instance.delete();
+    layout.delete();
+  });
+
+  it('uses the object default size ratio when the collapsed size was not remembered', () => {
+    const { depthField, layout } = makeSizeFields({
+      is3DInstance: true,
+      defaultSize: [40, 20, 10],
+    });
+    const instance = new gd.InitialInstance();
+    instance.setShouldKeepRatio(true);
+    // As if the project was saved with a size of 0.
+    instance.setHasCustomSize(true);
+    instance.setHasCustomDepth(true);
+    instance.setCustomWidth(0);
+    instance.setCustomHeight(0);
+    instance.setCustomDepth(0);
+
+    depthField.setValue(instance, 5);
+    expect(getSize(instance)).toEqual([20, 10, 5]);
+
+    instance.delete();
+    layout.delete();
+  });
+
+  it('keeps integer sizes exact when scaling a square step by step', () => {
+    const { widthField, layout } = makeSizeFields({
+      is3DInstance: false,
+      defaultSize: [64, 64, 0],
+    });
+    const instance = new gd.InitialInstance();
+    instance.setShouldKeepRatio(true);
+
+    for (let width = 65; width <= 200; width++) {
+      widthField.setValue(instance, width);
+      expect(getSize(instance)).toEqual([width, width, 0]);
+    }
+
+    instance.delete();
+    layout.delete();
+  });
+
+  it('does not accumulate rounding errors when scaling a decimal cube step by step', () => {
+    const { widthField, layout } = makeSizeFields({
+      is3DInstance: true,
+      defaultSize: [64.02, 64.02, 32.01],
+    });
+    const instance = new gd.InitialInstance();
+    instance.setShouldKeepRatio(true);
+
+    let width = 64.02;
+    for (let i = 0; i < 200; i++) {
+      width = Math.round((width + 1) * 100) / 100;
+      widthField.setValue(instance, width);
+      expect(getSize(instance)).toEqual([
+        width,
+        width,
+        Math.round(width * 50) / 100,
+      ]);
+    }
+
+    instance.delete();
+    layout.delete();
+  });
+
+  it('only changes the edited dimension when the ratio is not kept', () => {
+    const { widthField, heightField, layout } = makeSizeFields({
+      is3DInstance: true,
+      defaultSize: [40, 20, 10],
+    });
+    const instance = new gd.InitialInstance();
+    instance.setShouldKeepRatio(false);
+
+    widthField.setValue(instance, 0);
+    expect(getSize(instance)).toEqual([0, 20, 10]);
+
+    heightField.setValue(instance, 0);
+    widthField.setValue(instance, 15);
+    expect(getSize(instance)).toEqual([15, 0, 10]);
+
+    instance.delete();
+    layout.delete();
+  });
+
+  it('only changes the edited dimension when the others are not 0', () => {
+    const { widthField, layout } = makeSizeFields({
+      is3DInstance: true,
+      defaultSize: [40, 20, 10],
+    });
+    const instance = new gd.InitialInstance();
+    instance.setHasCustomSize(true);
+    instance.setHasCustomDepth(true);
+    instance.setCustomWidth(0);
+    instance.setCustomHeight(30);
+    instance.setCustomDepth(10);
+    instance.setShouldKeepRatio(true);
+
+    widthField.setValue(instance, 5);
+    expect(getSize(instance)).toEqual([5, 30, 10]);
+
+    instance.delete();
+    layout.delete();
+  });
+});