Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Section] Better content clipping #157

Open
johsi-k opened this issue Feb 28, 2020 · 10 comments
Open

[Section] Better content clipping #157

johsi-k opened this issue Feb 28, 2020 · 10 comments

Comments

@johsi-k
Copy link
Contributor

johsi-k commented Feb 28, 2020

The Area mark has been set up such that when the second dependent variable (y2 or x2) is not specified, it defaults to 0.

However in some cases it may be undesirable for the dependent axis to start at 0. For instance when plotting speed against time, with area representing distance travelled, we may want a truncated y-axis beginning at 1000
image

Yet when y2 is not specified, we end up with a graph like this
image
reproduction: https://svelte.dev/repl/1e6fa75636d44e00a9ada00dc7c39dec?version=3.19.1

Rather than mandating that the user specify y2={[1000, 1000, 1000]} or replacing its default 0 with domainMin in the back end, I suggest we clip whatever is rendered by Mark to the bounds of the data

<clipPath id={`clip-${sectionId}-data`}>
  <rect 
    x={Math.min(...rangeX)}
    y={Math.min(...rangeY)}
    width={Math.abs(rangeX[0] - rangeX[1])}
    height={Math.abs(rangeY[0] - rangeY[1])}
    fill="white"
  />
</clipPath>

instead of the bounds of the section, as is currently the case

<clipPath id={`clip-${sectionId}`}>
  <rect 
    x={Math.min(scaledCoordinates.x1, scaledCoordinates.x2)}
    y={Math.min(scaledCoordinates.y1, scaledCoordinates.y2)}
    width={Math.abs(scaledCoordinates.x2 - scaledCoordinates.x1)}
    height={Math.abs(scaledCoordinates.y2 - scaledCoordinates.y1)}
  />
 </clipPath>
...
<g class="section" clip-path={`url(#clip-${sectionId})`} >
  ...
  <slot />
</g>

For example in the case of polygons,

