A tiny bug in BoundingBox.draw()

[zyuchuan]

No description provided.

[zyuchuan]

Class BoundingBox() {
    ...

    draw(ctx, x, y) {
    if (!x) x = 0;
    if (!y) y = 0;
    ctx.rect(this.x + x, this.y + y, this.w, this.h);
    ctx.stroke() // this is wrong
  }
}

[0xfe]

Can you explain please? A stroke() call is required to render the rectangle.

[zyuchuan]

Can you explain please? A stroke() call is required to render the rectangle.

BoundingBox::draw(...) calls ctx.rect(...), which in turn creates a <rect> element in DOM tree, this is all necessary for rendering a rectangle. The following stx.stroke() creates a <path> element in DOM tree, and render it using whatever path definition in previous operations.

If you run stavenote_tests.js and check generated HTML code for StaveNote (SVG): StaveNote BoundingBoxes - Treble, you'll find something as shown below:

...
<rect fill="none" ....>...</rect>
<path stroke-width="1"...>...</path>
...

<rect> element is a bounding box, follows each <rect> element is a <path> element, which renders a note head, which is created by ctx.draw(), this is obviously wrong.

[0xfe]

Okay, got it -- but if you remove the <stroke> the box will not be rendered, no? Isn't a more appropriate fix here to add a ctx.beginPath() before the rect(...) call?

Also, can you send me a screenshot of the bounding box tests, to verify that no new issues are introduced? :-)

[zyuchuan]

As shown above, the <rect> node in red frame is a bounding box. Note that the two <path> nodes marked by cyan frames are identical, they both render a note A4, the lower <path> element is rendered by function call ctx.stroke().

I think it's OK to call ctx.beginPath() before the rect(...) call, but it's not necessary to do that. I went through the source code, SVGContext::stroke() does nothing except creating a <path> node. We don't need to create a <path> node, we just need create a <rect> node, then SVG will do all dirty works, such as rendering.

[gristow]

Just catching up on this -- in fact, it appears I made an error when first implementing SVGContext in that SVGContext.rect(x, y, w, h) adds a rectangle object directly to the SVG without waiting for a stroke or fill call. Looking at the code, I realize now this is different than the implementation in CanvasContext. (See this fiddle for example.)

So: in fact, the issue is in the SVGContext code, not the BoundingBox code. Or: alternately, we could rewrite CanvasContext to follow what SVGContext has been doing.

Did all the visual regression tests pass? It's entirely possible that this bug isn't causing any visual differences between the two, given how rarely context.rect() is used in the codebase.

[0xfe]

Thanks @gristow -- I had the same thought and was just about to tag you in. Removing the stroke() would probably break the canvas implementations.

If the fix is simple enough, I'd prefer to fix SVGContext.rect(), so this doesn't bite us in the future. I don't think we should CanvasContext.

@zyuchuan, thanks for catching this bug! :-)