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

Clean up DOM when using container option upon re-running or Cycle .dispose() being called #1

Open
wyqydsyq opened this issue Jun 13, 2017 · 1 comment

Comments

@wyqydsyq
Copy link

wyqydsyq commented Jun 13, 2017

When the Cycle instance running this driver calls .dispose() (e.g. in response to cycle-restart receiving a HMR update) any generated elements/objects should be cleaned up, this already happens implicitly when using the fullscreen automatic element or the canvas option as Regl directly replaces those elements when running the next time, but if specifying container you end up with multiple canvas elements. This might be the intended behaviour, I'm just opening it as an issue because it's not the behaviour I expected.

For example, if you use something like this and get a HMR update:

makeReglDriver({
  container: document.getElementById('viewport')
}),

canvas elements from previous executions will remain inside the #viewport element, a new canvas with the new result is appended to #viewport.

My guesses for a potential solution would be one of:

  • When options.container is set:
    • Hard: Unset the container's children as an explicit cleanup step, only removing children that were created by this driver, this will require the driver to track the elements regl creates.
    • Easy: Clear all of the container element's children out prior to calling setupRegl.
@Widdershin
Copy link
Member

This is a bug, after dispose() the world should be as it was prior to the driver being created.

There is currently no disposal implemented on this driver, and there definitely should be.

Are you interested in having a go at making a pull request? I'd be happy to review it and merge it 😄

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

No branches or pull requests

2 participants