Proposal: Arduboy2Base::drawBitmapFrame

[Pharap]

(Note: I'm proposing this separately because although it's effectively working towards a similar goal to #68, the implementation is potentially more demanding and requires a bit more commitment, whereas #68 is much more straight forward.)

Following on from the suggestion to allow drawBitmap to handle Sprites-format images (see #68), in addition to allowing drawBitmap to handle the width and height elements, it would potentially be useful to allow Arduboy2Base to handle multiple frames, thus implementing full Sprites-format support.

Unfortunately this can't be done with a drawBitmap overload, because introducing the frame parameter would conflict with the default argument, hence a new function name would be needed, and I am proposing drawBitmapFrame as it seems the most logical answer - it draws a frame of a bitmap.

Key thoughts:

• Allows the Sprites-format images with frames to be used with drawBitmap
  • This might make drawBitmap a more viable alternative to Sprites in some situations
• Much like with Sprites::getWidth and Sprited::getHeight, this hides pgm_read_byte from inexperienced users.
• Backwards compatible - the new function will have a new name, so it won't conflict with existing code

Proposed implementation:

void Arduboy2Base::drawBitmapFrame(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t width, uint8_t height, uint8_t frame, uint8_t colour = WHITE)
{
	uint8_t roundedHeight = ((height & 0x7) != 0) ? ((height & ~0x7) + 8) : height;
	size_t frameSize = ((width * roundedHeight) / 8)
	size_t frameIndex = (frameSize * frame);

	drawBitmap(x, y, &bitmap[2 + frameIndex], width, height, colour);
}

void Arduboy2Base::drawBitmapFrame(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frame, uint8_t colour = WHITE)
{
	uint8_t width = pgm_read_byte(&bitmap[0]);
	uint8_t height = pgm_read_byte(&bitmap[1]);

	uint8_t roundedHeight = ((height & 0x7) != 0) ? ((height & ~0x7) + 8) : height;
	size_t frameSize = ((width * roundedHeight) / 8)
	size_t frameIndex = (frameSize * frame);

	drawBitmap(x, y, &bitmap[2 + frameIndex], width, height, colour);
}

Note that the height is explicitly rounded to the next nearest multiple of eight to account for the possibility that the user has provided a height that is a multiple of eight, which would upset the frame calculations. This rounding uses addition, masking and a single branch, which should be relatively cheap overall.

The alternative would be to require the height to be a multiple of eight and allow the function to simply fall apart if this were not the case, but I'm doubtful that the amount of speed and memory saved by avoiding the rounding calculation would be worth the inconvenience to the user.

---

In hindsight, it's a shame that the height used in the Sprites format is the actual height rather than the number of columns. Multiplying by eight would have been cheaper than trying to account for a height that is not a multiple of eight.

[MLXXXp]

This rounding uses addition, masking and a single branch, which should be relatively cheap overall.

One goal of the Arduboy2 library has been to optimize for size and/or speed (no matter how small the savings), sometimes at the expense of things not working if the developer doesn't follow the documentation. Unless there would be a common use case for providing a sprite that didn't have a height that's a multiple of eight, I'd be in favour of leaving out the rounding. I think "inconveniences" that may result with vs without the rounding step could be equally as bad.

For the likely rare case that a developer actually wanted to use a non-modulo 8 height sprite, they could provide their own function (standalone or inherit & overload) that did the rounding before calling drawBitmapFrame().

---

Otherwise, I don't see a problem with adding these functions to the next release, so will do.

[Pharap]

Unless there would be a common use case for providing a sprite that didn't have a height that's a multiple of eight

I can see several arguments in favour of accepting heights that are not a multiple of eight...

Firstly, Sprites actually does account for non-multiples of 8 (and has done so for the last 6 years, since the time in which Sprites.cpp was first added), so for many it will already be the expected behaviour because they will be used to Sprites doing the same.

This means that there will be plenty of images already in use that don't have multiple-of-eight heights, and it's likely that image converters don't bother to check either.

likely rare case

You might be surprised to learn that it's not actually that rare.
I can find plenty of examples of non-multiple-of-eight images in active use in games.

For example:

• MiniRogue's dice and wands
• FirePanic's angels and ground
• 1948's explosions
• Minesweeper's icons
• SpaceCab's landing gear
• Cistercia's numeral components
• RogueBoy's main logo, healthbar, and slots

Even TeamARG managed to break the rule at least once, and they appear to be the ones who allowed non-multiples-of-eight in the first place.

---

One other point, in case a 'rule of majority' argument isn't enough:

With the addition of Sprites::getHeight and Sprites::getWidth, I think it's also likely that people will be wanting to use those retrieved values in calculations, so if the height is required to be a multiple of eight, that could cause complications for some uses of Sprites::getHeight, forcing the user to store the actual height separately from the one stored in the sprite data.

This would be a problem for the work-in-progress game that lead me to suggest Sprites::getHeight and Sprites::getWidth in the first place.

---

Proposed compromise

Hence, I think the best option overall would be to make the multiple-of-eight-correcting version the main version and provide an alternative 'unsafe' function for the minority of users who wanted to opt-out of the multiple-of-eight-correction because they can guarantee that their images are multiples of eight and thus are safe to skip the extra step.

[ace-dent]

My humble suggestion: don't do this!
You are duplicating existing functionality of Sprites::drawOverwrite and I cannot see an advantage. (??)
At the stage the user is advancing in what they wish to render, they will probably 'graduate' to Sprites functions anyway.

Duplicate code adds to maintenance and also user confusion.
How would you communicate when the new user should choose drawBitmapFrame vs drawOverwrite?

Sorry if I've missed something, but genuinely want to know if this has any real world utility?

[Pharap]

You are duplicating existing functionality of Sprites::drawOverwrite
A) Arduboy2::drawBitmap does not behave the same as Sprites::drawOverwrite, it behaves like Sprites::drawSelfMasked.
B) Arduboy2::drawBitmap can produce smaller (but slower) code than Sprite::drawSelfMasked.

At the stage the user is advancing in what they wish to render, they will probably 'graduate' to Sprites functions anyway.
You seem to be assuming that Arduboy2::drawBitmap is somehow a stepping stone towards using Sprites as opposed to a separate and equally acceptable render function with its own benefits and drawbacks.

People who use Sprites usually do so because they are either using one of the other sprite rendering functions (e.g. Sprites::drawOverwrite, Sprites::drawSelfMasked), but for those who want Arduboy2::drawBitmap behaviour, Arduboy2::drawBitmapFrame may be a better option than Sprites::drawSelfMasked if size is more important than speed.

For those who are considering using one or the other, the ability to change the function without having to change the sprite data would be very convenient.

How would you communicate when the new user should choose drawBitmapFrame vs drawOverwrite?
With the documentation, the same way the behaviours of all functions are communicated.

Arduboy2::drawBitmapFrame would be used when it can be shown to produce smaller code and the associated slowdown is acceptable, exactly the same as when Arduboy2::drawBitmap would be used.

---

Thinking about it, existing code that uses Arduboy2::drawBitmap might even benefit from using Arduboy2::drawBitmapFrame instead, since it might lead to smaller images, less complex image manipulation logic, or a reduction in size as a result of having fewer pointer-filled arrays. (I.e. many of the benefits that Sprites::drawSelfMasked currently has over Arduboy2::drawBitmap.)