fix: use expression to generalize zeroOrMinOrMax only when there is a…

… scale
vega · May 12, 2024 · 9b8e62e · 9b8e62e
1 parent 24f9979
commit 9b8e62e
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 18 deletions.
diff --git a/src/compile/mark/encode/zeroOrMinOrMax.ts b/src/compile/mark/encode/zeroOrMinOrMax.ts
@@ -13,11 +13,30 @@ export function zeroOrMinOrMax({
   mode: 'min' | 'zeroOrMin' | {zeroOrMax: {widthSignal: string; heightSignal: string}};
   mainChannel: PositionScaleChannel | PolarPositionScaleChannel | NonPositionScaleChannel;
 }): VgValueRef {
-  if (mode !== 'min' && scaleName && scale.domainDefinitelyIncludesZero()) {
-    return {
-      scale: scaleName,
-      value: 0
-    };
+  const domain = `domain('${scaleName}')`;
+  const min = `${domain}[0]`;
+  const max = `peek(${domain})`; // peek = the last item of the array
+
+  if (scale && scaleName) {
+    // If there is a scale (and hence its name)
+    const domainHasZero = scale.domainHasZero();
+    if (mode === 'min') {
+      return {signal: `scale('${scaleName}', ${min})`}; // encode the scale domain min
+    } else {
+      // zeroOrMin or zeroOrMax mode
+      if (domainHasZero === 'definitely') {
+        return {
+          scale: scaleName,
+          value: 0
+        };
+      } else if (domainHasZero === 'maybe') {
+        if (mode === 'zeroOrMin') {
+          return {signal: `scale('${scaleName}', inrange(0, ${domain}) ? 0 : ${min})`}; // encode the scale domain min
+        } else {
+          return {signal: `scale('${scaleName}', inrange(0, ${domain}) ? 0 : ${max})`}; // encode the scale domain max
+        }
+      }
+    }
   }
 
   if (mode === 'zeroOrMin' || mode === 'min') {
@@ -29,7 +48,7 @@ export function zeroOrMinOrMax({
       case 'y':
         return {field: {group: 'height'}};
       default:
-        return {signal: `scale('${scaleName}', domain('${scaleName}')[0])`}; // encode the scale domain min
+        return undefined; // For non-position, it's impossible to reach this line if the field doesn't have a scale.
     }
   } else {
     // zeroOrMax
@@ -48,7 +67,7 @@ export function zeroOrMinOrMax({
       case 'y':
         return {value: 0};
       default:
-        return {signal: `scale('${scaleName}', domain('${scaleName}')[1])`}; // encode the scale domain max
+        return undefined; // For non-position, it's impossible to reach this line if the field doesn't have a scale.
     }
   }
 }
diff --git a/src/compile/scale/component.ts b/src/compile/scale/component.ts
@@ -31,23 +31,37 @@ export class ScaleComponent extends Split<ScaleComponentProps> {
   }
 
   /**
-   * Whether the scale definitely includes zero in the domain
+   * Whether the scale definitely includes or not include zero in the domain
    */
-  public domainDefinitelyIncludesZero() {
-    if (contains([ScaleType.LOG, ScaleType.TIME, ScaleType.UTC], this.get('type'))) {
+  public domainHasZero(): 'definitely' | 'definitely-not' | 'maybe' {
+    const scaleType = this.get('type');
+    if (contains([ScaleType.LOG, ScaleType.TIME, ScaleType.UTC], scaleType)) {
       // Log scales cannot have zero.
       // Zero in time scale is arbitrary, and does not affect ratio.
       // (Time is an interval level of measurement, not ratio).
       // See https://en.wikipedia.org/wiki/Level_of_measurement for more info.
-      return false;
+      return 'definitely-not';
     }
-    if (this.get('zero') !== false) {
-      return true;
+
+    const scaleZero = this.get('zero');
+    if (
+      scaleZero === true ||
+      // If zero is undefined, linear/sqrt/pow scales have zero by default.
+      (scaleType === undefined && contains([ScaleType.LINEAR, ScaleType.SQRT, ScaleType.POW], scaleType))
+    ) {
+      return 'definitely';
     }
-    return some(
-      this.get('domains'),
-      d => isArray(d) && d.length === 2 && isNumber(d[0]) && d[0] <= 0 && isNumber(d[1]) && d[1] >= 0
-    );
+
+    const domains = this.get('domains');
+
+    if (domains.length > 0) {
+      const hasDomainWithZero = some(
+        domains,
+        d => isArray(d) && d.length === 2 && isNumber(d[0]) && d[0] <= 0 && isNumber(d[1]) && d[1] >= 0
+      );
+      return hasDomainWithZero ? 'definitely' : 'definitely-not';
+    }
+    return 'maybe';
   }
 }
 

diff --git a/test/compile/mark/point.test.ts b/test/compile/mark/point.test.ts
@@ -238,7 +238,7 @@ describe('Mark: Point', () => {
 
     it('should test for invalid values on y', () => {
       expect(props.y).toEqual([
-        {field: {group: 'height'}, test: '!isValid(datum["yield"]) || !isFinite(+datum["yield"])'},
+        {scale: 'y', value: 0, test: '!isValid(datum["yield"]) || !isFinite(+datum["yield"])'},
         {scale: Y, field: 'yield'}
       ]);
     });