openframeworks / openFrameworks

openFrameworks is a community-developed cross platform toolkit for creative coding in C++.
http://openframeworks.cc
Other
9.98k stars 2.55k forks source link

add functionality to ofTrueTypeFont #8133

Closed chilina closed 1 month ago

chilina commented 2 months ago

Related forum thread: https://forum.openframeworks.cc/t/oftruetypefont-and-font-collection-ttc-file/43895

I'd like to propose some additional functionality for ofTrueTypeFont. I'm happy to submit a pull request based on any feedback on the following.

Background: A common True Type Font file format is a True Type Collection, with a file extension of .ttc. There are often several font styles stored in a collection, which can be accessed when loading by ofTrueTypeFontSettings::index. ofTrueTypeFont has a private member std::shared_ptr<struct FT_FaceRec_> face. This struct has a member 'num_faces' which stores the number of styles in the file. This value doesn't currently have any getters, and can't be accessed in a child class because it's private.

Having access to the value in 'num_faces' would be helpful. There are maybe 4 different ways to do this; 3 involve modifying OF and 1 is a quick workaround. Any thoughts on the following would be helpful for a pull request.

Option 1: add a public getter ofTrueTypeFont::getIndexSize() to return the value of face->num_faces. The return value would be 0 for nullptr.

Option 2: make std::shared_ptr<struct FT_FaceRec_> face a protected member. This requires the Freetype2 lib includes to use it in a child class, but would allow full access to all of the members of the FTFaceRec struct. Maybe too much access?

Option 3: add a member ofTrueTypeSettings::indexSize to store the value, and add a getter to ofTrueTypeSettings::getSettings() to get it. I haven't tested this one yet. Maybe too much access?

Option 4 (the workaround): no changes to OF. The number of styles in a .ttc file can be found with a do-while loop that loads each one of them with an incremented value for ofTrueTypeSettings::index until one of them fails to load.

artificiel commented 2 months ago

very interesting! nailing the API would be the best way to determine the requirements and decide between your 4 options.

what is the correlation between the current OF "load operation" vs getting the variants info? on the forum you mention OF loads index 0 by default, but do we need to load a font to get access to the variants?

is this feasible?

static std::vector<std::string> ofTrueTypeFont::getVariantsName(of::filesystem::path path_to_ttc);

to inspect and perhaps dynamically choose variants by name? (I presume the variants names are simply text labels, that aim to be coherent by convention, as opposed to a strict enum defined somewhere?)

and for the Settings, we don't want the user to have load twice; more like an interface like this:

ofTrueTypeFontSettings settings("Helvetica", 24);
settings.variant = "Bold";
ttf_.load(settings);

as for sure we want to go by name. so within load(), a loop iterates and hopes to find "Bold" somewhere and load that or default to 0 with a log message?

(and maybe setup() itself can also be adapted for another string param)

would the above cover your use cases? maybe you have other requirements, or maybe there's additional juicy stuff to extract from the FTFaceRec? it could also be exposed in something like ofTrueTypeFontVariantInfo:

static std::vector<ofTrueTypeFontVariantInfo> ofTrueTypeFont::getVariantsInfo(of::filesystem::path path_to_ttc);

in all cases I would not worry about details and access within ofTrueTypeFont itself, as long as the user-facing API is slick (the implementation can change; the API is forever)

chilina commented 1 month ago

or maybe there's additional juicy stuff to extract from the FTFaceRec?

Yes there are a few members in this struct that would be useful. Their names and descriptions are here: https://freetype.org/freetype2/docs/reference/ft2-face_creation.html. One of them is style_name, which is a string for the style (like "Italic", "Bold", etc), which would facilitate searching with a string(s).

and for the Settings, we don't want the user to have load twice

In OF currently, the font does fully load before its FTFaceRec is populated, which involves doing everything needed to use it with .drawString(). There is quite a bit involved. So I do like the idea of this:

