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

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

Open
typemytype opened this issue Jan 30, 2016 · 6 comments

Comments

@typemytype
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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 #14 (comment).

@typesupply
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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 #14 (comment).

@typesupply
Copy link
Member

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.

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

No branches or pull requests

2 participants