Another Mistake?

Am I losing my edge, if I ever had an edge? I realized something interesting about the evolving design. I decide my edge is no more dull than usual.

In Previous Episodes

We started with components knowing their parent, with an update method, but also with properties to provide the values that child components wanted. That started getting a bit chaotic, to the point that at least one component was asking for its parent’s parent info. And the components were soon caching the values they computed in update, so that they could return results without recomputing.

It seemed that (almost?) all components could be satisfied by knowing two points on their parent, which we called start and finish. And we moved to a scheme where we pass those two values to update, and returned the child component’s own start / finish values from update. When updating a linkage, we just go through the components, passing each one the start / finish from its predecessor, which—for now—is always its parent.

Last episode, we started work on a Linkage object, intended to contain and process a complete linkage of components, embedding all the rolling parameter logic, and intended to be able to apply any function to the components after updating, such as we would want for drawing them. That object presently includes an inner class, Element, containing, thus far, a component, a pointer to its parent, and its most recently updated start / finish values.

But wait! What we had before we switched over to calling update with the start / finish values, was components that had a pointer to their parent and a cached start / finish pair. It looks as if we just moved a perfectly good set of object members outside of the object and put them into another object instead. Is this actually progress?

When I went through the above review in my mind, my first reaction was that this was not progress, and that the changes described above might be a mistake. But now I’d like to explain why I think this is in fact progress.

As things stand now, the components amount to functions with some invariant parameters, reflecting things that don’t change, such as the radius of a crank, and computing their positions based on the positions of things to which they are connected. In a sense, a component is a “closure”, a function some of whose parameters are defined and some of which remain free. ¹ And, as things stand now, the component does not know its parent: all the information it needs is either constants within itself, or parameter info provided to update.,.

The system, whatever it is, does need to know the start / finish for all the components. In the fullness of time, it will draw animations, or export information to another system that animates a “real” linkage. But the individual component does not need to know its own start / finish values. Therefore, I reason, it should not cache them, and someone who does want to know them should cache them. That someone, today, is our nascent Linkage class.

So, there you are. I observe that we’re just moving information out of the component and putting essentially the same information into the Linkage Element, and I tentatively² conclude that that’s OK.

Preparing the Ground

Yesterday I put in elementary Linkage and Element classes. I took a quick look at doing drawing with those classes, and it seemed a bit too tricky. I don’t remember why it seemed that way and it might have just been that I was ready for breakfast. We’ll check that out shortly. But first, I think I’d like to change all the component classes so that they do not have the parent parameter at all. Instead, we’ll represent that information as we set up a Linkage, or manage it directly in classes, until we get a better idea.

I was thinking of proceeding in two phases, first changing the __init__ methods not to save the parent, then running things to see what breaks, and then changing signatures as a second phase. Let’s hang a bit more loose and just look at each component one at a time and see what that one needs to eliminate its parent connection.

FixedPoint has no parent. Nice, done! Wheel saves its parent in _position and there is a test that uses it. I’ll just Change Signature and then run and fix the tests.

I goofed. Undo that and run the tests clean. Yes, one of my tests in TestLinkage is failing. Better deal with that first. Good news: I had rigged it to fail on purpose. Green. Do again.

Yes, one test fails:

    def test_wheel_basics(self):
        loc = vector(2, 3)
        wheel = Wheel(parent=loc, angle_degrees=90)
        assert wheel._angle_radians == math.pi / 2
        assert wheel._position == loc
        assert wheel._angle_radians == 90*math.pi/180.0

No problem, we don’t support position any more. Remove the line. Commit: Wheel does not accept parent.

Now Crank, same drill, Change Signature and test. All still green. Commit: Crank does not accept parent.

PyCharm is really a dream for this sort of thing. It correctly edited four or five files for me, lots of tests. And that is with the cursed “AI” turned off. My hat’s off to JetBrains. Not that I generally wear a hat in the house anyway.

Now ConRod. Same thing. Again green. Similar commit.

Now Piston. A couple of tests fail. Bummer.