it could also be exposed in something like ofTrueTypeFontVariantInfo

Maybe ofTrueTypeFontMetadata? As a struct that mirrors FTFaceRec ? Which populates with .getMetadata() or similar and doesn't require fully loading the font for use.

Maybe a slow-baked rewrite of some aspects of ofTrueTypeFont would be helpful, to add these ideas like "search for style by string" or "what index is currently loaded" or "how many indexes does this font file have" or "find all the fonts in a folder that belong to a particular font family". But that said, moving the struct to protected from private, or mirroring its values somewhere in the class, would allow people to design their own interfaces as needed.

Then, there is also this forum post regarding methods like .drawStringToFit() that could be added to the class at some point: https://forum.openframeworks.cc/t/calculate-letter-spacing-for-oftruetypefont-setletterspacing/42897/4 . I just used this again recently and it works great! And with access to the metadata, we might (maybe) also do things like '.findFontSizeToFit()', which would find the font size with a suitable value for its "char advance" value to fit a width for a given string.

in all cases I would not worry about details and access within ofTrueTypeFont itself

I find this quite helpful and will keep it in mind. I think my plan is to play around with some of these ideas for a while and see if I can take it someplace, and also get better acquainted with the Freetype2 lib. And maybe people will post more suggestions and ideas here in the meantime.

I do sometimes wonder why some things like struct FTFaceRec are private in OF, instead of protected. There may be some great reasons for not exposing it. Its so much easier to derive child classes with only protected members in the parent.

dimitre commented 1 month ago

I personally think they can be changed from private to protected. maybe this is like this just because it was unneeded until now.

artificiel commented 1 month ago

yes for sure, if going protected allows you to experiment in userland, let's do that! my response above is discussing changes/additions to ofTrueTypeFont's API, which can come once you've figured out a good set of tools to integrate.

