Report, then Anti-Duplication

I made some changes to the scaling of images. I’ll report on that, but what I really want to do is to eliminate some duplication. I surprise myself.

I found an incantation in Arcade/Pillow that returns the bounding box of a texture that includes transparent pixels. Applied that, didn’t entirely like the result, added a scaling factor to the scale_texture function. Here’s what we’ve got now:

params.py
# dungeon parameters

cell_size = 16

def scale_texture(texture, adjust=1):
    pil = texture.image
    x0, y1, x1, y0 = pil.getbbox()
    x = x1 - x0
    y = y1 - y0
    size = x if x > y else y
    return cell_size / size * adjust

Get the bounding box in left upper right bottom order because that’’s what you get; get the width and height. (Should rename those, will do in a moment.) Get the max as size, scale to that size times the adjustment factor. Same basic scaling as before,, cell_size/size, except that we use the visible size of the texture, and we can adjust it by a factor.

The factor is there because without it, dungeon contents images tend to fill the entire dungeon tile and we want them smaller. So the tiles are scaled at 1, and the contents at 0.5, like this:

class ReceivableContent(Content):
    def draw(self, cx, cy, cell_size):
        texture = self.shape
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

The current settings give me a picture that I nearly like:

Yesterday, when I looked at that, I only saw one thing that I was concerned about: the code that does this includes lots of duplication in the four or five classes we have for content. Today, I see two more things that trouble me. The chest looks a bit small to me, but that might be OK. But to get the Button tile to the right of the dot to be the size of the full tile, I had to set its adjustment factor to 1, while the others are 0.5. Finally—yes, I know that makes three more—the DrawableContent class includes a scale factor creation parameter which is not used.

Now, ab initio I had intended to have a class hierarchy of ContentItems, and to inherit or override actual behavior as needed. I backed away from that idea, because received wisdom is that one shouldn’t inherit actual behavior, my own experience with inheriting behavior is mixed at best, and, most important, it began to look as if single inheritance wouldn’t do the job, as some classes that I could foresee needing looked to want to inherit from more than one of the classes we could imagine.

There may have been a way to avoid that last concern, but the mere prospect was enough to cause me to decide to unwind the hierarchy and deal with the duplication at some future time, possibly never.

Never seems to have arrived, or nearly so.

In the past few days, I have modified those ContentItem classes more than once, with edits that are similar or the same, and only today I noticed that there is a scale factor construction parameter on one of them but not the others.

This is messy, the classes have already shown themselves to be subject to change, and as we add more content, we’re sure to change them and add to them further, exacerbating the situation and also making it worse.¹ So let’s see what we have and what we can do about it. Here are all the existing ContentItem classes. Scan, read, skip, as you see fit. I’ll be scanning, looking to see what’s the same and what’s different.

class Content(ABC):
    @abstractmethod
    def draw(self, cx, cy, size):
        pass
    @abstractmethod
    def interact_with_player(self, dungeon):
        pass
    @abstractmethod
    def placed(self, dungeon):
        pass

class DrawableContent(Content):
    error_texture = ':resources:images/items/star.png'
    def __init__(self, name, resource=None, scale=1):
        self.name = name
        self.scale = scale
        try:
            self.shape = arcade.load_texture(resource)
        except:
            self.name = 'invalid resource: ' + name
            self.shape = arcade.load_texture(self.error_texture)

    def draw(self, cx, cy, cell_size):
        texture = self.shape
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

    def interact_with_player(self, dungeon):
        pass
    def placed(self, dungeon):
        pass

class Button(Content):
    error_texture = ':resources:images/items/star.png'
    def __init__(self, name, resource=None):
        resource = '/Users/ron/Desktop/DungeonTiles/png/objects/floor button/Button (1).png'
        self.name = name
        try:
            self.shape = arcade.load_texture(resource)
        except:
            self.name = 'invalid resource: ' + name
            self.shape = arcade.load_texture(self.error_texture)

    def draw(self, cx, cy, cell_size):
        texture = self.shape
        scale = scale_texture(texture, 1)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

    def interact_with_player(self, dungeon):
        dungeon.publish('button_pressed')

    def placed(self, dungeon):
        pass

