rougier / freetype-py

Python binding for the freetype library
Other
298 stars 88 forks source link

Fix bindings for FT_Outline #142

Closed takaakifuji closed 2 years ago

takaakifuji commented 2 years ago

Some of the existing Python bindings for FT_Outline didn't work for me. This PR is intended to address those.

HinTak commented 2 years ago

I doubt your change is correct - it appears that you made a mistake in the first commit (https://github.com/rougier/freetype-py/pull/142/commits/cd72982c3ae67ffb1dd92cd8606660101b2f7401) then makes further changes in two more commits to undo your own mistakes.

HinTak commented 2 years ago

The last commit is fine, but I am not convinced about the first two commits. For the time being, I suggest you submit the last commit as an independent pull first.

takaakifuji commented 2 years ago

Sorry for the confusion, but I'm afraid the commit order you're having might be in the opposite order? The first commit is 9054f11, which is the one that only addresses the typo.

face = Face('./Vera.ttf')
face.set_char_size( 4*48*64 )
flags = FT_LOAD_DEFAULT | FT_LOAD_NO_BITMAP
face.load_char('S', flags )
slot = face.glyph
outline = slot.outline
bbox = FT_BBox()
FT_Outline_Get_BBox(ctypes.byref(outline._FT_Outline), ctypes.byref(bbox)) # pass
FT_Outline_Get_BBox(outline._FT_Outline,               ctypes.byref(bbox)) # gets segfault

I'm a bit confused now. In the example above, FT_Outline_Get_BBox() should crash when FT_Outline is not wrapped with byref, and it is indeed wrapped in Outline.get_bbox(). That's why I thought the other methods like FT_Outline_GetInsideBorder(), FT_Outline_GetOutsideBorder(), FT_Stroker_Export(), FT_Stroker_ExportBorder() and FT_Stroker_ParseOutline() should get the same treatment. Let me know if I'm missing something. Maybe I should include an example code for testing.

rougier commented 2 years ago

Test code and/or example would be great.

takaakifuji commented 2 years ago

Added a code that illustrates the scenario this PR is intended to fix.

rougier commented 2 years ago

Thanks. I'm waiting for @HinTak feedback on your explanation.

HinTak commented 2 years ago

@takaakifuji @rougier I get confused myself about it sometimes. Some of the complication is in ft_structs.py. Python values are pointer types, but because of how ft_structs.py it written, it allows you to address the individual fields in a pointer to struct "ptr_to_struct->member" in c as "python_value.member". So whether you do ctypes.byref depends on whether that piece of memory was initiated by python or by c, and whether it is intended to be modified/updated by the c side. Granted some of the older code may be always wrong and has been wrong forever, so I'd suggest adding test cases as you start to use them and find them not working correctly. However, I'd warn against make wholesale changes to make it looks "more uniform". As I said, there are small differences whether the pointer to struct is allocated by python or by c. Does this make sense?

takaakifuji commented 2 years ago

@HinTak Sorry, but I'm not sure I'm seeing your point.

I suppose one of the sources for the confusion here is that FT_Outline is just a struct, while the most of the other handles are pointers. We have no FT_Outline_Rec. In case if you obtain FT_Outline from a glyph, it often should be the one contained in FT_GlyphSlot which is a pointer allocated by FreeType. And FT_Outline is stored as a struct not as a pointer in FT_GlyphSlot_Rec. All of the functions exposed in ftoutln.h accepts FT_Outline * (sometimes with const in front), and that's why I thought they needed ctypes.byref regardless of the ownership, as Outline._FT_Outline is always expected to be a struct.

The test case I added at 5ac6416 should give you an overview of how ctypes.byref works. I don't currently see any scenarios that the change causes issues. Could you have a look if I'm missing something? Thanks for the reviewing.

rougier commented 2 years ago

I'm myself a bit confused but the code looks ok. I propose to merge and react later in case of problems.

HinTak commented 2 years ago

@takaakifuji @rougier there is a python class FT_Outline defined in freetype/ft_structs.py , which is different and confusing enough against the c struct. Most of the time python will auto-convert between the python object (which is a reference type, and corresponds to the c pointer to struct FT_Outline *, rather than the struct itself). The definitive way of telling if entities cross the python - c boundary correctly and whether help is needed/wanted is the set FT2_DEBUG appropriately and see what arrives on the other side.

takaakifuji commented 2 years ago

Thanks. As we haven't defined argtypes for each function (#24 closed without getting merged), I expect that implicit conversion to pointer types wouldn't happen. I'm assuming that's what @HinTak concerns about. And the ptr_to_struct->member example mentioned in another comment doesn't look the case for FT_Outline to me, because the reference will never be stored as a pointer in the whole binding. That's my understanding so far.

This PR contains a code (examples/outline-test.py) to test methods which seem to have never been covered, and it's intended to prevent the segfault which happened in the previous versions. In that sense, even if I'm still missing something I think this will be a step forward anyway :)

rougier commented 2 years ago

I just merge and we'll see if this leads to unforeseen problems Thanks for your contribution.