(drawStringToFit()/drawStringToWidth() plus a couple other methods are in a branch, but a lot of micro-changes broke auto-merge, and also some ambiguities linger on some aspects of the implementation (notable the Settings/setup are messy) so it's sort of in limbo at the moment.)

chilina commented 1 month ago

Hey any help on the following would be awesome! I'm a little tripped up by it:

The following static global function is defined on line 373. Its job is to make some platform-specific tweaks and then use the Freetype library to load a font face:

static bool loadFontFace(const of::filesystem::path& _fontname, FT_Face & face, of::filesystem::path & filename, int index) { ... }

I don't think its called anywhere outside of ofTrueTypeFont, where it is called once by .load(). Could this function potentially move into the class? Alternatively, is there a way to call it from a child class of ofTrueTypeFont? I tried with extern in the child and various declarations but I get a linker error.

I think loadFontFace() belongs in the class.

artificiel commented 1 month ago

indeed, and in general stuff should not float at the top level.

but there are many aspects to ofTrueTypeFont that would beneficiate such cleanups, but they are currently difficult to execute because there is a chicken-and-egg problem to committing code: on one side, small, atomic commits are encouraged in order to facilitate tracking, but larger effort means putting the code in sometimes a broken state as the small commits might depend on each other and function as a group. (and also the merge latency means you may make a pull request now but it gets merged in January; in the meantime in the context of a longer effort it's difficult for you to build upon that code as it's spread over multiple branches).

so i guess i'm saying we should be merging requests to a form of developement branch (either a generic /develop or a specific /feature_truetype), and have small merge/discussions like this on that branch and not on /master. but i don't know if things are setup in a way that makes that practical.

roymacdonald commented 1 month ago

well I think that it is there as static for some reason, maybe because of how it instances the TrueType library, I am not sure but moving it out and making it a class instance function just because does not sound reasonable to me. It is defined in the cpp file so it is not global, thus it makes sense to have it defined there not as a class function because it does not need to be called from anywhere else. It is just a method of tidying up code and encapsulating and probably optimizing? If you want to call from a derived class you would need to make it part of the class or expose it as a global function which I would not recommend. Or just copypaste, since it is a static function it should work if you just copy it.

artificiel commented 1 month ago

(as a side note, i've noticed in older issues that the /develop branch was once there, but sometime around 2015 it sort of disappeared; i haven't seen discussions on why that went out of style)

artificiel commented 1 month ago

@roymacdonald i interpreted the question not vs the static nature but the namespace; reading your comment makes it explicit that there is ambiguity! but it should stay static.

roymacdonald commented 1 month ago

@artificiel I agree that there should be a development or whatever branch in order to try new stuff before merging into master. It is really simple to do such. As for the namespacing of those static functions, these being static is preventing external linkage, so It should not be a problem or necessary to put inside a namespace. Maybe I misunderstood what you mean.

chilina commented 1 month ago

Hey this is helpful and thanks for the replies! I worry about breaking code both in OF and in projects that may have used this function.

@roymacdonald what do you think about making the private members protected? Also @artificiel any thoughts on this?

I'm fine with the static part of loadFontFace(), but it would be nice if it were part of the class. Truetype mentions how only 1 thread can be used with the lib, which is maybe why it's static.

As part of the class, it could be called in a derived class to load some faces (and only faces and not the entire font(s)) to see what styles are present (for instance). Or a folder of font files could be searched for "Italic" with only suitable fonts loaded into a vector.

There is also the option to use it as is and give ofTrueTypeFont a protected .loadFace() that makes a face and returns a shared_ptr (the struct with all the info in it) for use somewhere else. And it could do this without using or otherwise impacting the face member of the class, or the loaded font, the settings, etc.

I have moved it into the class and tested; things do seem to work OK, but I haven't done a ton of testing. Like roymacdonald mentioned, I feel like it might be how it is for a reason.

roymacdonald commented 1 month ago

I see that there is a lot of platform specific stuff in the ofTrutypeFont.cpp file. My best guess is that all these were left there to keep the .h file without any platform specific stuff.

@roymacdonald what do you think about making the private members protected?

In the case of classes made with the initial intention having these extended (such as ofNode) I think it is a bad idea to make any private member to be protected. In the case of ofTrueTypefont, which I believe was not created with the idea of it being extended it might be a more acceptable thing. But in general I think it needs to be analyzed on a member by member basis as some things might be more sensitive than others.

What is it that you want to make protected?

What I've done in the past a lot in similar cases is to just copy the whole class, package as an addon, rename and add the changes I want, test, publish, hopefully have others to use it and eventually see if those changes can go into OF's core.

artificiel commented 1 month ago

@roymacdonald i'd like to bounce back on your agreement about a development branch. there's been discussion in various issues into turning 'git'/nightlies into the "current" OF version, and this of course requires a level of distance between committing new code and "publishing" it to master. but how does one "make it happen"?

there is a kind of leak in the momentum management of development (leading to forum posts such as "is it time to leave OF" and "is OF still relevant"). dynamics of releases is part of that, as well as code style/readability — we are not in the days of trying to lure Processing users by trying to be 1:1 anymore... potential "new users" have a level of litteracy that makes parts of OF look really dated, considering the generic tutorial and materials C++ learners are exposed to in 2024.

i also wrote this a while ago: https://github.com/openframeworks/openFrameworks/discussions/7320 -- it has a wider scope than just a /develop branch but i still agree on what was my perspective then.

(note: this is out-of-scope for this issue; feel free to redirect where you think it is approriate!)

chilina commented 1 month ago

What is it that you want to make protected?

I want access to shared_ptr face, which is private, so I can dereference it and get some info out of it. And a public getter for the (protected) internal settings (by copy not reference) so I can clone them, modify something, and then reload a font with it. I also want to be able to load a font face without loading the whole font, so maybe initLibrary() too and the namespace-less loadFontFace().

What I've done in the past a lot in similar cases is to just copy the whole class, package as an addon, rename and add the changes I want

This is really a fantastic idea. Why didn't I think of this? Lately I've been deriving child classes from OF into a personal add-on for things I constantly use together, like for example an ofImage (as a child), an ofDirectory and an ofParameter to index it, an ofFbo and a shader for cropping, etc. But I couldn't derive from ofTrueTypeFont in a useful way because of the private members.

I've got these changes made locally in ofTrueTypeFontSettings, so maybe that's about the same as creating an addon with a modified copy. I'll just have to remember to re-implement things when I upgrade versions.

(note: this is out-of-scope for this issue; feel free to redirect where you think it is approriate!)

Hey perfectly fine to continue discussing this here, and I can even change the title too you'd like.

roymacdonald commented 1 month ago

I see. I would recommend you to go for the addon. I have tried before having my own branch of OF with my modified version of whichever class, but it becomes annoying when you try to update OF. It is way much easier to do it as an addon. The bare minimum of an addon is just a folder named as the addon and inside of it another folder named src, which holds the h and cpp files. So you just need to make 2 folders copy the files needed into it, change the name of the files and classes and that's it. You can even publish it an see if it gains any more traction. If you check my github, you'll see that I do it a lot. :)

