From c04bdea865640ef2a46958047b8505d9c8f3e97a Mon Sep 17 00:00:00 2001 From: Kanit Wongsuphasawat Date: Wed, 11 Oct 2023 18:03:40 -0700 Subject: [PATCH] fix: support x/yOffset without x/y --- src/compile/mark/encode/position-rect.ts | 13 ++++- src/compile/scale/parse.ts | 14 +----- src/compile/scale/range.ts | 50 +++++++++++++------ src/stack.ts | 16 +++--- .../compile/mark/encode/position-rect.test.ts | 27 ++++++++++ test/compile/scale/range.test.ts | 42 ++++++++++++++++ test/stack.test.ts | 37 +++++++++++++- 7 files changed, 161 insertions(+), 38 deletions(-) diff --git a/src/compile/mark/encode/position-rect.ts b/src/compile/mark/encode/position-rect.ts index afbd417d48..b2229b1433 100644 --- a/src/compile/mark/encode/position-rect.ts +++ b/src/compile/mark/encode/position-rect.ts @@ -166,7 +166,14 @@ function positionAndSize( const hasSizeFromMarkOrEncoding = !!sizeMixins; // Otherwise, apply default value - const bandSize = getBandSize({channel, fieldDef, markDef, config, scaleType: scale?.get('type'), useVlSizeChannel}); + const bandSize = getBandSize({ + channel, + fieldDef, + markDef, + config, + scaleType: (scale || offsetScale)?.get('type'), + useVlSizeChannel + }); sizeMixins = sizeMixins || { [vgSizeChannel]: defaultSizeRef( @@ -190,7 +197,9 @@ function positionAndSize( */ const defaultBandAlign = - scale?.get('type') === 'band' && isRelativeBandSize(bandSize) && !hasSizeFromMarkOrEncoding ? 'top' : 'middle'; + (scale || offsetScale)?.get('type') === 'band' && isRelativeBandSize(bandSize) && !hasSizeFromMarkOrEncoding + ? 'top' + : 'middle'; const vgChannel = vgAlignedPositionChannel(channel, markDef, config, defaultBandAlign); const center = vgChannel === 'xc' || vgChannel === 'yc'; diff --git a/src/compile/scale/parse.ts b/src/compile/scale/parse.ts index a2334bcf7a..e6baf799af 100644 --- a/src/compile/scale/parse.ts +++ b/src/compile/scale/parse.ts @@ -1,7 +1,6 @@ -import {getMainChannelFromOffsetChannel, isXorYOffset, ScaleChannel, SCALE_CHANNELS, SHAPE} from '../../channel'; +import {ScaleChannel, SCALE_CHANNELS, SHAPE} from '../../channel'; import {getFieldOrDatumDef, ScaleDatumDef, TypedFieldDef} from '../../channeldef'; import {channelHasNestedOffsetScale} from '../../encoding'; -import * as log from '../../log'; import {GEOSHAPE} from '../../mark'; import { NON_TYPE_DOMAIN_RANGE_VEGA_SCALE_PROPERTIES, @@ -56,17 +55,6 @@ function parseUnitScaleCore(model: UnitModel): ScaleComponentIndex { } let specifiedScale = fieldOrDatumDef && fieldOrDatumDef['scale']; - if (isXorYOffset(channel)) { - const mainChannel = getMainChannelFromOffsetChannel(channel); - if (!channelHasNestedOffsetScale(encoding, mainChannel)) { - // Don't generate scale when the offset encoding shouldn't yield a nested scale - if (specifiedScale) { - log.warn(log.message.offsetEncodingScaleIgnored(channel)); - } - continue; - } - } - if (fieldOrDatumDef && specifiedScale !== null && specifiedScale !== false) { specifiedScale ??= {}; const hasNestedOffsetScale = channelHasNestedOffsetScale(encoding, channel); diff --git a/src/compile/scale/range.ts b/src/compile/scale/range.ts index a25d382470..e114d5625e 100644 --- a/src/compile/scale/range.ts +++ b/src/compile/scale/range.ts @@ -221,11 +221,39 @@ function parseScheme(scheme: Scheme | SignalRef): RangeScheme { return {scheme}; } +function fullWidthOrHeightRange( + channel: 'x' | 'y', + model: UnitModel, + scaleType: ScaleType, + {center}: {center?: boolean} = {} +) { + // If step is null, use zero to width or height. + // Note that we use SignalRefWrapper to account for potential merges and renames. + const sizeType = getSizeChannel(channel); + const sizeSignal = model.getName(sizeType); + const getSignalName = model.getSignalName.bind(model); + + if (channel === Y && hasContinuousDomain(scaleType)) { + // For y continuous scale, we have to start from the height as the bottom part has the max value. + return center + ? [ + SignalRefWrapper.fromName(name => `${getSignalName(name)}/2`, sizeSignal), + SignalRefWrapper.fromName(name => `-${getSignalName(name)}/2`, sizeSignal) + ] + : [SignalRefWrapper.fromName(getSignalName, sizeSignal), 0]; + } else { + return center + ? [ + SignalRefWrapper.fromName(name => `-${getSignalName(name)}/2`, sizeSignal), + SignalRefWrapper.fromName(name => `${getSignalName(name)}/2`, sizeSignal) + ] + : [0, SignalRefWrapper.fromName(getSignalName, sizeSignal)]; + } +} + function defaultRange(channel: ScaleChannel, model: UnitModel): VgRange { const {size, config, mark, encoding} = model; - const getSignalName = model.getSignalName.bind(model); - const {type} = getFieldOrDatumDef(encoding[channel]) as ScaleFieldDef | ScaleDatumDef; const mergedScaleCmpt = model.getScaleComponent(channel); @@ -245,18 +273,7 @@ function defaultRange(channel: ScaleChannel, model: UnitModel): VgRange { } } - // If step is null, use zero to width or height. - // Note that we use SignalRefWrapper to account for potential merges and renames. - - const sizeType = getSizeChannel(channel); - const sizeSignal = model.getName(sizeType); - - if (channel === Y && hasContinuousDomain(scaleType)) { - // For y continuous scale, we have to start from the height as the bottom part has the max value. - return [SignalRefWrapper.fromName(getSignalName, sizeSignal), 0]; - } else { - return [0, SignalRefWrapper.fromName(getSignalName, sizeSignal)]; - } + return fullWidthOrHeightRange(channel, model, scaleType); } case XOFFSET: @@ -374,6 +391,11 @@ function getOffsetStep(step: Step, offsetScaleType: ScaleType) { function getOffsetRange(channel: string, model: UnitModel, offsetScaleType: ScaleType): VgRange { const positionChannel = channel === XOFFSET ? 'x' : 'y'; const positionScaleCmpt = model.getScaleComponent(positionChannel); + + if (!positionScaleCmpt) { + return fullWidthOrHeightRange(positionChannel, model, offsetScaleType, {center: true}); + } + const positionScaleType = positionScaleCmpt.get('type'); const positionScaleName = model.scaleName(positionChannel); diff --git a/src/stack.ts b/src/stack.ts index db05480795..00e7bb3b38 100644 --- a/src/stack.ts +++ b/src/stack.ts @@ -176,16 +176,16 @@ export function stack(m: Mark | MarkDef, encoding: Encoding): StackPrope groupbyChannels.push(dimensionChannel); groupbyFields.add(dimensionField); } + } - const dimensionOffsetChannel = dimensionChannel === 'x' ? 'xOffset' : 'yOffset'; - const dimensionOffsetDef = encoding[dimensionOffsetChannel]; - const dimensionOffsetField = isFieldDef(dimensionOffsetDef) ? vgField(dimensionOffsetDef, {}) : undefined; + const dimensionOffsetChannel = dimensionChannel === 'x' ? 'xOffset' : 'yOffset'; + const dimensionOffsetDef = encoding[dimensionOffsetChannel]; + const dimensionOffsetField = isFieldDef(dimensionOffsetDef) ? vgField(dimensionOffsetDef, {}) : undefined; - if (dimensionOffsetField && dimensionOffsetField !== stackedField) { - // avoid grouping by the stacked field - groupbyChannels.push(dimensionOffsetChannel); - groupbyFields.add(dimensionOffsetField); - } + if (dimensionOffsetField && dimensionOffsetField !== stackedField) { + // avoid grouping by the stacked field + groupbyChannels.push(dimensionOffsetChannel); + groupbyFields.add(dimensionOffsetField); } // If the dimension has offset, don't stack anymore diff --git a/test/compile/mark/encode/position-rect.test.ts b/test/compile/mark/encode/position-rect.test.ts index a053a2d4d3..f042b1f725 100644 --- a/test/compile/mark/encode/position-rect.test.ts +++ b/test/compile/mark/encode/position-rect.test.ts @@ -29,6 +29,33 @@ describe('compile/mark/encode/position-rect', () => { }); }); + it('produces correct x-mixins for xOffset without x', () => { + const model = parseUnitModelWithScaleAndLayoutSize({ + data: {values: []}, + mark: 'bar', + encoding: { + xOffset: { + field: 'a', + type: 'nominal' + }, + y: { + field: 'b', + type: 'quantitative' + } + } + }); + + const props = rectPosition(model, 'x'); + expect(props.x).toEqual({ + signal: 'width', + mult: 0.5, + offset: {scale: 'xOffset', field: 'a'} + }); + expect(props.width).toEqual({ + signal: `max(0.25, bandwidth('xOffset'))` + }); + }); + it('produces correct x-mixins for timeUnit with bandPosition = 0', () => { const model = parseUnitModelWithScaleAndLayoutSize({ data: {values: []}, diff --git a/test/compile/scale/range.test.ts b/test/compile/scale/range.test.ts index 36cb40bec6..2e92b6866e 100644 --- a/test/compile/scale/range.test.ts +++ b/test/compile/scale/range.test.ts @@ -132,6 +132,48 @@ describe('compile/scale', () => { ); }); + it('should return [-width/2, width/2] for xOffset without x', () => { + const model = parseUnitModelWithScaleExceptRange({ + mark: 'bar', + encoding: { + xOffset: {field: 'xSub', type: 'nominal'}, + y: {field: 'y', type: 'nominal'} + } + }); + + expect(parseRangeForChannel('xOffset', model)).toEqual( + makeImplicit([ + { + signal: '-width/2' + }, + { + signal: 'width/2' + } + ]) + ); + }); + + it('should return [-height/2, height/2] for yOffset without y', () => { + const model = parseUnitModelWithScaleExceptRange({ + mark: 'bar', + encoding: { + yOffset: {field: 'xSub', type: 'nominal'}, + x: {field: 'y', type: 'nominal'} + } + }); + + expect(parseRangeForChannel('yOffset', model)).toEqual( + makeImplicit([ + { + signal: '-height/2' + }, + { + signal: 'height/2' + } + ]) + ); + }); + it('should return padded duration range when there is a nested offset with year time scale and default padding', () => { const model = parseUnitModelWithScaleExceptRange({ mark: 'bar', diff --git a/test/stack.test.ts b/test/stack.test.ts index 455748d07d..d215d92250 100644 --- a/test/stack.test.ts +++ b/test/stack.test.ts @@ -1,5 +1,5 @@ import {NonArgAggregateOp} from '../src/aggregate'; -import {DETAIL, X, Y} from '../src/channel'; +import {DETAIL, X, Y, YOFFSET} from '../src/channel'; import * as log from '../src/log'; import {ARC, AREA, BAR, PRIMITIVE_MARKS, RECT} from '../src/mark'; import {ScaleType} from '../src/scale'; @@ -327,6 +327,41 @@ describe('stack', () => { } }); + it('should be correct for grouped bar', () => { + for (const stackableMark of [BAR, AREA]) { + const spec: TopLevel = { + data: {url: 'data/barley.json'}, + mark: stackableMark, + encoding: { + x: {field: 'yield', type: 'quantitative'}, + y: {field: 'variety', type: 'nominal'}, + yOffset: {field: 'site', type: 'nominal'}, + color: {field: 'site', type: 'nominal'} + } + }; + const _stack = stack(spec.mark, spec.encoding); + expect(_stack.fieldChannel).toBe(X); + expect(_stack.groupbyChannels).toEqual([Y, YOFFSET]); + } + }); + + it('should be correct for grouped bar without nesting', () => { + for (const stackableMark of [BAR, AREA]) { + const spec: TopLevel = { + data: {url: 'data/barley.json'}, + mark: stackableMark, + encoding: { + x: {field: 'yield', type: 'quantitative'}, + yOffset: {field: 'site', type: 'nominal'}, + color: {field: 'site', type: 'nominal'} + } + }; + const _stack = stack(spec.mark, spec.encoding); + expect(_stack.fieldChannel).toBe(X); + expect(_stack.groupbyChannels).toEqual([YOFFSET]); + } + }); + it('should stack even for two plain quantitatives with the orient on the axes', () => { for (const mark of [BAR, AREA]) { const spec: TopLevel = {