visx icon indicating copy to clipboard operation
visx copied to clipboard

Brush onBrushEnd does not seem to report the end x position

Open holgergp opened this issue 1 year ago • 2 comments

Hello visx folks!

I am currently introducing a brush to a bar char. While working quite nicely, I noticed that the onBrushEnd callback - that reports a bound - does not report the target x value correctly. I made a sandbox for this: https://codesandbox.io/p/sandbox/relaxed-star-q6jjzg?file=%2Fsrc%2FApp.js There I introduced a Brush which is console logging the bound onBrushEnd. The Brush is using a scaleBand on the x-axis and a scaleLinear on the y-axis. The bound which is reported has the following structure: { "x0": 0, "x1": 0, "xValues": [ "Item 1", "Item 2" ], "y0": -31.428571428571406, "y1": 402.85714285714283 }

While y0 and y1 seem reasonable, x0 might be correct, x1 being 0 is not what I am expecting, as the rectangle has width > 0.

Am I using the brush wrong? Looks like a bug to me. How would the idea be, to get the coordinates of the brushed rectangle?

Thanks for your awesome work!

Cheers from Düsseldorf!

Holger

holgergp avatar Jan 18 '24 10:01 holgergp

Hi there.

Just some thoughts on potential bug:

The scaleBand type of scale doesn't have the invert() function we need to convert pixel space to values space (see https://d3js.org/d3-scale/band

As a result, the bounds struct will contain x as zero (if undefined, etc.). see Brush.tsx (convertRangeToDomain())

/// Brush/Brush.tsx

convertRangeToDomain(brush: BaseBrushState) {
    const { xScale, yScale } = this.props;
    const { x0, x1, y0, y1 } = brush.extent;

    const xDomain = getDomainFromExtent(xScale, x0 || 0, x1 || 0, SAFE_PIXEL);
    const yDomain = getDomainFromExtent(yScale, y0 || 0, y1 || 0, SAFE_PIXEL);

    const domain: Bounds = {
      x0: xDomain.start || 0,
      x1: xDomain.end || 0,
      xValues: xDomain.values,
      y0: yDomain.start || 0,
      y1: yDomain.end || 0,
      yValues: yDomain.values,
    };

    return domain;
  }

As I see it, the scaleInvert function wraps cases like this but seems to be the code is focused on ordinalScale only. see utils.tsx (getDomainFromExtent(), scaleInvert())

May it have the bug?

PS. at the moment I am unable to debug that...sorry PS. The idea by holgergp to extend the bounds with extra info looks great for me. If I remember, the d3-brush events contain target object etc as well.

Regards, Den

dsdevgit avatar Jan 31 '24 13:01 dsdevgit

@dsdevgit I also get wrong wrong domain report in the end - it's slightly extended (by a couple of pixel). In onBrushEnd I update original value and then update brush value with updateBrush. For example if I only move the right boundary - the left one also shifts. Looks like it's happening because of SAFE_PIXEL. Can you explain what's the purpose of it? If I set it to 0 - there are no issues.

Here's the reproduction https://codesandbox.io/p/sandbox/quizzical-khorana-yf9dkw?file=%2FExample.tsx%3A130%2C20 - just click on the brush or try to move/resize.

nd-novorender avatar Jun 12 '24 11:06 nd-novorender