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

tidy up on shutdown to allow multiple init/shutdown cycles per process #299

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gkoulin
Copy link
Contributor

@gkoulin gkoulin commented Oct 1, 2024

  • Hide window.
  • Clear contextStack.
  • Reset cached lazy properties. Had to convert from namespace to struct to allow default initialisation reset.
  • Set state::initialized = false.

@nmwsharp
Copy link
Owner

nmwsharp commented Oct 2, 2024

thanks for this, it would be great to cleanly shutdown & reinitialize (some sloppy engineering on my part that I have put off fixing for many years)

I'm afraid of some subtle bugs due to static variables. I have been gradually removing/consolidating static variables wherever possible, but there are still some hanging around. Also, openGL resource management & shutdown.

After enabling tests, it looks like there's a huge amount of memory being leaked. I think maybe the render::engine is not being freed, and is getting recreated each time? It will take some more debugging; I started looking at this but ran out of time for now.

…ocess

- use `std::uique_ptr<Engine>` to manage global engine lifetime. This means that we no longer need `bool state::initialized`, we can just use the engine ptr to infer initialized state.
- Hide window.
- Clear `contextStack`.
- Reset cached `lazy` properties. Had to convert from namespace to struct to allow default initialisation reset.
In `PolyscopeTest` fixture, initialise polyscope at the start of each test and shutdown at the end. This will test multiple shutdown cycles during the lifetime of the fixture.
@gkoulin
Copy link
Contributor Author

gkoulin commented Oct 2, 2024

Thanks for the feedback. I can't believe ASAN only checks memory leaks on GCC. That's really strange.

Anyway, I've updated ownership of the engine. render::engine is now a std::unique_ptr<Engine>. All other references to the concrete engine, private to the backend, are essentially weak raw pointers.

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 this pull request may close these issues.

2 participants