Closed rogermc2 closed 6 years ago
That'll take some time to review ;). Does this code still use parts of FTGL or does it solely depend on FreeType?
It depends only on FreeType. (Although i did copy the encoding enumeration from your FTGL, but have not actually used it yer,) I expected that reviewing it will take substantial effort which I will appreciate and hope that it will be worthwhile. I look forward to you advice regarding improvement. In the mean time, I'm returning to the next stage of producing a next level example based on a learnopengl text rendering example.
I would like to have the FT stuff in a separate project, not in the examples. Can you:
opengl-freetype.gpr
similar to opengl-ftgl.gpr
and the others.FT
as the root package and make the other packages children of that (e.g. FT.Glyphs
instead of FT_Glyphs
). The root package may be a good place for types like FT_Memory
and FT_Stream
(also drop the FT_
prefix from those, they are in a package and thus do not need a namespace prefix).src/freetype
.FT.API
and drop the FT_
from their names (just like the imported OpenGL functions do not have the gl
prefix).Some suggestions from a quick review:
Get_
prefix from functions like Get_Bitmap
(in the public packages, not the API imports). Non-boolean functions should use nouns as names according to the Ada Style Guide.FT_Char_Map
is a System.Address
?). Drop the FT_
prefix (see above).Unsigned_Char_To_Address
is an implementation detail and should be moved to the package body.Print_Bitmap
should be Print_Bitmap_Metadata
since it does not actually print the bitmap. Same for Print_Character_Data
.pragma Pure;
to packages if possible, else pragma Preelaborate;
if possible.Thanks for these which I'll implement as soon as possible. I'll need to study up on the use of the pragmas.
I've incorporated the recommended changes as best I can. I've now started on a string example and I am finding the need for many changes so its probably best to postpone further review until I've got that running. In view of this I'm adding the string example to this branch to ease development.
Development of my freetype Ada implementation has now stabilized so its review can be continued. 've now moved the string example to another branch to ease review of this PR.
What is the status of this PR? I cannot review it as long as you keep pushing commits. Are you still working on stuff? If so, please comment here when it's ready (and afterwards, do not push new commits).
OK. All freetype_gl examples development frozen. No more pushes to freetype_gl examples unless changes requested. Please review.
I just updated to OSX 10.13 High Sierra following which this and other examples involving freetype failed to work. The latest commits reflect my attempts to fix the problem. In doing so I discovered an apparent error in GL.API.spec hence a commit for that. I now suspect the problem is with the High Sierra upgrades freetype framework as the failing routine appears to be FT_New_Face. I'm now investigating this.
I have confirmed that the problem is with High Sierra by restoring my second (laptop) computer to Sierra, checking that the restored version of ftgl_shader_example works then pulling the latest remote version and checking that it still works.
have you tried linking to a homebrew-supplied freetype instead of the one from the system?
I tried homebrew freelib 2.8 then did an upgrade which produced 2.8.1 for high_sierra. The program still failed with both versions of freelib producing the same error. Under Sierra, all my branches run OK using the latest pulls from origin.
I needed to resolve some conflicts after regenerating the API because of changes I did for Windows compatibility. Will probably need to generate again with your additions before merging, please do not commit anything to this branch before I'm done.
I pushed some improvements to make the interface cleaner. I hope that does not break too much on your side. There are some things I want you to do / answer before merging:
exception when others => raise FreeType_Exception
, please add the name of the original exception to the message. You can retrieve that with Ada.Exceptions.Exception_Name
. This way, a user can see what exactly went wrong.FT.Interfac
? I know that interface
is a reserved name, but I do not really get why those things are not directly in FT
. the package Interfac
does not give any meaningful subcategory.Errors.Error_Code
while others raise an exception. Is there a reason for this not to be consistent?Ah yes, and please replace /System/Library/Fonts/Helvetica.dfont
in the example with a path to a file that is commited into the repository. Use some font with an open source license.
Thanks for the changes to FT Interface. Changed example font to Arial.ttf. Is this OK? Since pulling your latest changes, the Open Sierra problem also occurs with Sierra for this branch!
FT.Interfac originates from the original freetype_freetype I think. My first attempt to integrate it into FT failed with lots of circular dependencies. I've changed its name to FT.Freetype to reflect this until I can investigate incorporating it into FT.
Copyright messages added. The Errors.Error_Code returns originate from the C freetype functions. Should I raise exceptions in the API for these?
My point about the font was that it is an absolute path to your system. Someone on Windows does not have that path in their system, nor does someone on Linux. You need to download some open source font and commit it into the repository, and then change the path to be relative to the executable pointing to that font. A good choice would probably be Google's Noto fonts.
Since pulling your latest changes, the Open Sierra problem also occurs with Sierra for this branch!
Not good, will investigate. What I changed is the time when the OpenGL dynamic functions are loaded. Previously, GL.Init
was called from within Glfw.Init
which did not work on Windows. The Windows documentation said that getting those function pointers only works when an OpenGL context has already been created and marked as current. Thus, I moved GL.Init
to the constructor of a Windows (a Window includes an OpenGL context). If that now causes problems on macOS, I need to add platform-specific code. I already upgraded to High Sierra, so I'll have a look there.
Should I raise exceptions in the API for these?
Well, if you raise the same exception for all possible errors, that would certainly be a loss of information. But you can, directly in FT
, declare exceptions for each subcategory, like this:
General_Error : exception;
Glyph_Character_Error : exception;
Use _Error
instead of _Exception
as suffix like the Ada standard library does. Then, when you encounter an error, do:
case Code is
when Errors.Generic_Errors => raise General_Error with Errors.Description (Code);
when Errors.Glyph_Character_Errors => raise Glyph_Character_Error with Errors.Description (Code);
...
end case;
You can put that into an own procedure similar to Raise_Exception_On_OpenGL_Error
.
Thanks for this info, particularly regarding the error handling which I am now working on. A weird thing happened. One of my High Sierra branches worked OK today! But the others still fail. I've no idea what I did to get it working!
I've put a note_fonts directory in the examples directory. I found it useful to put "file not found" into Setup_Font. This suggests that it would be good to have this in FT.Freetype.New_Face. However, Ada.Directories is not pre elaborated so my method doesn't work. Can you suggest a way of implementing a "file not found" test in New_Face. The ftgl_shader_example is again working on my Sierra machine. I've no idea what fixed it. Good News! It also works under High Sierra.
I've still to finish implementing your exception handling recommendations.
I got your code to work under macOS High Sierra without any modifications. The errors seems to be not always reproducible.
For file not found, I'd just check the error code returned by FreeType. I'd assume it returns Cannot_Open_Resource
if the file does not exist or is not readable.
As for the package names, I think you can rename the FT.Freetype
package to FT.Faces
since most functions in there seem to deal with faces. Move Init_FreeType
to FT
and rename it to Init
(it is obvious that it initializes Freetype as it is part of that package). Since you use the types in FT.API
in the public interface, do not put them in FT.API
. Put them where they belong (either in FT
or in FT.Faces
) and make them private. Make the package FT.API
private since it contains stuff the user should not need to see. With the types in place, it should be possible to move all subprogram declarations from FT.API.Freetype
to FT.API
.
OK. Good advice. Thanks. As long as I don't run into circular references!
Just so that you're aware, I decided to publish v0.5 just now without the freetype stuff. The reason is that a lot has happened since the last release and I wanted to have a new one now that things are working pretty well (fixed Windows stuff earlier today).
Since this PR will still take some time, I decided to move it to the next release. Hopefully, that one will not be that far in the future :).
After your current PRs are done, I plan to move development to a develop
branch as most git repos do. I did not do this until now because there was too little activity, but that has changed. Please wait with new PRs until I have finished with that.
OK. Sorry to not getting around to recommend routines from Common. In particular, the projection etc. transforms.
I've got most things done but can't figure out how to fix Face_Ptr
.
If I move it into FT.Faces
I get circular dependencies. If I make it private in FT.API
I get a problem with FT.Faces.Face
when converting it to an Object_Pointer
.
I have a similar problem with Glyph_Slot_Ptr
.
What's the circle introduced when moving Face_Ptr
to FT.Faces
? It's not obvious to me.
All occurrences I see where FT.API.Face_Ptr
is used (e.g. FT.Glyphs
) already depend on FT.Faces
anyway. By removing the types from FT.API
, you do not need to with it in specifications anymore and can move all with FT.API;
clauses into the bodies, breaking cycles that stem from FT.API
needing to depend on FT.Faces
.
OK. Many thanks.
Moving the with FT.API;
into the Faces body was the thing I missed.
But, I still have a problem with private types Face_Ptr
and Glyph_Slot_Ptr
when converting to Object_Pointer
.
For example, in FT.Glyphs.Bitmap
, `Glyph_Pointer := To_Pointer (System.Address (Glyph_Slot))' which complains about invalid conversion of private type.
Do away with the conversions. Glyph_Slot_Ptr
should be a access Glyph_Slot_Record
in the first place. To ensure C compatibility, use pragma Convention (C, Glyph_Slot_Ptr)
.
Does this mean that I should change other system.address types to access types? I'll try, It seems that it would be much better in many ways. I've never liked using system.address which particularly because it is weak typying. I got the original system.address bindings from the automatic GNAT binding generation tool. This answers a question that I always had (in the first place); how to relate the addresses to the objects that they reference.
The High Sierra problem was caused by Apple removing Helvetica.dfont and many other dfonts! This problem would be indicated now as we are using Error Descriptions. It would be nice to be able to display a warning dialog for such problems.
Typically, you use System.Address
for C bindings only when you do not need to access the underlying object. Then it's perfectly fine to derive a private type from System.Address
and use that.
Glad to see the High Sierra problem resolved. Since I already got your code to work on Windows 10, your code does not seem to contain anything platform-specific. We are on a good way.
I've completed changing System.Address to access types wherever I found possible.
Some minor things that could be improved:
Bool
subtype (back?) to FT.API
and use the standard Boolean
in the public interface. This is important so that people can use boolean expressions in the place of that parameter. You can then convert the Boolean value to Bool
in the wrapper.FT_
prefixes in the public API. I don't mind them in FT.API
, but the prefix is not needed since we have packages. Prefixes that are still existing are FT_Pos
(might be Position
) and FT_Vector
.FT_Vector
seems unnecessary. Make it public and do away with the Vector_X
and Vector_Y
functions.FT_Bitmap_Size
, FT_BBox
(rename to Bounding_Box
), Size_Metrics_Record
(rename to Size_Metrics
). That would reduce the number of value accessor functions.The API is not yet memory safe – things like correctly calling FT_Done_Glyph
and FT_Reference_Library
/ FT_Done_Library
can be managed automatically with Ada's controlled types. Unless you want to do that yourself, tell me when you're ready with your changes so I can implement that.
My understanding is that FT_Bitmap_Size, FT_BBox , Size_Metrics_Record are effectively part of the Face_Record which "belongs to" the C freetype machine and therefore should not be publicly accessible so I would prefer to keep them private. But if you think its "safe" to make them public I'll do so. Otherwise, I've made all the other changes. I'd prefer for you to attend to the memory safeness.
I've been keeping the other branches updated including the Geometric Algebra branch. I've extended freetype in the String and Geometric Algebra branches to provide a more convenient AP interface and have been testing these branches at each update. Although currently simple the Geometric Algebra use is working to my satisfaction.
I'll have a take at memory safety, including the handling of the internal records.
It is safe to return them from a getter function because as record values, they will be copied, so the user does not get access to any of FreeType's internal values. I think it is nicer for the user to provide one getter which returns the set of values inside the record, but I will do that myself as part of the memory safety restructuring. Hold on.
Ok. Thanks, I'll be interested in your improvements. I agree about providing one getter function per record. Much nicer. Its something I've been struggling with in Geometric Algebra, in the sense of what's the best way to do it, so it will be good to see your approach. In GA I've found the most convenient way has been to code the GA "core machine" in records (good practice I think) and provide the getters as arrays containing the data elements; but it doesn't really seem "right".
I dug a bit into the FreeType documentation. It seems like FT_Init_FreeType
initializes a library that should be destroyed with FT_Done_FreeType
, not with FT_Done_Library
like you do. But documentation is not really clear, so before I change anything: Did you have another source of documentation which you got the idea to use FT_Done_Library
from?
Background is that FT_Library
does have reference-counting built-in, but from what I understand, one needs to use FT_New_Library
instead of FT_Init_FreeType
to be able to use it, which requires building a FT_Memory
struct. This should be doable but will take some of my time.
Well I read the source of FT_Init_FreeType
and it calls FT_New_Library
and does some additional initialization so it seems safe.
I don't remember where I got the idea to use FT_Done_Library from; possibly from freetyp_gl. As I tried FTGL and porting freetyp_gl before deciding to develop directly from FT, my initial design was probably based on these. I'm fairly it wasn't based directly on FT documentation.
This, from freetypt_freetype.h, seems to cover the situation:
Type
FT_Library
Description
A handle to a FreeType library instance. Each library is completely independent from the others; it is the root of a set of objects like fonts, faces, sizes, etc.
It also embeds a memory manager (see @FT_Memory), as well as a scan-line converter object (see @FT_Raster).
In multi-threaded applications it is easiest to use one FT_Library object per thread.
In case this is too cumbersome, a single FT_Library object across threads is possible also (since FreeType version 2.5.6), as long as a mutex lock is used around @FT_New_Face and @FT_Done_Face.
Note
Library objects are normally created by @FT_Init_FreeType, and destroyed with @FT_Done_FreeType. If you need reference-counting (cf. @FT_Reference_Library), use @FT_New_Library and @FT_Done_Library.
Just from a symmetry perspective it looks like, that as I used FT_Init_FreeType , I should have used FT_Done_FreeType, not FT_Done_Library. If you decide that FT_New_Library and associated FT_Memory struct should be used instead of FT_Init_FreeType to provide reference-counting then I will be happy to undertake the needed changes.
I must have made a mistake way back. I've just remembered that the original example I have been porting is from https://learnopengl.com/#!In-Practice/Text-Rendering which initiates with FT_Init_FreeType(&ft) FT_New_Face(ft, "fonts/arial.ttf", 0, &face) and states: Also be sure to clear FreeType's resources once you're finished processing the glyphs: FT_Done_Face(face); FT_Done_FreeType(ft); So, I don't know where the FT_Done_Library came from.
I have a strange problem with the latest version.
It works on one computer but not the other.
Hello_Character returned constraint error: raised CONSTRAINT_ERROR : erroneous memory access
I traced the error to the 'Finalize (Object);' statement in 'FT.Init'.
When I remove this 'Finalize' statement the program works.
I did try restricting calls to 'Finalize' with 'Object.Initialized' but this had no effect.
I have been unable to find a solution to this problem.
I tried implementing the new "finalize" API into my strings example on my "other" computer; the one that doesn't fail with the ftgl_shader_example.
However, the same error seems to be occurring.
I got a deeper error message than the one from the equivalent of ftgl_shader_example's Setup_Font
; that is an error message from FT.Faces.New_Face
:
FT.Faces.New_Face error: Invalid Library Handle
I haven't been able to figure out why the library's handle should be invalid.
This is my first effort at using freetype. It only displays one character; my simplest approach to getting started with a freetype implementation. I look forward to suggestions for improvement.