class TestPiston:

    def verify(self, piston):
        s = piston._parent.start
        f = piston._parent.finish
        start, finish = piston.update(s,f)
        assert start == piston._parent._finish
        assert start.distance(finish) == pytest.approx(piston._length)
        delta = finish - start
        assert math.atan2(delta.y, delta.x) == pytest.approx(piston._radians)

    def test_basic(self):
        fin = vector(3,7)
        piston = Piston(length=2, angle=0)
        self.verify(piston)

    def test_45(self):
        fin = vector(3,7)
        piston = Piston(length=2, angle=45)
        self.verify(piston)

I think we have to pass in the fin object and use it in the test. Pretty trivial test: it’s tempting to remove it. I’d rather have a trivial test as a placeholder than no test at all.

I use a Change Signature refactoring and two Inline refactorings to get this:

    def verify(self, parent, piston):
        start, finish = piston.update(parent, parent)
        assert start == parent
        assert start.distance(finish) == pytest.approx(piston._length)
        delta = finish - start
        assert math.atan2(delta.y, delta.x) == pytest.approx(piston._radians)

Green. Commit: no components include parent.

I noticed, looking at Piston, that it still caches the start and finish values. Oh. Most of them are doing that, because the drawing code wants those values.

We need to change the drawing methods to accept the start / finish as parameters. That’ll require changes to the two drawing loops, the init one and the drawing one. First the init:

s = f = None
for component in components:
    s, f = component.update(s, f)
    component.init_draw(canvas)

Oh nice. We can just Change Signature on init_draw and change it to use the parameters. This will break things, and we have no tests for the drawing: we just look at it. I’ll tick through each one, changing it.

After just a bit of inserting start, finish parameters and referring to them, drawing is working. Now I can go through and make sure I’m not saving any of those values in my objects.

A bit more tidying and we’re good. I am still caching the canvas objects created in init_draw, so that draw can update them, and it turns out that one object, ConRod, needs the parent’s start value cached, as that is the center of the crank, and we need that to draw the guide line. If we didn’t draw that, we could eliminate that cache.

I’ve decided not to worry about the drawing cache for now, and perhaps forever. We could do something tricky, passing each draw method a little stash object or some such thing but for now things are much nicer. I commit a final version.

Here, just for scanning, so you can see that they’re pretty clean, are the components:

class FixedPoint:
    def __init__(self, position):
        self._position = position

    def update(self, start, finish):
        return self._position, self._position

    def init_draw(self, canvas, _s, _f):
        pass
    def draw(self, canvas, _s, _f):
        pass

class Wheel:
    def __init__(self, *, angle_degrees=0):
        self._cached_line = None
        self._angle_radians = angle_degrees * math.pi / 180.0

    def update(self, start, finish):
        return start, finish + vector(1,0).rotate_xy(self._angle_radians)

    def turn_by(self, degrees):
        self._angle_radians += degrees * math.pi / 180.0

    def init_draw(self, canvas, start, finish):
        radius = 1
        top = start + vector(radius, radius)
        bot = start - vector(radius, radius)
        end = start + vector(radius, 0).rotate_xy(-self._angle_radians)
        canvas.create_oval(bot.x, bot.y, top.x, top.y,
                            outline='green', width=3)
        self._cached_line = canvas.create_line(start.x, start.y,
                                               end.x, end.y,
                                               fill='green', width=3)

    def draw(self, canvas, start, finish):
        canvas.coords(self._cached_line, start.x, start.y, finish.x, finish.y)


class Crank:
    def __init__(self, *, radius, lead_degrees=0):
        self._radius = radius
        self._leadRadians = lead_degrees * math.pi / 180.0

    def update(self, p_start, p_finish):
        dif = p_finish - p_start
        parent_radians = math.atan2(dif.y, dif.x)
        center = p_start
        r = self._radius
        effective_angle = self._leadRadians + parent_radians
        x = r * math.cos(effective_angle) - 0 * math.sin(effective_angle)
        y = r * math.sin(effective_angle) + 0 * math.cos(effective_angle)
        finish = vector(x, y) + center
        return p_start, finish

    def init_draw(self, _canvas, _s, _f):
        pass
    def draw(self, _canvas, _s, _f):
        pass

