Widget: Don't terminate fonts on widget terminate

Driftwood2D / Driftwood

Driftwood 2D Tiling Game Engine and Development Suite

http://tileengine.org/

MIT License

24 stars 1 forks source link

Widget: Don't terminate fonts on widget terminate #108

Closed pmer closed 7 years ago

pmer commented 7 years ago

This PR can be automatically merged.

But I'd like to say something while I'm here. Fonts & other cache-able resources (images, sounds, etc) shouldn't be terminated except by CacheManager.

If an area creates a TextWidget, that widget loads a font. If the widget is terminated, it also terminates the font. If another TextWidget is created with the same font, the font is re-loaded from disk.
When leaving an area, all widgets are destroyed. Therefore, all fonts are terminated. Therefore, any fonts that the new area uses have to be re-loaded.

This PR fixes the font reloading issue. A font will now only be loaded once. (Try leaving blue1 and coming back.) It looks like a similar thing might be going on in other places of the code, but I haven't checked. If you see any others, correct them!

pmer commented 7 years ago

I added https://github.com/seisatsu/Driftwood/commit/8af12c457c17f4f3b8a86127da123694359fdcaa to spot them.

seisatsu commented 7 years ago

Who terminates the font if the cache is disabled?

pmer commented 7 years ago

If you want resource sharing to work while the cache is disabled you'll need to do reference counting. Then we can replace this self.font = None with self.font.deref().

Otherwise if the policy is that there's no resource sharing when the cache is disabled then you'll want:

if cache disabled:
    self.font._terminate()

pmer commented 7 years ago

Actually I think we need reference counting even if the cache is enabled. Otherwise the cache might call the destructor after the cache entry times out.

Let me check if this is indeed the case.

UPDATE: Yep. See #111.

pmer commented 7 years ago

@seisatsu Why do we allow the cache be disabled? I am having a hard time justifying it to myself.

pmer commented 7 years ago

I am working on fixing the CacheManager, but I don't plan to support actually disabling the cache because this simplifies the logic & code.

Also since it's unlikely that people will disable the cache it wouldn't have gotten tested much and would have been at risk to develop bugs.

To achieve a result similar to disabling the cache you can set the expiration time to something low, such as 10 seconds.