robotools / compositor

A basic OpenType GSUB and GPOS layout engine.
MIT License
31 stars 11 forks source link

somehow the XP and YP are different compared a pre LayoutEngine version of compositor #14

Open typemytype opened 8 years ago

typemytype commented 8 years ago

I was quickly testing a basic mark feature

The current version:

screen shot 2016-01-30 at 20 42 42

an older version:

screen shot 2016-01-30 at 20 46 37

this is tested with the current version of FeaturePreview.roboFontExt

The new version also leaves traceback on non existing glyphs.

Traceback (most recent call last):
  File "/Users/frederik/Documents/dev/tmt/vanilla/Lib/vanilla/vanillaEditText.py", line 11, in controlTextDidChange_
    self.action_(notification.object())
  File "/Users/frederik/Documents/dev/tmt/vanilla/Lib/vanilla/vanillaBase.py", line 210, in action_
    self.callback(sender)
  File "/Users/frederik/Documents/dev/tmt/defconAppKit/Lib/defconAppKit/controls/glyphSequenceEditText.py", line 20, in _inputCallback
    self._finalCallback(self)
  File "featurePreview.py", line 91, in glyphLineViewInputCallback
  File "featurePreview.py", line 189, in updateGlyphLineView
  File "/Users/frederik/Documents/dev/typeSupply/compositor/Lib/compositor/font.py", line 157, in process
    glyphRecord.advanceWidth += self[glyphRecord.glyphName].width
  File "/Users/frederik/Documents/dev/typeSupply/compositor/Lib/compositor/font.py", line 130, in __getitem__
    glyph = self.glyphSet[name]
  File "/Users/frederik/Documents/dev/fonttools/Lib/fontTools/ttLib/__init__.py", line 701, in __getitem__
    return self._glyphType(self, self._glyphs[glyphName], self._hmtx[glyphName])
  File "/Users/frederik/Documents/dev/fonttools/Lib/fontTools/cffLib.py", line 532, in __getitem__
    charString = self.charStrings[name]
KeyError: 'g'
typesupply commented 8 years ago

Any GPOS support beyond single and pair positioning is almost certainly broken. It's weird that the two versions give different results. I don't recall changing anything in the core. Are these from the same binary?

KeyError

Yeah. This is a feaLib issue. I'm going to open a PR for that when I have a chance. It needs to raise a FeaLibError with a note about the unknown glyph.

typemytype commented 8 years ago

This is not really a feaLib issue as I was just reading binaries files.

I fixed it by returning the fallback glyph whenever a glyph is requested that does not exist maybe I have to fork-patch and pull those little changes

gr Frederik www.typemytype.com

On 31 Jan 2016, at 02:27, Tal Leming notifications@github.com wrote:

Any GSUB support beyond single and pair positioning is almost certainly broken. It's weird that the two versions give different results. I don't recall changing anything in the core. Are these from the same binary?

KeyError

Yeah. This is a feaLib issue. I'm going to open a PR for that when I have a chance. It needs to raise a FeaLibError with a note about the unknown glyph.

— Reply to this email directly or view it on GitHub https://github.com/typesupply/compositor/issues/14#issuecomment-177353408.

typesupply commented 8 years ago

Got it. I'll fix it.

I've found where the value difference is happening. I'm trying to figure out a way to fix it now. (It's a chicken or egg issue.)

typesupply commented 8 years ago

So, here's what is happening on the value difference. Previously, the glyph records resulting from GSUB processing had their advance width set before processing GPOS. See here. This advance width value is used internally to calculate x placement values in some of the GPOS subtable types. For example.

The issue that I'm stuck on is that the new LayoutEngine object doesn't know anything about glyphs and so it therefore doesn't have an advance width to preset before processing GPOS. I can modify the LayoutEngine object to have something like a hmtx table. However, I'm not certain that my GPOS code is correct (for subtable types beyond basic pair positioning), so the first thing I want to do is get that working with a high degree of certainty. I'll open a new issue for that and then come back to this.

typemytype commented 8 years ago

the layout engine could call willBeginProcessGSUB didProcessGSUB and the same for GPOS

the font object could then add the overwrite didProcessGSUB and adjust the advanceWidth values of the glyph records

gr Frederik www.typemytype.com

On 01 Feb 2016, at 05:23, Tal Leming notifications@github.com wrote:

So, here's what is happening on the value difference. Previously, the glyph records resulting from GSUB processing had their advance width set before processing GPOS. See here. https://github.com/typesupply/compositor/blob/db296a7ec7d0b09b58791231f356b511b8604f90/Lib/compositor/__init__.py#L211 This advance width value is used internally to calculate x placement values in some of the GPOS subtable types. For example. https://github.com/typesupply/compositor/blob/master/Lib/compositor/subTablesGPOS.py#L363 The issue that I'm stuck on is that the new LayoutEngine object doesn't know anything about glyphs and so it therefore doesn't have an advance width to preset before processing GPOS. I can modify the LayoutEngine object to have something like a hmtx table. However, I'm not certain that my GPOS code is correct (for subtable types beyond basic pair positioning), so the first thing I want to do is get that working with a high degree of certainty. I'll open a new issue for that and then come back to this.

— Reply to this email directly or view it on GitHub https://github.com/typesupply/compositor/issues/14#issuecomment-177755639.

typesupply commented 8 years ago

the layout engine could call willBeginProcessGSUB didProcessGSUB and the same for GPOS

the font object could then add the overwrite didProcessGSUB and adjust the advanceWidth values of the glyph records

All GPOS processing may need the advanced width set for proper calculation in the complex subtable types. So it can't just be in the subclass.