mainly talking about the data formatting of framed bitmap since its different from the other bitmap format
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
Not using, no objection.
1 Like
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. 
While youāre at it, might as well move the define out so that CONSTEXPR can be used in other places.
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?
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)]);
}
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.
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
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.
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
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ā¦)
1 Like