Skip to content

Commit

Permalink
Plot entity store creation pass incorrect bounds to the entity store,…
Browse files Browse the repository at this point in the history
… which thenm excludes any valid entities which are incorrectly identified from being considered for nearest calculation. This fix converts the bounds passed here to be relative to the plots own origin, as are the entities positions, so that this now gets correctly calculated (#3388)
  • Loading branch information
alechill authored and themadcreator committed Aug 30, 2017
1 parent fa04969 commit 99d9306
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 5 deletions.
21 changes: 16 additions & 5 deletions src/plots/plot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { Dataset, DatasetCallback } from "../core/dataset";
import {
AttributeToAppliedProjector,
AttributeToProjector,
Bounds,
IAccessor,
IRangeProjector,
Point,
Expand Down Expand Up @@ -777,17 +778,29 @@ export class Plot extends Component {
protected _getEntityStore(datasets?: Dataset[]): Utils.IEntityStore<Plots.ILightweightPlotEntity> {
if (datasets !== undefined) {
const entityStore = new Utils.EntityStore<Plots.ILightweightPlotEntity>();
entityStore.addAll(this._buildLightweightPlotEntities(datasets));
entityStore.addAll(this._buildLightweightPlotEntities(datasets), this._localOriginBounds());
return entityStore;
} else if (this._cachedEntityStore === undefined) {
const entityStore = new Utils.EntityStore<Plots.ILightweightPlotEntity>();
entityStore.addAll(this._buildLightweightPlotEntities(this.datasets()), this.bounds());
entityStore.addAll(this._buildLightweightPlotEntities(this.datasets()), this._localOriginBounds());
this._cachedEntityStore = entityStore;
}

return this._cachedEntityStore;
}

/**
* _localOriginBounds returns bounds of the plot from its own origin, rather than from parent origin (as provided by `this.bounds()`)
*
* @return {Bounds}
*/
protected _localOriginBounds(): Bounds {
return {
topLeft: { x: 0, y: 0 },
bottomRight: { x: this.width(), y: this.height() },
};
}

protected _entityBounds(entity: Plots.IPlotEntity | Plots.ILightweightPlotEntity) {
const { datum, index, dataset } = entity;
const { x, y } = this._pixelPoint(datum, index, dataset);
Expand Down Expand Up @@ -828,11 +841,9 @@ export class Plot extends Component {
* or undefined if no {Plots.PlotEntity} can be found.
*
* @param {Point} queryPoint
* @param {bounds} Bounds The bounding box within which to search. By default, bounds is the bounds of
* the chart, relative to the parent.
* @returns {Plots.PlotEntity} The nearest PlotEntity, or undefined if no {Plots.PlotEntity} can be found.
*/
public entityNearest(queryPoint: Point, bounds = this.bounds()): Plots.IPlotEntity {
public entityNearest(queryPoint: Point): Plots.IPlotEntity {
const nearest = this._getEntityStore().entityNearest(queryPoint);
return nearest === undefined ? undefined : this._lightweightPlotEntityToPlotEntity(nearest);
}
Expand Down
33 changes: 33 additions & 0 deletions test/plots/plotTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,39 @@ describe("Plots", () => {
assert.deepEqual(entities3, entities1);
assert.strictEqual(lightweightPlotEntitySpy.callCount, 4);
});

describe("adding entities", () => {
let addAllSpy: sinon.SinonSpy;

beforeEach(() => {
addAllSpy = sinon.spy(Plottable.Utils.EntityStore.prototype, "addAll");
});

afterEach(() => {
addAllSpy.restore();
});

it("supplies plot bounds from its own origin when adding entities to the store", () => {
const dataset = new Plottable.Dataset();
plot.addDataset(dataset);
const width = 200;
const height = 100;
const originX = 50;
const originY = 20;

plot.setBounds(width, height, originX, originY);
const parentSpaceBounds = plot.bounds();

plot.entities();
const plotLocalBounds = addAllSpy.args[0][1];

assert.notDeepEqual(parentSpaceBounds, plotLocalBounds);
assert.deepEqual(plotLocalBounds, {
topLeft: {x: 0, y: 0},
bottomRight: {x: width, y: height},
});
});
});
});

describe("entityNearest", () => {
Expand Down

1 comment on commit 99d9306

@blueprint-bot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plot entity store creation pass incorrect bounds to the entity store, which thenm excludes any valid entities which are incorrectly identified from being considered for nearest calculation. This fix converts the bounds passed here to be relative to the plots own origin, as are the entities positions, so that this now gets correctly calculated (#3388)

Demo: quicktests | fiddle

Please sign in to comment.