'drawBitmap' Frames


#21

mainly talking about the data formatting of framed bitmap since its different from the other bitmap format


#22

Would anybody object if I were to replace the library’s Display::drawBitmap(x, y, bitmap, frame) implementation with my own?

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frame)
{
	using std::size_t;
	
	#if defined(__cpp_constexpr)
	#define CONSTEXPR constexpr
	#else
	#define CONSTEXPR const
	#endif
	
	CONSTEXPR size_t widthIndex = 0;
	CONSTEXPR size_t heightIndex = 1;
	CONSTEXPR size_t dataIndex = 2;
	CONSTEXPR size_t bitsPerByte = 8;
	
	#undef CONSTEXPR

	const size_t width = bitmap[widthIndex];
	const size_t height = bitmap[heightIndex];
	const size_t frameSize = ((width * height) / (bitsPerByte / Display::m_colordepth));

	Display::drawBitmapData(x, y, width, height, &bitmap[dataIndex + (frameIndex * frameSize)]);
}

As far as I can tell the existing version is broken anyway,
so I doubt many games are using it.

I think offloading the work to drawBitmapData is likely to be easier to maintain anyway,
and it would be consistent with what drawBitmap(x, y, bitmap) is doing.


Edit

To make this easier for people, here’s a poll:

  • No, no problem
  • I have no opinion either way
  • Yes, I have an objection!

0 voters


#23

Not using, no objection.


#24

I don’t think I’ve ever used that, so no objection to changing it.
The effort you went through to use constexpr, if available, is totally overkill, though. :stuck_out_tongue:
While you’re at it, might as well move the define out so that CONSTEXPR can be used in other places.


#25

If it were up to me I’d have just added constexpr, broken mbed-online and begun the C++11 revolution. :P

I could, but that would mean another thing on the list to convince people to accept.
I’m always reluctant to:

  • Make more than the necessary number of changes in a single PR
  • Make changes that other people might object to

Perhaps we ought to form a summit of developers and have a discussion about the library and some changes that could be made to it?


#26

I understand the concern, but, as it is, that way of coding doesn’t have any advantage over keeping it simple:

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frame)
{
	using std::size_t;
	
	const size_t widthIndex = 0;
	const size_t heightIndex = 1;
	const size_t dataIndex = 2;
	const size_t bitsPerByte = 8;

	const size_t width = bitmap[widthIndex];
	const size_t height = bitmap[heightIndex];
	const size_t frameSize = ((width * height) / (bitsPerByte / Display::m_colordepth));

	Display::drawBitmapData(x, y, width, height, &bitmap[dataIndex + (frameIndex * frameSize)]);
}

#27

The advantage is the semantic implication that they are intended to be compile time constants rather than simply read-only at runtime.

The final result will (probably) be the same in this case,
but only constexpr guarantees compile time initialisation,
which can make a difference in some circumstances.

I could also technically leave out all the consts (apart from the one on bitmap),
and the compiled code would (theoretically) be the same, but the semantics would be different.


#28

While constexpr has that semantic implication, I question the intent in the first place. The effort you go through to express that intent also expresses a need: the hint that something maybe should be calculated compile-time is more important than legibility and maintainability.

When someone else comes across the code in the future, either to make changes or to learn how it works, the simple version better expresses what the function is supposed to do than the original. Relevant comic.

If we’re going to kludge support for features the compiler doesn’t have, might as well do this:

#if !defined(__cpp_constexpr)
#define constexpr const
#endif

#29

There’s two problems with doing #define constexpr const:

  • It violates the principle of “macros should be in macro case
  • It violates the principle of least surprise by giving a keyword a new meaning
    • (Even though said keyword wouldn’t be a keyword at that point)

And it would only be two lines shorter anyway.


The other alternative is:

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frame)
{
	using std::size_t;
	
	#if defined(__cpp_constexpr)
	constexpr size_t widthIndex = 0;
	constexpr size_t heightIndex = 1;
	constexpr size_t dataIndex = 2;
	constexpr size_t bitsPerByte = 8;
	#else
	const size_t widthIndex = 0;
	const size_t heightIndex = 1;
	const size_t dataIndex = 2;
	const size_t bitsPerByte = 8;
	#endif

	const size_t width = bitmap[widthIndex];
	const size_t height = bitmap[heightIndex];
	const size_t frameSize = ((width * height) / (bitsPerByte / Display::m_colordepth));

	Display::drawBitmapData(x, y, width, height, &bitmap[dataIndex + (frameIndex * frameSize)]);
}

Which is longer and opens up more room for error because it constains duplication,
but it might be marginally more comprehensible.


Really though I don’t think the CONSTEXPR code that big a deal, it’s pretty basic macro usage.
There’s plenty of more cryptic stuff already in the library.

With any luck eventually the C++98 dependence will be scrapped and then this can be changed to constexpr permanently.


#30

Before I go uploading the code, since this was a point of contention…

  • Use #define CONSTEXPR
  • Use #if defined(__cpp_constexpr) with multiple variable declarations
  • Use const (and leave a comment to remind maintainers to change to constexpr when C++11 finally arrives)

0 voters


#31

4 votes is enough for me.

(Though I’d love to know who had ‘no opinion either way’ for the earlier poll. :P)

As an added bonus this PR comes with free function documentation so I never have to complain about not knowing how this function works again.

The new format should be familiar to Arduboy users since it’s essentially the same as the format Sprites uses, but with the frame data matching the format expected by the current screen mode.


Edit:

Just in case… @jonne, add this to your todo list please.


(Maybe I’ll fix the rectangle bug next…)