Pull requests

#6 Declined
Repository
dangillet dangillet
Branch
default
Repository
marcusva marcusva
Branch
default

Implemented a method from_text in the SpriteFactory.

Author
  1. dangillet
Reviewers
Description

First attempt at implementing a from_text method in SpriteFactory.

Not sure if it is the way to go.

Welcome any feedback.

  • Learn about pull requests

Comments (23)

  1. Bil Bas

    Don't forget we have wrapping in SDL2:

    textSurface = sdlttf.TTF_RenderText_Shaded_Wrapped(..., width)

    Also, if the background color is RGBA(0, 0, 0, 0) it should really use TTF_RenderText_Blended_Wrapped

    And to deal with TTF not being available you could just catch the exception if it fails.

  2. dangillet author

    Hi Bil,

    Thanks for the comments. I was actually unaware that SDL2_TTF had wrapped functions. That's really awesome! So I tried to incorporate them. I also changed to blended when the background is black.

    I'm new to Hg and I messed up a bit with my repo. I can see that obviously this is seen in this updated pull request. Sorry for that. Is there something I can do to clean it up?

    Otherwise I'm not sure where you would catch exceptions if TTF is not available. Do you mean in the common.py file?

  3. Bil Bas

    Great that you've updated it so quickly!

    However, SDL_Color(0, 0, 0) is solid black, which is a reasonable non-transparent background colour. SDL_Color(0, 0, 0, 0) is transparency and should be the default background colour (and cause Blended).

  4. Bil Bas

    Not entirely sure where you should catch exceptions. I know in my project, where I've implemented a very similar (though less robust) from_text(), I've just assumed that SDL_TTF is loaded, because it will be ;)

  5. Bil Bas

    Oh and Blended functions don't require a background color (for obvious reasons). I can fork and edit your version if you are feeling I'm just telling you what to do!

  6. dangillet author

    Thanks a lot. I was indeed trying to run some tests and failed miserably for that reason. I've updated the code and made some more changes. I welcome any feedback as it makes me improve my programming skills. I don't have (yet) an ego problem. :)

    There's something else I noticed which annoys me a bit. If we don't delete the SpriteFactory before sdl2ext.quit() is called, we get an exception as the SpriteFactory.del() tries to call TTF_CloseFont(). I don't see how to address this problem properly. I'm thinking that maybe the Font dict should be dealt in a separate class, like a FontResource. The user would have to shut it down properly before calling sdl2ext.quit().

    In the meantime, I will check in the del method if TTF is still initialized, and if not, re-initialize it, close the fonts, and close TTF. Or is this an overkill and I could just catch the exception and pass silently on it?

  7. Bil Bas

    No such thing as windir when not on Windows, so use 'if "windir" in os.environ:' to skip if it if it isn't going to be there. Where fonts are on Linux/Mac, is more difficult to discover, so maybe leave that for now (just so it doesn't break because we don't have a windir!).

  8. dangillet author

    Right! I try now to look in different folders depending on the OS. The idea is that if you just type a font name without any path, it should be one of the system font, a bit like web browser fonts.

    At least now if the platform is not recognized, no prefix is used and an exception will be thrown because the font couldn't be found in the current folder.

  9. Marcus von Appen repo owner

    Thanks for your efforts, Dan!

    Please go ahead with the seperate class of a FontResource or FontManager, which takes care of managing the TTF fonts. To save you the hassle of creating your own font detection, please look at https://bitbucket.org/marcusva/python-utils/src/9a7e4f481c76197f315b5096fc5a3e33b550c24d/utils/sysfont.py?at=default, which provides a system font detection for Windows and X11 (MacOS X still missing).

    I do not want to have platform-specific font detection code within py-sdl2, though. From my experience it can often create problems, if the underlying system is wrongly (or differently) configured, fonts are not available on the target system, etc. - and the developer should look into the right place instead of creating a ticket here ;-).

    Back to the FontManager class. It should/could be able to create rendered text, ideally with only a small set of of function arguments, with defaults being set as class attributes. A quick 1-minute brainstorming:

    class FontManager:
      def __init__(self, ...):
        self.bgcolor = bgcolor
        self.fgcolor = fgcolor
        self.fonts = {} # dict containing a TTF_Font to name mapping
        self.defaultfont = None # Default font name to be used - must be available in self.fonts
        self.fontsize = ....
    
      def __del__(self):
          # clean all fonts - it's the user's responsibility to clean up before quitting.
      def render(self, text, bgcolor=None, fgcolor=None, font=None):
         ....
      # and other useful methods
    
    class SpriteFactory:
      ...
      def from_text(self, text, **kwargs):
        manager = self.default_args["fontmanager"]
        # some checks
        textsf = manager.render(**kwargs))
        return textsf
    
  10. dangillet author

    Hi Marcus,

    Thanks for the guidance. I do agree that it's probably not a good idea to try to have a system font detection in py-sdl2.

    I'm leaving tomorrow for my work. I'll try to code a bit, but I might be able to update this code only by the end of the week (week-end). Just to say that I'm not ignoring your post.

  11. dangillet author

    Hi Marcus,

    Here is what I came up with. I tried to use mock in the tests. I hope you don't mind. It's a bit of a learning experience for me, so if you have any suggestions to the way I use them, please feel free.

    There might be a few other things to clean / update. But hopefully this will be a better starting point.

    Cheers, Dan.

  12. dangillet author

    Hi Marcus,

    Here is the updated version, taking into account your very useful comments. Thanks for that.

    I'm still not 100% sure that my tests do the right thing. I guess there could be many more. Just let me know if I missed some obvious cases. Also I could have missed some CamelCases, so don't hesitate to point them out. :)

    Regards, Dan.

  13. dangillet author

    Hi,

    I've updated the code according to your comments. I was not 100% sure at how to make default_font behave. Let me know if you like what I did. If not, let me know what behavior you have in mind.

    As usual, I'm opened to feedback.

    Best regards, Dan.

  14. Marcus von Appen repo owner

    A (probably) last set of comments - I think the FontManager, you designed, is a solid start, which has room for improvements in later stages. If you could edge out the last small issues, I would assume it to be ready for a merge. Thanks for your effort!

  15. dangillet author

    Thanks for your kind words. But you helped tremendously in the design of the FontManager, so I don't think I should take that much credit. I'm glad someone can show me better ways to approach a problem, and takes the time to do it.

    So here is the (probably) last changes.

    Daniel.

  16. Marcus von Appen repo owner

    Two more things spotted, one leftover and a question regarding the comic.ttf file - is the file free from any copyright and legal restrictions (placed in the Public Domain)?

  17. dangillet author

    I looked on the web and couldn't make sure comic.ttf was in the Public Domain. So I replaced it by another font under the Public Domain. Its licence is included.

  18. Marcus von Appen repo owner

    I would like to avoid a different license (such as the OFL) within the distribution, since it complicates redistributing or modifying the whole package. Please use a font, for which the author(s) decided not to care about what happens to it.

  19. dangillet author

    Ok, understood. I found another font without any license. This made me realize that default alias mechanism should be modified in case the font_path contains dots. So resources/effloresce.regular.ttf will lead to effloresce.regular as default alias.

  20. dangillet author

    Well, according to what I read, it seemed ok to use it. But instead of debating about this, I thought of a better solution. I just copied tuffy.ttf and renamed it tuffy.copy.ttf. So it's twice the same font, but different font files, and so behaves like different fonts for the test cases.