wwwtyro / candygraph

Fast by default, flexible 2D plotting library.
Other
435 stars 11 forks source link

Scissor should have .dispose() method #40

Closed shamrin closed 2 years ago

shamrin commented 2 years ago

Scissor object should have .dispose() method.

While implementing PR #39, I had to jump through some hoops to properly dispose of buffers:

const lineStrips = [
    createLineStrip(...),
    createLineStrip(...),
];
const scissor = createScissor(cg, ..., lineStrips);
const xaxis = createOrthoAxis(cg, ...);
const yaxis = createOrthoAxis(cg, ...);
cg.render(coords, viewport, [scissor, xaxis, yaxis]);
// ...
xaxis.dispose();
yaxis.dispose();
lineStrips.forEach(child => child.dispose());

It would be better if Scissor would have .dispose() method:

const renderables = [
    createScissor(cg, ..., [
        createLineStrip(...),
        createLineStrip(...),
    ]),
    createOrthoAxis(cg, ...),
    createOrthoAxis(cg, ...}),
];
cg.render(coords, viewport, renderables);
// ...
renderables.forEach(renderable => renderable.dispose())

P.S. Even better if 1) disposing was automatic, or 2) there was something like cg.disposeLastRenderBuffers(), or 3) one could do something like cg.render(...).dispose().

flekschas commented 2 years ago

Regarding:

disposing was automatic

auto-disposing was a thing until https://github.com/wwwtyro/candygraph/pull/26 but it was removed in favor of a simpler way to retain resources. @wwwtyro can explain best. Also see our discussion on re-introducing auto-disposal at https://github.com/wwwtyro/candygraph/issues/34#issuecomment-1134987124 and please chime in :)

shamrin commented 2 years ago

Scissor does have .dispose() method. I forgot to check in the parent Composite class. 🤦