SBIN2010 commented on code in PR #42151:
URL: https://github.com/apache/superset/pull/42151#discussion_r3609044850


##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -76,120 +73,153 @@ export function parseParams({
   return [name, formattedValue, formattedPercent];
 }
 
-const HALF_DONUT_SWEEP_LIMIT = 180;
-
 /**
- * Geometric configuration for each type of semi-circular layout.
- *
- * - `centerOffset` — offset of the chart center from the baseline 50% on the 
X and Y axes.
- *                    Resulting position: `50% + offset`.
- * - `totalBase`    — base position of the "Total" text as a percentage on the 
X and Y axes.
- *
- * The values are empirically tuned so that the "Total" text visually remains
- * at the geometric center of the arc after the chart is re-centered. The
- * `left`/`right` totalBase values sit 5% inside the shifted chart center
- * (60% and 40% respectively) to compensate for the text being positioned by
- * its left edge rather than its midpoint.
+ * Bounding box of the pie arc in unit coordinates: outer radius = 1,
+ * mathematical y-up convention matching ECharts' angle convention
+ * (0° points right, 90° points up, angles sweep clockwise from
+ * `startAngle` to `startAngle - sweptAngle`).
  */
-const HALF_DONUT_LAYOUT: Record<
-  HalfDonut,
-  {
-    centerOffset: { x: number; y: number };
-    totalBase: { left: number; top: number };
-  }
-> = {
-  top: { centerOffset: { x: 0, y: 20 }, totalBase: { left: 50, top: 68.5 } },
-  bottom: { centerOffset: { x: 0, y: -20 }, totalBase: { left: 50, top: 30 } },
-  left: { centerOffset: { x: 10, y: 0 }, totalBase: { left: 55, top: 50 } },
-  right: { centerOffset: { x: -10, y: 0 }, totalBase: { left: 35, top: 50 } },
-  none: { centerOffset: { x: 0, y: 0 }, totalBase: { left: 50, top: 50 } },
-};
+export interface ArcBoundingBox {

Review Comment:
   It’s probably better to move this to `types`.



##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -76,120 +73,153 @@ export function parseParams({
   return [name, formattedValue, formattedPercent];
 }
 
-const HALF_DONUT_SWEEP_LIMIT = 180;
-
 /**
- * Geometric configuration for each type of semi-circular layout.
- *
- * - `centerOffset` — offset of the chart center from the baseline 50% on the 
X and Y axes.
- *                    Resulting position: `50% + offset`.
- * - `totalBase`    — base position of the "Total" text as a percentage on the 
X and Y axes.
- *
- * The values are empirically tuned so that the "Total" text visually remains
- * at the geometric center of the arc after the chart is re-centered. The
- * `left`/`right` totalBase values sit 5% inside the shifted chart center
- * (60% and 40% respectively) to compensate for the text being positioned by
- * its left edge rather than its midpoint.
+ * Bounding box of the pie arc in unit coordinates: outer radius = 1,
+ * mathematical y-up convention matching ECharts' angle convention
+ * (0° points right, 90° points up, angles sweep clockwise from
+ * `startAngle` to `startAngle - sweptAngle`).
  */
-const HALF_DONUT_LAYOUT: Record<
-  HalfDonut,
-  {
-    centerOffset: { x: number; y: number };
-    totalBase: { left: number; top: number };
-  }
-> = {
-  top: { centerOffset: { x: 0, y: 20 }, totalBase: { left: 50, top: 68.5 } },
-  bottom: { centerOffset: { x: 0, y: -20 }, totalBase: { left: 50, top: 30 } },
-  left: { centerOffset: { x: 10, y: 0 }, totalBase: { left: 55, top: 50 } },
-  right: { centerOffset: { x: -10, y: 0 }, totalBase: { left: 35, top: 50 } },
-  none: { centerOffset: { x: 0, y: 0 }, totalBase: { left: 50, top: 50 } },
-};
+export interface ArcBoundingBox {
+  minX: number;
+  maxX: number;
+  minY: number;
+  maxY: number;
+}
 
 /**
- * Determines the type of semicircular layout based on the start angle and 
swept angle.
+ * Computes the bounding box of an annular sector from its angles.
  *
- * All four semicircle orientations are supported:
- * - `'top'`    — the arc is positioned at the top; the chart center shifts 
downwards.
- * - `'bottom'` — the arc is positioned at the bottom; the chart center shifts 
upwards.
- * - `'left'`   — the arc is positioned at the left; the chart center shifts 
right.
- * - `'right'`  — the arc is positioned at the right; the chart center shifts 
left.
+ * The box is spanned by the outer arc endpoints, the inner arc endpoints
+ * (which collapse to the pie origin when `innerRatio` is 0), and every axis
+ * extreme (0°/90°/180°/270°) the arc sweeps through.
  *
- * @param startAngle - The start angle of the arc in degrees (0–360).
- * @param sweptAngle - The swept angle of the arc in degrees (10–360).
- * @returns The type of semicircular layout.
+ * @param startAngle - The start angle of the arc in degrees.
+ * @param sweptAngle - The total angle covered by the arc in degrees.
+ * @param innerRatio - Inner radius as a fraction of the outer radius (0–1).
  */
-export const getHalfDonut = (
+export function getArcBoundingBox(
   startAngle: number,
   sweptAngle: number,
-): HalfDonut => {
-  if (sweptAngle > HALF_DONUT_SWEEP_LIMIT) return 'none';
-
-  const normalized = startAngle % 360;
-
-  if (normalized === 180) return 'top';
-  if (normalized === 0) return 'bottom';
-  if (normalized === 270) return 'left';
-  if (normalized === 90) return 'right';
-
-  return 'none';
-};
+  innerRatio: number,
+): ArcBoundingBox {
+  if (sweptAngle >= 360) {
+    return { minX: -1, maxX: 1, minY: -1, maxY: 1 };
+  }
+  const toRad = (deg: number) => (deg * Math.PI) / 180;
+  const endAngle = startAngle - sweptAngle;
+  const points: [number, number][] = [];
+  [startAngle, endAngle].forEach(angle => {
+    const x = Math.cos(toRad(angle));
+    const y = Math.sin(toRad(angle));
+    points.push([x, y], [innerRatio * x, innerRatio * y]);
+  });
+  for (
+    let axis = Math.ceil(endAngle / 90) * 90;
+    axis <= startAngle;
+    axis += 90
+  ) {
+    points.push([Math.cos(toRad(axis)), Math.sin(toRad(axis))]);
+  }
+  const xs = points.map(([x]) => x);
+  const ys = points.map(([, y]) => y);
+  return {
+    minX: Math.min(...xs),
+    maxX: Math.max(...xs),
+    minY: Math.min(...ys),
+    maxY: Math.max(...ys),
+  };
+}
 
-const getHalfDonutLayout = (startAngle: number, sweptAngle: number) =>
-  HALF_DONUT_LAYOUT[getHalfDonut(startAngle, sweptAngle)];
+/**
+ * Arcs covering only a sliver of the circle would otherwise scale up without
+ * bound; cap the fit scale at the factor a quarter arc reaches naturally.
+ */
+const MAX_RADIUS_SCALE = 2;
+
+export interface PieLayout {

Review Comment:
   And it's better to move this to types.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to