Vega Expressions for Lasso Selection

[dvmoritzschoefl]

This PR includes the following vega expressions:

• lassoPath (generates a svg path from a lasso signal, can be directly used on a path mark)
• lassoAppend (handles the logic to append a point to the lasso signal)
• intersectLasso (performs the intersection tests for the lasso and returns the appropriate tuples)

There is an example specification added that allows to test the lasso (called lasso.vg.json). If this PR gets approved, I will create another PR for vega-lite that includes the generator for lasso selections.

[arvind]

Hi @dvmoritzschoefl, this PR looks like a great start. I've left some line/block-level suggestions but three high-level thoughts:

1. I think it'd be useful to have these lasso functions remain as general purpose as possible rather than tightly coupled with Vega-Lite selections. So, I recommend removing any references to selections here. Instead, have this function return an array of scene graph items within the lasso and, in the expression string generated byVega-Lite, pass this array to the vlSelectionTuples function introduced in add expr functions for extensional selection predicates #3009 and improved in Optimize selection functions for ID-only stores #3397 to extract SELECTION_ID and populate the selection stores.

2. Relatedly, I'm not sure I'm totally following why we have this function inverting pixels -> data coordinates. Can we not stay entirely in pixel space for the entirety of the hit testing process (i.e., an intersectLasso function instead)? I'm imagining the same steps you have currently (start with a lasso path, calculate the bounding box, intersect it with the scenegraph, and then validate which pointInPolygon), but you'd never make a roundtrip to data space at all. Instead, and following (1) above, we'd only go into data space once we have all the scene graph items that fall within the lasso and just to extract the SELECTION_ID. Does this make sense? I could very well believe I'm missing something though.

3. A more minor point: in Vega and Vega-Lite, we default to a 2-space indent. Do you mind updating this file to follow that format? Thanks!

[dvmoritzschoefl]

Hi @dvmoritzschoefl, this PR looks like a great start. I've left some line/block-level suggestions but three high-level thoughts:

1. I think it'd be useful to have these lasso functions remain as general purpose as possible rather than tightly coupled with Vega-Lite selections. So, I recommend removing any references to selections here. Instead, have this function return an array of scene graph items within the lasso and, in the expression string generated byVega-Lite, pass this array to the vlSelectionTuples function introduced in add expr functions for extensional selection predicates #3009 and improved in Optimize selection functions for ID-only stores #3397 to extract SELECTION_ID and populate the selection stores.
2. Relatedly, I'm not sure I'm totally following why we have this function inverting pixels -> data coordinates. Can we not stay entirely in pixel space for the entirety of the hit testing process (i.e., an intersectLasso function instead)? I'm imagining the same steps you have currently (start with a lasso path, calculate the bounding box, intersect it with the scenegraph, and then validate which pointInPolygon), but you'd never make a roundtrip to data space at all. Instead, and following (1) above, we'd only go into data space once we have all the scene graph items that fall within the lasso and just to extract the SELECTION_ID. Does this make sense? I could very well believe I'm missing something though.
3. A more minor point: in Vega and Vega-Lite, we default to a 2-space indent. Do you mind updating this file to follow that format? Thanks!

1. I used the functions you mentioned, the function is now called 'intersectLasso' instead and does not use data space
2. solved
3. updated to 2-space indent

attached is a working lasso spec
lasso.vg.zip

[arvind]

Hi @dvmoritzschoefl, thanks for making the changes! Looks like we're pretty close. I left a handful of comments, just to make the code a bit more functional rather than imperative. Two other high-level comments:

• packages/vega/test/specs-valid.json is currently deleted but shouldn't be.
• docs/*.js will be built as part of the final version release, so you don't need to include them in this PR.

[lgtm-com]

This pull request introduces 2 alerts when merging df07358 into 0a56954 - view on LGTM.com

new alerts:

• 2 for Syntax error

[dvmoritzschoefl]

using entries().reduce gave an error for me, this version works
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce

[dvmoritzschoefl]

Hi @dvmoritzschoefl, thanks for making the changes! Looks like we're pretty close. I left a handful of comments, just to make the code a bit more functional rather than imperative. Two other high-level comments:

• packages/vega/test/specs-valid.json is currently deleted but shouldn't be.
• docs/*.js will be built as part of the final version release, so you don't need to include them in this PR.

Yes sry I made a mistake checking these files in, should be ok by now. I changed your suggestions accordingly and made sure the lasso still works.

[arvind]

Thanks @dvmoritzschoefl! The updates look good to me, and apologies for my malformed suggestions — the dangers of writing JavaScript code on-the-fly in the GitHub editor textbox 🤦

I've cc'ed @jheer to take a final look in case he notices anything I missed, and then we can merge 🙂

[dvmoritzschoefl]

@arvind very nice, I will adjust the vega-lite PR to those changes tomorrow and ping you when I am finished