roymacdonald commented 1 month ago

@artificiel I will answer later but I agree a lot with your points. :)

chilina commented 1 month ago

After changing the namespaces of a several classes, structs, and defines, the addon approach has gotten stalled here. Any suggestions would be great! The rendered doesn't know how to draw my class, and I'm not sure how to solve this:

void ofxTrueTypeFont::drawString(const string &  c, float x, float y) const{
    if (!bLoadedOk){
        ofLogError("ofxTrueTypeFont") << "drawString(): font not allocated";
        return;
    }
// No matching member function for call to 'drawString':
    ofGetCurrentRenderer()->drawString(*this,c,x,y);
}

I'm using the programmable renderer if it matters.

Edit:
Actually I think there is a way to just copy the stuff out of the class that I need to use, and then use it as a helper instead as a full copy of ofTrueTypeFont as an addon.

Thanks everyone that has helped with comments! Maybe someday the private members will be protected and ofTrueTypeFont will be more open for deriving child classes from.

artificiel commented 1 month ago

@chilina i don't think making the fields protected was rejected. i think @roymacdonald suggested experimenting in userland within an addon, but if the interface of ofTrueTypeFont is too restrictive to subclass effectively, i don't see why members cannot go protected.

and once you streamline functions and access and the load ordering, i also don't see why the API can't evolve:

ofTrueTypeFontSettings settings("Helvetica", 24);
settings.variant = "Bold";
ttf_.load(settings);

would be a tremendous improvement.

chilina commented 1 month ago

Hey yes I like that approach to the API a lot, and expanding the settings a bit to facilitate seems like it would be easy to do. Maybe if you get a development branch going we can start playing around with this class a bit more. I don't use it that much, but always very glad it's there when I need to use it.

roymacdonald commented 1 month ago

Just make a PR with these changes and we can have it as branch instead other than master. I guess that the copy the whole thing did not work because it needs that call to the renderer, which expects an ofTruetypeFont reference. Thus, the most straightforwards solution is to make some of the ofTrueTypeFont members protected. Thus, @chilina go ahead, make those changes and a PR and we can move it into a development branch. Cheers

chilina commented 1 month ago

Actually I think there is a way to just copy the stuff out of the class that I need to use, and then use it as a helper instead as a full copy of ofTrueTypeFont as an addon. Hey thanks @roymacdonald and I did get a version working that only uses parts of the class (parts not related to rendering). So I think I'll play around with it for a while longer, and see if I have any problem with the Freetype parts.

chilina commented 1 month ago

Hey I'm going to close this and thanks for all the comments! I'll eventually submit a pull request with some ideas, something that would be fun to see in a development branch for a while.