robotools / defcon

A set of UFO based objects for use in font editing applications.
MIT License
66 stars 36 forks source link

Point Observation #267

Open typesupply opened 5 years ago

typesupply commented 5 years ago

Almost all objects in defcon participate in the notification system (which is based on the observer pattern). Points are the main exception and it has been a problem for years. Perhaps someone could solve this.

The Problem

When you make a change to a Point, nothing is informed about the change to the font. For example:

contour = glyph[0]
point = contour[0]
point.x = 100

In the current version of the Point object, the contour will not receive any notification that point has changed. This is fine as long as the high-level code is diligent about notifying contour on it's own. In fact, if you call contour.move, that's what Contour does internally. However, if the high-level code does not do the notification work, things get out of sync. This sucks because defcon has a "it just works" model and this doesn't just work.

Some History

When I wrote defcon, Points were just like every object in that they tied into the overall notification center. Any change made to a Point would trigger a Point.Changed notification through the shared contour-glyph-font notification system. As best as I can recall, this created some significant problems due to the number of points in a font:

  1. Loading a UFO was slow because each Point object in the GLIF being read had to go through the full process of joining the notification system.
  2. Moving the contents of a glyph (as in the case of setting a left margin) would cause sum([len(contour) for contour in glyph]) Point.Changed notifications to be posted and this was a serious performance drag.
  3. A substantial amount of memory was required for all of the points in the notification system.

I removed Points from the notification system because I couldn't find a way to solve it. It has bugged (no pun intended) me since.

Possible Solutions

A lot has changed since the early days of defcon. We have better control for holding and muting notifications. Many parts of defcon have been optimized. Hardware has improved.

These are the ideas that I've had for trying to solve the problem:

  1. Put Points back in the notification system and see what happens. Maybe hardware improvements have improved in the last 13 years.
  2. Create a weak reference between points and their parent contours and use that to notify the contour that a point changed.
  3. Create a separate, very light notification system just for communication between points and a contour rather than using the notification dispatcher that is shared across a font. (This shared dispatcher itself is an optimization from the early days of defcon. Creating many dispatchers was expensive.)
  4. Find an observer pattern replacement that scales well.
  5. ??????????

I have no idea if these will work and I don't have time to take on the issue myself. If someone out there wants to give it a shot, please do. Test it by throwing some big UFOs at it and moving lots of points around. I'll stay subscribed to this issue if you want to discuss.

typesupply commented 5 years ago

I made a simple, unoptimized test to see what would happen if Point became fully observable. This is, I think, the slowest possible way to solve this. The results were surprising:

test font contains 730 glyphs
load current: 5.917273044586182
load observable: 6.746060848236084
move current: 0.30922865867614746
move observable: 0.7738111019134521

Loading a font with observation in place only increased the load time by approximately 17%. That's not great, but it is much faster than what I recall from 13 years ago. Moving all of the points in a glyph is slower, but it's still pretty fast and that can be optimized.

Note: I didn't look at memory footprint or anything else.

import time
import weakref
from defcon.objects.base import BaseObject
from defcon import Font, Contour, Point

class HackedContour(Contour):

    def insertPoint(self, index, point):
        point._contour = weakref.ref(self)
        return super(HackedContour, self).insertPoint(index, point)

class ObservablePoint(Point, BaseObject):

    changeNotificationName = "Point.Changed"
    _contour = None

    def _get_font(self):
        contour = self.contour
        if contour is not None:
            return contour.font
        return None

    font = property(_get_font)

    def _get_contour(self):
        if self._contour is not None:
            return self._contour()
        return None

    contour = property(_get_contour)

    # ------------------
    # Property Overrides
    # ------------------

    def _get_segmentType(self):
        return super(ObservablePoint, self)._get_segmentType()

    def _set_segmentType(self, value):
        super(ObservablePoint, self)._set_segmentType(value)
        self.dirty = True

    segmentType = property(_get_segmentType, _set_segmentType)

    def _get_x(self):
        return super(ObservablePoint, self)._get_x()

    def _set_x(self, value):
        super(ObservablePoint, self)._set_x(value)
        self.dirty = True

    x = property(_get_x, _set_x)

    def _get_y(self):
        return super(ObservablePoint, self)._get_y()

    def _set_y(self, value):
        super(ObservablePoint, self)._set_y(value)
        self.dirty = True

    y = property(_get_y, _set_y)

    def _get_smooth(self):
        return super(ObservablePoint, self)._get_smooth()

    def _set_smooth(self, value):
        super(ObservablePoint, self)._set_smooth(value)
        self.dirty = True

    smooth = property(_get_smooth, _set_smooth)

    def _get_name(self):
        return super(ObservablePoint, self)._get_name()

    def _set_name(self, value):
        super(ObservablePoint, self)._set_name(value)
        self.dirty = True

    name = property(_get_name, _set_name)

    def _get_identifier(self):
        return super(ObservablePoint, self)._get_identifier()

    def _set_identifier(self, value):
        super(ObservablePoint, self)._set_identifier(value)
        self.dirty = True

    identifier = property(_get_identifier, _set_identifier)

# ----
# Test
# ----

def loadFont(path, pointClass):
    contourClass = None
    if pointClass is not None:
        contourClass = HackedContour
    font = Font(
        path=path,
        glyphContourClass=contourClass,
        glyphPointClass=pointClass
    )
    for layer in font.layers:
        for glyph in layer:
            for contour in glyph:
                for point in contour:
                    pass
    return font

def move(font):
    for layer in font:
        for glyph in layer:
            glyph.move((10, 10))

def loadTest(path, pointClass=None):
    font = loadFont(path, pointClass) # make sure module loading isn't factored in
    loops = 5
    start = time.time()
    for i in range(loops):
        loadFont(path, pointClass)
    end = time.time()
    elapsed = end - start
    return elapsed

def moveTest(path, pointClass=None):
    font = loadFont(path, pointClass)
    loops = 5
    start = time.time()
    for i in range(loops):
        move(font)
    end = time.time()
    elapsed = end - start
    return elapsed

path = '/path/to/a.ufo'

f = Font(path)
c = sum([len(layer) for layer in f])
print("test font contains %d glyphs" % c)

t = loadTest(path)
print("load current:", t)
t = loadTest(path, ObservablePoint)
print("load observable:", t)

t = moveTest(path)
print("move current:", t)
t = moveTest(path, ObservablePoint)
print("move observable:", t)
typesupply commented 5 years ago

(Hey, @typemytype and @justvanrossum. I'm pinging you with this in case you want to follow along or contribute.)

justvanrossum commented 5 years ago

I don't know. I'm skeptical to enroll Point objects into the notification system, even even the slowdown seems minor. But then again, I'm skeptical about Contour objects being observable. I'll not be a great help I'm afraid.