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

Loading fonts from memory #199

Merged
merged 23 commits into from
Sep 11, 2022

Conversation

klausweiss
Copy link
Contributor

@klausweiss klausweiss commented Aug 4, 2022

I was looking into loading fonts from memory in addition to loading from file, in order to be able to easily distribute monomer programs as standalone binaries. Since it isn't possible at the time of writing, would you consider adding this to monomer? I attempted implementing it.

What I have in this PR doesn't work unfortunately. It compiles, but the text doesn't show up (see the labels in the modified todo example below). It's my first time using c2hs, so I must have got something wrong. I'm out of expertise, so I'm calling for help if anyone is interested in doing so.

The todo changes (and an additional dependency) are only there for testing easily. These changes should be reverted when it's finally working.

Screenshot

image

Comment on lines 394 to 398
{- |
Alias for 'appFontDefFile' for backwards compatibility.
-}
appFontDef = appFontDefFile
{-# DEPRECATED appFontDef "Use appFontDefFile directly" #-}
Copy link
Contributor Author

@klausweiss klausweiss Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the changes here get through, I propose deprecating appFontDef in the light of having 2 distinct ways of loading fonts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go down this route, we'll also need to update the examples and docs code to use appFontDefFile instead of appFontDef.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klausweiss I'd prefer to keep the existing appFontDef name as is, and add the new memory based one as appFontDefMem. The main reason is to avoid breaking existing code. A secondary reason is this would make it work similarly to the image widget, that has image for path based images and imageMem for memory based ones.

If you prefer to keep it as appFontDefMemory, it works great for me too.

Copy link
Contributor Author

@klausweiss klausweiss Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good (re deprecation).

Renamed appFontDefMemory back to appFontDefMem.

if res >= 0
then return $ path : fonts
then return $ name : fonts
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the values inside the resulting IO [Text] weren't used and the fonts loaded from memory don't have a path identifying them, this was changed to return a list of font names instead.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! 👍

@fjvallarino
Copy link
Owner

@klausweiss I'll take a look during the weekend.

What I would check first is if the loading functions are returning a valid status code (i.e., they are not failing to load the embedded binary content). I'd also validate that the file embedding is actually happening. Finally, I'd check the chs bindings (I added a comment there).

@@ -118,6 +119,23 @@ withText t = useAsCString (T.encodeUtf8 t)
withNull :: (Ptr a -> b) -> b
withNull f = f nullPtr

allocCUString :: ByteString -> ((Ptr CUChar, CInt) -> IO a) -> IO a
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klausweiss I'm thinking maybe something like this could work (I have not tested it, and it may not compile as it stands):

withByteString :: ByteString -> (CString -> IO b) -> IO b
withByteString bs = useAsCString bs

{# fun unsafe fmCreateFontMem {`FMContext', withCString*`Text', withByteString*`ByteString'&} -> `Int' #}

Copy link
Contributor Author

@klausweiss klausweiss Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the C API requires it to use Ptr CUChar instead of Ptr CChar, which CString uses. I've made some progress (see this comment and the code I pushed), but that's still not it.

@klausweiss
Copy link
Contributor Author

klausweiss commented Aug 5, 2022

Thanks @fjvallarino!

What I would check first is if the loading functions are returning a valid status code (i.e., they are not failing to load the embedded binary content).

They do. 0, 1, 2, 3, ... for each consecutive call to the C function.

I'd also validate that the file embedding is actually happening.

Confirmed it's working, exactly the same bytes as in the input file.

Finally, I'd check the chs bindings (I added a comment there).

I've made some progress here. I've tried to use what's in nanovg-hs for this bit: https://github.com/cocreature/nanovg-hs/blob/cc8dfa0dc18a0792786c973b4e9a232fa7d3ecfd/src/NanoVG/Internal/FFIHelpers.hs#L24-L26

Now it randomly renders some dots where the text should be on labels:

Screenshot

image

I tried explicitly allocating memory for the bytestring (updated the PR with this code), but the results are same as above - randomly rendered dots.

@klausweiss
Copy link
Contributor Author

klausweiss commented Aug 5, 2022

Ok, I got this. I think it's the nanovg-hs code that might be a bit off.
I modified nanovg-hs to use the same trick with allocating memory for the font on my own to prevent it from being garbage-collected:

copyCUStringLenMemory (from, len) =
  let intLen = fromIntegral len
  in do
    to <- mallocBytes intLen
    copyBytes to from intLen
    return (to, len)

and made the NVGcontext free the font upon destruction. The results are shown in the image below (:

Screenshot

image

I'll open a PR to nanovg-hs to fix this later.

Copy link
Contributor Author

@klausweiss klausweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the nanovg fix has been released, I'm happy to open this PR for review.

I left some comments in places I'm unsure about.

Comment on lines +48 to +57
data FontDef
= FontDefFile
{ _fntFontName :: !Text -- ^ The logic name. Will be used when defining styles.
, _fntFontPath :: !Text -- ^ The path in the filesystem.
}
| FontDefMem
{ _fntFontName :: !Text -- ^ The logic name. Will be used when defining styles.
, _fntFontBytes :: !ByteString -- ^ The bytes of the loaded font.
}
deriving (Eq, Show, Generic)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to use this

data FontDef
  = FontDefFile
    { _fntName :: !Text  -- ^ The logic name. Will be used when defining styles.
    , _fntPath :: !Text  -- ^ The path in the filesystem.
    }
  | FontDefMem
    { _fntName :: !Text         -- ^ The logic name. Will be used when defining styles.
    , _fntBytes :: !ByteString  -- ^ The bytes of the loaded font.
    }
  deriving (Eq, Show, Generic)

but for reasons I don't understand (some lens stuff) I can't:

[ 21 of 104] Compiling Monomer.Graphics.Lens [Monomer.Graphics.Types changed]

.../monomer/src/Monomer/Graphics/Lens.hs:24:1: error:
    • Could not deduce (Applicative f) arising from a use of ‘pure’
      from the context: Functor f
        bound by the type signature for:
                   path :: Control.Lens.Type.Lens' FontDef Data.Text.Internal.Text
        at src/Monomer/Graphics/Lens.hs:24:1-42
      Possible fix:
        add (Applicative f) to the context of
          the type signature for:
            path :: Control.Lens.Type.Lens' FontDef Data.Text.Internal.Text
    • In the expression: pure ((FontDefMem x1_acDU) x2_acDV)
      In an equation for ‘path’:
          path _ (FontDefMem x1_acDU x2_acDV)
            = pure ((FontDefMem x1_acDU) x2_acDV)
      In the instance declaration for
        ‘HasPath FontDef Data.Text.Internal.Text’
   |
24 | makeLensesWith abbreviatedFields ''FontDef
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note this change creates traversals, not lenses, which is something to watch out for, see GHCI:

λ: FontDefMem "name" "Bytes" ^. fontPath 
""

I'm open to alternatives, but given how small these constructors are, I'm fine with leaving as is.

Copy link
Owner

@fjvallarino fjvallarino Sep 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the naming you chose. I guess the error is caused by a previous Lens/Prism generated in https://github.com/fjvallarino/monomer/blob/main/src/Monomer/Lens.hs.


Alternatively, since this constructors are only used in a couple of places, you can use:

data FontDef
  = FontDefFile !Text !Text -- ^ Associates a font name with a filesystem path.
  | FontDefMem !Text !ByteString -- ^ Associates a font name with a font loaded in memory.
  deriving (Eq, Show, Generic)

Then, in NanoVGRenderer, you need to deconstruct:

...
createFont (FontDefFile name path) = VG.createFont c name (VG.FileName path)
createFont (FontDefMem name bytes) = VG.createFontMem c name bytes
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I went with what you suggested. name was needed outside of the createFont function in both NanoVGRenderer and FontManager, so I hand-crafted a HasName instance for FontDef, since it can't be generated anymore due to no record syntax.

Comment on lines 394 to 398
{- |
Alias for 'appFontDefFile' for backwards compatibility.
-}
appFontDef = appFontDefFile
{-# DEPRECATED appFontDef "Use appFontDefFile directly" #-}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go down this route, we'll also need to update the examples and docs code to use appFontDefFile instead of appFontDef.

@klausweiss klausweiss marked this pull request as ready for review August 21, 2022 12:58
@Dretch
Copy link
Contributor

Dretch commented Sep 10, 2022

I had a play around locally and it works for me 😄

I wonder if there should be a pointer in the docs to https://hackage.haskell.org/package/file-embed-0.0.15.0/docs/Data-FileEmbed.html or similar (this is what I used and I assume this is intended).

Also there is appWindowIcon that might benefit from something similar, to enable fully self-contained binaries.

@fjvallarino
Copy link
Owner

@klausweiss I'm really sorry I missed this. I didn't see you had switched it to a PR ready for review.

I added a couple of comments. The PR looks great!


{-|
Available fonts to the application, loaded from the bytes in memory.
Specifying no fonts will make it impossible to render text.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe adding a short example about how to embed a font would be a good idea (the text I added is just a sample, change it as you see fit).

{-|
Available fonts to the application, loaded from the bytes in memory. 
Specifying no fonts will make it impossible to render text.

One use case for this function is to embed fonts in the application, without the need to distribute the font files. The [file-embed](https://hackage.haskell.org/package/file-embed-0.0.15.0/docs/Data-FileEmbed.html) library can be used for this.
@
appFontDefMemory "memoryFont" $(embedFile "dirName/fileName")
@
-}
appFontDefMemory :: Text -> ByteString -> AppConfig e

Copy link
Contributor Author

@klausweiss klausweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klausweiss I'm really sorry I missed this. I didn't see you had switched it to a PR ready for review.

@fjvallarino no worries, I'll make sure to mention you next time I do this (:

Also there is appWindowIcon that might benefit from something similar, to enable fully self-contained binaries.

@Dretch sounds good to me, happy to do this in a separate PR. Will need a couple days to find time for that though.

Comment on lines 394 to 398
{- |
Alias for 'appFontDefFile' for backwards compatibility.
-}
appFontDef = appFontDefFile
{-# DEPRECATED appFontDef "Use appFontDefFile directly" #-}
Copy link
Contributor Author

@klausweiss klausweiss Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good (re deprecation).

Renamed appFontDefMemory back to appFontDefMem.

Comment on lines +48 to +57
data FontDef
= FontDefFile
{ _fntFontName :: !Text -- ^ The logic name. Will be used when defining styles.
, _fntFontPath :: !Text -- ^ The path in the filesystem.
}
| FontDefMem
{ _fntFontName :: !Text -- ^ The logic name. Will be used when defining styles.
, _fntFontBytes :: !ByteString -- ^ The bytes of the loaded font.
}
deriving (Eq, Show, Generic)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I went with what you suggested. name was needed outside of the createFont function in both NanoVGRenderer and FontManager, so I hand-crafted a HasName instance for FontDef, since it can't be generated anymore due to no record syntax.

rev = "cc8dfa0dc18a0792786c973b4e9a232fa7d3ecfd";
sha256 = "0vvj4l2dfjqspl80bwq4vkcql5p7s5a7l1cv7vajkak0vn1ryy70";
rev = "283516a337c4d3606555728df0a39294e78a7cdf";
sha256 = "1vggli76ijqmx633yix4yg5dv58a14p8561jnprjc061sjngphzv";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity: computed with

nix-prefetch-url --unpack https://github.com/cocreature/nanovg-hs/archive/283516a337c4d3606555728df0a39294e78a7cdf.tar.gz

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I had not yet needed to update Nix dependencies; it's good to know.

name = fontDef ^. fontName
createFont FontDefFile{} = fmCreateFont ctx name (fontDef ^. fontPath)
createFont FontDefMem{} = fmCreateFontMem ctx name (fontDef ^. fontBytes)
name = fontDef ^. L.name
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Just to be clear: if you prefer your previous version with the records, go ahead with it; it works well for me.

@fjvallarino
Copy link
Owner

@klausweiss you did mention switching from Draft to Ready for Review in a previous comment, I just missed it :)

All looks good for me. I added a comment about choosing between your original design for the FontDef type or the alternative I proposed and, based on what you decide, I'm ready for merging.

Thanks!

@klausweiss
Copy link
Contributor Author

All looks good for me. I added a comment about choosing between your original design for the FontDef type or the alternative I proposed and, based on what you decide, I'm ready for merging.

Ok. I'll revert the change for the sake of not having to hand-craft the lens instance and am happy to merge afterwards. Thanks for the assistance!

@fjvallarino fjvallarino merged commit 619ac2d into fjvallarino:main Sep 11, 2022
@fjvallarino
Copy link
Owner

Thank you for the patience!

@klausweiss klausweiss deleted the feature/appFontDefMem branch September 11, 2022 10:31
@klausweiss
Copy link
Contributor Author

Also there is appWindowIcon that might benefit from something similar, to enable fully self-contained binaries.

@Dretch sounds good to me, happy to do this in a separate PR. Will need a couple days to find time for that though.

Sorry @Dretch, I didn't get round to doing this eventually. It's free for anyone to pick up.

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.

None yet

3 participants