class ConRod:
    def __init__(self, *, length, piston_angle):
        self._length = length
        self._piston_radians = piston_angle * math.pi / 180
        # mutable graphic support temps
        self._cached_line = None
        self._cached_wheel_center = None

    def update(self, start, finish):
        self._cached_wheel_center = start
        self._start, self._finish = (
            con_rod_ends_complete(
                piston_radians=self._piston_radians,
                wheel_center=start,
                radius=self.crank_radius(start, finish),
                crank_radians=self.crank_radians(start, finish),
                length=self._length))
        return self._start, self._finish

    def crank_radius(self, start, finish):
        return finish.distance(start)

    def crank_radians(self, start, finish):
        diff = finish - start
        return math.atan2(diff.y, diff.x)

    def init_draw(self, canvas, start, finish):
        self._cached_line = canvas.create_line(
            start.x, start.y,
            finish.x, finish.y,
            width=3, fill="red")
        wc = self._cached_wheel_center
        tg = vector(2*self._length, 0, 0).rotate_xy(self._piston_radians)
        canvas.create_line(wc.x, wc.y, wc.x + tg.x, wc.y + tg.y, fill="blue")
        one = vector(1,0).rotate_xy(self._piston_radians)
        half = wc + one + tg * 0.5
        s = 0.1
        canvas.create_oval(half.x-s, half.y-s, half.x+s, half.y+s, fill='red')

    def draw(self, canvas, start, finish):
        canvas.coords(self._cached_line,
                      start.x, start.y,
                      finish.x, finish.y)

class Piston:
    def __init__(self, *, length, angle):
        self._cached_line = None
        self._length = length
        self._radians = angle*math.pi/180

    def update(self, _start, finish):
        my_start = finish
        finish_offset = vector(self._length,0).rotate_xy(self._radians)
        my_finish = my_start + finish_offset
        return my_start, my_finish

    def init_draw(self, canvas, s, f):
        self._cached_line = canvas.create_line(s.x, s.y, f.x, f.y, width=6, fill='green')

    def draw(self, canvas, s, f):
        canvas.coords(self._cached_line, s.x, s.y, f.x, f.y)

ConRod is clearly more complicated than the others, and I haven’t even included the calculation function in the above. Here is it, same as it ever was, in the highly factored version:

def con_rod_ends_complete(*, piston_radians, wheel_center, radius, crank_radians, length):
    effective_radians = rotate_to_horizontal(crank_radians, piston_radians)
    start, finish = con_rod_ends(radius, effective_radians, length)
    sr, fr = rotate_inverse(start, finish, piston_radians)
    return translate_to_wheel_center(sr, fr, wheel_center)

def rotate_to_horizontal(crank_radians, piston_radians):
    return crank_radians - piston_radians

def rotate_inverse(start, finish, piston_radians):
    sr = start.rotate_xy(piston_radians)
    fr = finish.rotate_xy(piston_radians)
    return sr, fr

def translate_to_wheel_center(sr, fr, origin):
    return sr + origin, fr + origin

def con_rod_ends(crank_radius, crank_angle, length):
    # zero-based, horizontal
    crank_offset = vector(crank_radius,0).rotate_xy(crank_angle)
    constrained_end = con_rod_solve(crank_offset, length)
    return crank_offset, constrained_end

def con_rod_solve(crank_offset, length):
    x,y = crank_offset.x, crank_offset.y
    x_projection = math.sqrt(length*length - y*y)
    return vector(x+x_projection,0)

Overall, pretty clean. ConRod not so much, but it involves a lot of math that the others do not need. We might review it and see if there is some way to break it up into sub-components or something. And we might not. It’s working fine and very well tested.

Summary

I am pleased with these changes. The components no longer retain their old values, and we pass the relevant inputs into the various methods that need them, so that the components no longer know their parent: they just receive start and finish values, which are always vectors.

I had thought that we might do a bit more work on the Linkage and Element classes this morning, but this article is long enough, I am ready for an iced chai, and this is a great place to pause. So pause we will.

See you next time!