class ReceivableContent(Content):
    error_texture = ':resources:images/items/star.png'
    def __init__(self, name, resource=None):
        self.name = name
        try:
            self.shape = arcade.load_texture(resource)
        except:
            print('invalid resource: ' + name)
            self.name = 'invalid resource: ' + name
            self.shape = arcade.load_texture(self.error_texture)

    def draw(self, cx, cy, cell_size):
        texture = self.shape
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

    def interact_with_player(self, dungeon):
        dungeon.receive_item(self)
        dungeon.announce(f'You have received {self.name}!')

    def placed(self, dungeon):
        pass

class SecretKey(ReceivableContent):
    def __init__(self, name='a secret key', resource=None):
        resource = '/Users/ron/Desktop/DungeonTiles/png/objects/Key (1).png'
        super().__init__(name, resource)
        self.visible = False

    def draw(self, cx, cy, cell_size):
        if self.visible:
            super().draw(cx, cy, cell_size)

    def interact_with_player(self, dungeon):
        if self.visible:
            super().interact_with_player(dungeon)

    def placed(self, dungeon):
        self.visible = False
        def callback(event):
            dungeon.announce('A key has appeared!')
            self.visible = True
        dungeon.subscribe_once('button_pressed', callback)

The first thing I notice is that the base class is Content, not ContentItem, so I should call it by its correct name. I also rename the file, which was ‘content_item.py’. All the subclasses are in the same file, at least for now.

Our current classes are Drawable, Receivable, Button, and SecretKey. Drawable items are just drawn and have no other behavior. The skull and bones in the picture above are examples. Receivable content gives itself to Dot if she steps into its square, calling dungeon.receive_item, which puts the item into Dot’s inventory and removes the item.

The Button is an example of some kind of signaling object. When fully implemented, it will publish a message, in this cased button_pressed. Its complete behavior will need to include toggling between pressed and not pressed, changing its display, and sending two different messages. There will probably be other kinds of signals that do not change state or that have more than two states. I think it’s premature to design support for those until we have an example.

The SecretKey is an example of an object that subscribes to a message and makes itself visible and active upon hearing that message. I note another thing I had forgotten, which is that SecretKey still inherits behavior, from ReceivableContent. We’d like not to do that any more.

I think we’ll do that right now. To accomplish that we need to bring down all the code that it is currently using super() for. This is so painful, making duplication. Can we please wait, to see if we can skip messing up this class, and instead go directly to the new scheme?

OK, we’ll try.

Let’s look at just one case of duplication and see how we could remove it.

class DrawableContent(Content):
    def draw(self, cx, cy, cell_size):
        texture = self.shape
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

class ReceivableContent(Content):
    def draw(self, cx, cy, cell_size):
        texture = self.shape
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

Those look pretty similar to me. The code in SecretKey is similar but with a scale factor of 1, not 0.5.

Arrgh. I notice that DrawableContent has that scale parameter and main is actually setting it to 0.5. The “decision” at that point must have been that we would have some high-level way of associating a scale factor with a texture we want to use as content.

I suspect that we need to make more duplication before we fix this duplication. Maybe not.

I am thinking about trying a “Mixin” approach here. The idea will be to build specific classes, typically with just one method, like draw, with different implementations. Then we have our class inherit from the Mixin classes that define its behavior.

A related approach is the Strategy Pattern. Here, again, we would have a single-method Strategy class for each kind of draw we do. We might have methods in the class to set_draw_strategy, and we could call them when we create the instances, or in the class constructor. To me, that’s more suitable for a class that might change strategies in operation.

Either way, we have single-method classes defining behavior. In the one case, we inherit the behavior, which is less bad because it’s on purpose, I guess. In the other case we would instantiate the strategy, and save it in a member variable.

You know what? We’ll try both ways to see what we like.

It seems to me that much the same code is used in the Mixin/Strategy class.

Unfortunately, we’re dealing with drawing code, so we can’t readily test it. I’ll make sure I’m om a clean repo and just try to spike it in.

Mixin

Here’s the Mixin style.

class NormalDrawing:
    def draw(self, cx, cy, cell_size):
        texture = self.shape
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

class DrawableContent(NormalDrawing, Content):
    # no draw method

One objection one could have to this is that PyCharm correctly realizes that NormalDrawing has no shape member and squiggles that line. I discover that PyCharm can be asked to ignore missing attributes. That may or may not be a good thing.

I notice that PyCharm sees the duplication of this drawing code elsewhere, telling me that I can do this:

class ReceivableContent(NormalDrawing, Content):

That works and all displays just fine.

Strategy

I rather like that. But I promised to try the more strategy-like approach, so I’ll reset and go for that.

class NormalDrawingStrategy:
    def draw(self, texture, cx, cy, cell_size):
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

class DrawableContent(Content):
    def __init__(self, name, resource=None, scale=1):
        self.name = name
        self.scale = scale
        self.drawing_strategy = NormalDrawingStrategy()
        ...

    def draw(self, cx, cy, cell_size):
        self.drawing_strategy.draw(self.shape, cx, cy, cell_size)

Note that since we instantiate the strategy class, it didn’t know texture, and so I passed it to the draw. That works, but so would this:

class NormalDrawingStrategy:
    def __init__(self, texture):
        self.texture = texture

    def draw(self,  cx, cy, cell_size):
        texture = self.texture
        scale = scale_texture(texture, 0.5)
        arcade.draw_texture_rect(
            texture,
            arcade.XYWH(cx, cy, texture.width, texture.height).scale(scale)
        )

class DrawableContent(Content):
    error_texture = ':resources:images/items/star.png'
    def __init__(self, name, resource=None, scale=1):
        self.name = name
        self.scale = scale
        try:
            texture = arcade.load_texture(resource)
        except:
            self.name = 'invalid resource: ' + name
            texture = arcade.load_texture(self.error_texture)
        self.drawing_strategy = NormalDrawingStrategy(texture)

    def draw(self, cx, cy, cell_size):
        self.drawing_strategy.draw(cx, cy, cell_size)

OK, reset again.

Reflection

To me, the Strategy approach is more “acceptable”, in terms of received and personal wisdom about inheritance. And it does offer one possible advantage: an object’s strategy could be changed at run time, whereas the Mixin approach is settled at compile time. We have no reason to do that at present, particularly with content.

If we were talking about dungeon denizens here, we could imagine that they can become pacified or angry, and rather than provide them with conditional code, we could just substitute the AngryStrategy for the PeacefulStrategy and they’d all get mad.

Now we could do a strategy swap with the SecretKey. Its default strategy could be no drawing and no interaction, and upon hearing the button message, it could swap in NormalDrawingStrategy and ReceivableInteractionStrategy.

I think that if we were to do that, there would be no conditionals in SecretKey: all its behavior would come from plugging in different behavior strategies.

I’m glad we had this little chat. I’ve decided, for values² of “decided”, that we’ll follow the Strategy path, not the Mixin path.

Summary

I’m pleasantly surprised. Actually doing the thing both ways had significant value to me: it educated me, and gave me a more intuitive sense of the two schemes, in the real context of real code. And it provided an opportunity to consider the characteristics of the two scheme in context, leading me to see that the pluggable Strategy offers more flexibility than the Mixin, and that the additional cost is really pretty small.

We will have some duplication, at least for a while. I suspect that what we “really” want is a single Content class with multiple constructors where we configure just the one we want. And that suspicion is new to me today, since earlier on I was concerned about construction and now I’m seeing it as a possible advantage.

We’ll see, but if I had to bet right now, I’d bet that we wind up with a single Content class, with just the right amount of flexibility via pluggable Strategy objects. My guess is that we’ll wind up with something nicer than any for of inheritance could have given us, even at this small scale.

Very interesting!