{#if renderPolygon}
    <path clip-path={`url(#clip-${$sectionContext._sectionId}-data)`}
      class={type.toLowerCase()}
      d={generatePath($tr_screenGeometry)}
      fill={$tr_fill}
      stroke={$tr_stroke}
      stroke-width={$tr_strokeWidth}
      fill-opacity={$tr_fillOpacity}
      stroke-opacity={$tr_strokeOpacity}
      opacity={$tr_opacity}
    />
{/if}
@johsi-k johsi-k changed the title Better content clipping in Section [Section] Better content clipping Feb 28, 2020
@luucvanderzee
Copy link
Collaborator

This makes sense, but we need to think this through, because in the current implementation this will also clip off the axes, which are currently implemented using florence marks. I think we might need to change the axes anyway, because currently they don't work well with zooming and in polar coordinates...

@johsi-k
Copy link
Contributor Author

johsi-k commented Feb 29, 2020

Yes the axes would be clipped once we attach the section-data clipPath to each rendered element in Mark.svelte and Layer.svelte. Not only the axes but potentially all labels and titles that occur outside the section-data boundary. We probably need to rethink our informational model.

@luucvanderzee
Copy link
Collaborator

What about a prop to control clipping? We can switch it off by default for all marks except the Label, and the user can then override it when needed.

@luucvanderzee
Copy link
Collaborator

Also, for the Area mark in particular, the solution here would be to just have different default behavior when the x2/y2 props are not provided: just use the lower bound of the domain. This is also how the Rectangle works.

@johsi-k
Copy link
Contributor Author

johsi-k commented Mar 2, 2020

just use the lower bound of the domain

Hm how would that work more generally though? The rectangle and area marks happen to be special cases we can hack around.

Also we have an example here where all marks are draggable into the padding/axes region when they shouldn't be:
drag-all-marks

@luucvanderzee
Copy link
Collaborator

luucvanderzee commented Mar 2, 2020

Hm how would that work more generally though? The rectangle and area marks happen to be special cases we can hack around.

I don't think we have to think about how it works more generally, precisely because they are special cases. I also would not call this 'hacking around': it is just specifying sensible default behavior. The current default behavior is not sensible, therefore we change it.

Also we have an example here where all marks are draggable into the padding/axes region when they shouldn't be:

Do you mean that they should be cropped as they are dragged outside of the padded area? Because to that I would agree, but that could be solved with the solution suggested above. Or do you mean that you should not be able to move them beyond the borders of padded area? Because I think there are cases where you do want to be able to do that, and that if you don't, then that should be solved in user-land.

@johsi-k
Copy link
Contributor Author

johsi-k commented Mar 3, 2020

I don't think we have to think about how it works more generally, precisely because they are special cases.

The area and rectangle marks are special cases only because their geometries can be directly manipulated with the x2/y2 props. But how do we manipulate the geometries of all other marks?

Say we have a <Point x={100} y={110} radius={50} />:
image
How would we crop its geometry to the domain bounds? Circles have to become semicircles when clipped, and to do the same for every other mark will require a custom geometry converter.

In the absence of a method for Section to know if it has received an axis component (as opposed to other marks) as a child, I'd take your first suggestion of adding a prop to control clipping, but to have clipping enabled by default for all marks/layers. This can then be overridden in the axis component by disabling clipping in each of its child Line, LineLayer and LabelLayer components.

In Mark.svelte

<script>
  export let clipped = true
</script>

{#if renderPolygon}
  <path clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}
    class={type.toLowerCase()}
    ...
  />
{/if}

{#if renderCircle}
  <circle clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}
    class="point"
    ...
  />
{/if}

{#if renderLine}
  <path clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}
    class="line"
    ...
  />
{/if}

{#if renderLabel}
  <text clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}
    class="label"
    ...
  >
    {aesthetics.text}
  </text>
{/if}

In Layer.svelte

<script>
  export let clipped = true
</script>

{#if renderPolygon}
  <g class={`${type.toLowerCase()}-layer`} clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}>
    {#each Object.keys($tr_screenGeometryObject) as $key ($key)}
      <path
        class={type.toLowerCase()}
        ...
      />
    {/each}
  </g>
{/if}

{#if renderCircle}
  <g class="point-layer" clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}>
    {#each Object.keys($tr_screenGeometryObject) as $key ($key)}
      <circle
        class="point"
        ...
      />
    {/each}
  </g>
{/if}

{#if renderLine}
  <g class="line-layer" clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}>
    {#each Object.keys($tr_screenGeometryObject) as $key ($key)}
      <path
        class="line"
        ...
      />
    {/each}
  </g>
{/if}

{#if renderLabel}
  <g class="label-layer" clip-path={clipped ? `url(#clip-${$sectionContext._sectionId}-data)` : 'none'}>
    {#each Object.keys($tr_screenGeometryObject) as $key ($key)}
      <text 
        class="label"
        ...
      >
        {textObject[$key]}
      </text>
    {/each}
  </g>
{/if}

In Line.svelte, LineLayer.svelte and LabelLayer.svelte

<script>
  export let clipped = true
</script>

<!-- for each of the respective files -->
<Mark type="Line" ...  {clipped} />
<Layer type="Line" ... {clipped} />
<Layer type="Label" ... {clipped} />

In the axis components

<g class="x-axis"> <!-- "y-axis" in YAxis.svelte -->
  {#if baseLine}
    <Line ... clipped={false} />
  {/if}

  {#if ticks}
    <LineLayer ... clipped={false} />
    <LabelLayer clipped={false} />
  {/if}

  {#if title.length > 0}
    <Label clipped={false} />
  {/if}
</g>

@luucvanderzee
Copy link
Collaborator

I'd take your first suggestion of adding a prop to control clipping, but to have clipping enabled by default for all marks/layers. This can then be overridden in the axis component by disabling clipping in each of its child Line, LineLayer and LabelLayer components.

Yeah that works for me! Code looks good 👍

How would we crop its geometry to the domain bounds? Circles have to become semicircles when clipped, and to do the same for every other mark will require a custom geometry converter.

If we implement the clipped prop like in the code you just posted, we don't need to- it will be clipped off automatically. With the clipped prop, the Area will also be clipped automatically, so the original problem you started this issue with would be solved. However, there is a separate issue I want to draw attention to:

  1. Do we want the Area to have default values for x2 and y2?
  2. If we do, what should their default values be?

To question 1 I say yes, and to question 2 I say: the lower bound of the domain in their respective dimension. This is a sensible default in my opinion. We could, of course, also have another default value, like we have now. However, I think that is a weird default value, that's why I argue for the other one.

This is, however, a different discussion from whether we should implement clipping- we should, and the code you just posted looks perfect for that! But, in my opinion, we should also change the default value for the Area.

@johsi-k
Copy link
Contributor Author

johsi-k commented Mar 4, 2020

Yeah that works for me!

Thanks! I'll PR that if everyone else is ok with it.

To question 1 I say yes, and to question 2 I say: the lower bound of the domain in their respective dimension. This is a sensible default in my opinion. We could, of course, also have another default value, like we have now. However, I think that is a weird default value, that's why I argue for the other one.

What happens when y1 is negative? By setting the default value of y2 to the lower bound of the domain we would end up with a graph like
image

instead of this, as one might expect
image

@luucvanderzee
Copy link
Collaborator

luucvanderzee commented Mar 4, 2020

That's a good point, didn't think of that. In that case we can do something like

function getY2Default (domain) {
  if (isQuantitative(domain) {
    if (domain.every(v => v > 0) || domain.every(v => v < 0)) {
      return domain[0]
    } else {
      return 0
    }
  }

  if (isCategorical(domain) {
    return domain[0]
  }

  if (isTemporal(domain) {
    return domain[0]
  }
}

Or am I missing cases where this would not work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants