Problems with sprites

Hanski · November 8, 2018, 9:58pm

If you are making particle effects with that function, that could be important , but otherwise, I do not think you can see any difference in Pokitto’s FPS counter in the usual game.

Pharap · November 8, 2018, 10:30pm

Even when drawing in direct mode?

Better for testing, not neccessarily release builds.
It’s like a kind of assert function, and asserts are supposed to be for debug builds.

There could be a separate function for saving to file.
And I’m not sure that’s something the Pokitto library itself should be handling.
Image saving seems like a job for a 3rd party library.

No it isn’t. You’re changing the definition of the value, so all the existing sprites using this scheme will have to be altered anyway.

That would be a lot easier if it were the first value instead of the third.

The difference is that height and width are required to actually draw the frame, but the maximum number of frames is not, it’s only required for error checking.

Exactly, that’s why I’m advocating having a choice.

template<std::size_t size>
void drawBitmapFrameSafe(int x, int y, const char (&bitmap)[size], std::size_t frameIndex)
{
	const std::size_t frameCount = bitmap[0];

	if(frame >= frameCount)
		crash("frame out of bounds");
		
	const std::size_t width = bitmap[1];
	const std::size_t height = bitmap[2];
	const std::size_t frameSize = width * height;
	
	if(((frameIndex + 1) * frameSize) - 1) >= size)
		crash("frame exceeds bitmap size");
	
	drawBitmapFrameUnsafe(x, y, &bitmap[1], frameIndex);
}

template<std::size_t size>
void drawBitmapFrameUnsafe(int x, int y, const char (&bitmap)[size], std::size_t frameIndex)
{
	const std::size_t width = bitmap[0];
	const std::size_t height = bitmap[1];
	const std::size_t frameSize = width * height;
	
	const char * frame = bitmap[2 + (frameSize * frameIndex)];
	// Do drawing
}

void drawBitmapFrameUnsafe(int x, int y, const char bitmap*, std::size_t frameIndex)
{
	const std::size_t width = bitmap[0];
	const std::size_t height = bitmap[1];
	const std::size_t frameSize = width * height;
	
	const char * frame = bitmap[2 + (frameSize * frameIndex)];
	// Do drawing
}

It costs nothing but an extra function call.
Possibly not even that if the compiler inlines it.

Pharap · November 8, 2018, 11:05pm

Okay, easiest way to settle this:
@jonne picks what happens and everybody else accepts the decision.

jonne · November 8, 2018, 11:13pm

My vote is for the method that is directly compatible with the way piskel exports frames

Pharap · November 8, 2018, 11:21pm

#ifndef NEW_PISKEL_H
#define NEW_PISKEL_H

#include <stdint.h>

#define NEW_PISKEL_FRAME_COUNT 1
#define NEW_PISKEL_FRAME_WIDTH 16
#define NEW_PISKEL_FRAME_HEIGHT 16
#define NEW_PISKEL_COLOR_BITS 8

/* 16-bit (565 rgb) palette data for "New Piskel" */
static const uint16_t new_piskel_pal[] = { 
0x0, 0xa9c7, 
};

/* Piskel data for "New Piskel" */
static const uint8_t new_piskel[258] = {
16,16,
//frame 0 
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,
};
#endif

drummyfish · November 9, 2018, 7:14am

But that exporter is also written by “us” :-) I think the Piskel export format should be changed anyway – now it says the number of frames via a define, which will collide with other exported animations. I really strongly believe we should do it the way I expressed, but I think I’ve provided all the arguments now, so the choice is of the maintaner.

Maybe I’d like to also hear @FManga’s preference (but I really don’t wanna be forcing you to read through all that rumbling ).

EDIT:

One more use case I am thinking of: many times you just want to loop the animation without caring about anything else. This is very common. If the animation format contains the number of frames and the behavior is defined to modulo (can be implemented efficiently) the frames (which I think is how it should behave, rather than throwing an error), this is extremely easy to do, simply pass frameCount as the frame to render. There could even be a simple function in the library to do it – like drawLoopingAnimation(x, y, anim, speed = 1). This way you can simply export a new animation from Piskel, plug it in and it just works, without editing any code. Or, if the code is already compiled and the animation is loaded from file, you don’t have to recompile the code. Extremely friendly to beginners – just export, call a function and it loops.

Hanski · November 9, 2018, 7:48am

I am personally not against any solutions we slightly argue here, so we do not need to have consensus in my opinion. There are good arguments, and any suggested implementation would improve the current situation. It is only a matter of taste: what design aspects one values most. Just go ahead @Pharap

FManga · November 9, 2018, 12:12pm

I skimmed the thread so apologies if what I say doesn’t make sense.

In general, I prefer the lib to contain the simplest (easiest to read and/or smallest compiled) possible implementation for a given feature. Different games will have different priorities and it’s important that they don’t pay for features they don’t use. To do this, concerns should be separated: the function that blits a bitmap should not worry about animation.

The function that handles animation, if implemented with templates as per @Pharap’s last suggestion, does not need a frameCount for error checking since it has size… but preserving backwards-compatibility means we shouldn’t use bitmap[0] for anything. Which is a pitty, but necessary unless we’re up to the task of adjusting piskel and each bitmap of every game out there. Maybe we can re-purpose it down the road, but right now everybody’s hands are too full for that. I’d suggest renaming drawBitmapFrameSafe to simply drawBitmap, since the safe option should be the implied default and using unsafe should be a conscious optimization (in a similar vein to drawPixel and drawPixelRaw). The frameIndex could have a default value of 0 and dropping “Frame” from the name would result in no surprises.

For animation we should consider an entirely different format, when we’re ready for all the work that entails. Specifically: each frame should have its own width/height, offsetX/Y and duration. Instead of an array of frames, the ideal would be a spritesheet packed with maxrects. Since sprites have a lot of transparency around them, we can make blitting faster if each row has two bytes indicating the amount of transparency it can skip in the beginning and end. At this point, we’d be using Animation::update as Display::drawBitmap is obviously the wrong function. That would be used for static images and tiles only.

The current drawBitmap and drawBitmapData functions sorely need to be broken up into several smaller blitting functions (blit2bpp, blit4bpp, etc) to remove code duplication. Preferably, with the same signature:
blitXbpp( bitmapData, srcx, srcy, srcw, srch, srcStride, destx, desty );
Once that’s done, it would be easy and painless to write assembly-optimized versions for each. This way we’d have more readable, easier to maintain, faster, backwards-compatible and smaller code.

Hanski · November 9, 2018, 1:26pm

Looks good to me. That would be a quite flexible animation system. For a beginner, s/he can just use a series of bitmaps to make a very simple animation.

An excellent optimization idea!

Just have to make sure that the blit functions will be inlined with “-O3” compiler option.

HomineLudens · November 9, 2018, 4:38pm

I really need that! Is anybody going to implement this soon?

Pharap · November 10, 2018, 6:42am

Which is the width and height only version in this case?

I very much agree with this. Not paying for what you don’t use is part of the C++ philosophy.

If you don’t use something, it gets optimised out. (That goes for variables, functions, arguments, classes etc.)

Because this part makes it redundant?

if(((frameIndex + 1) * frameSize) - 1) >= size)
	crash("frame exceeds bitmap size");

(I hope I got that logic right, it looks a bit ugly.)

If so that’s a very good point, I hadn’t considered that at the time.

What happens at the moment is:

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap) expects the format with width and height
void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frame) expects width, height and framew, but it’s unsalvagably broken.

So really the first overload is the only one we should care about backwards compatibility for,
the second overload (as far as I can tell) has never worked and needs replacing.

In which case bitmap[0] should be width, bitmap[1] should be height and the rest is just frame data.
Effectively, it’s a flat array.

I can live with that, it makes sense.

If we did that, we’d have to get rid of void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap) and just have void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frameIndex = 0),
otherwise the compiler wouldn’t know which to call.

And if we did that then to maintain backwards compatibility as well, we’d have no choice but to use the width and height only versions.

I agree with this. Though I also think maybe it would be better if that were an extension library.

This is what I did in my Arduboy2 port (which I swear I’ll get back to at some point).

What would stride be in this case?

Though I think it might make sense to have a version that doesn’t have srcx and srcy,
because cutting those out will probably reduce size and slightly increase speed.
(Following the same “don’t pay for what you don’t use” logic.)

(And also I’d rather sourceX, destinationX et cetera, because I think clarity is more important than short names, but I suspect I’m alone there.)

FManga · November 10, 2018, 8:39am

Ah, I wasn’t clear on the two versions. Is the width and height only version what piskel exports?

I’m not 100% sure about the logic (- 1?), but yes.

Having a single implementation instead of duplicated code that’s half-broken sounds good.

Definitely. Display/Core have too much stuff already.

stride is how many pixels you need to skip to go from one row to the next. In other words, the full bitmap’s width. sourceWidth and sourceHeight do not represent the width and height of the bitmap when part of it is being clipped off screen.
sourceX and sourceY are also used when there is clipping, so I don’t think it would be good to cut those out (you are very likely to use it and the cost is minimal so might as well pay for it).

(src/dest are common enough abbreviations that I don’t think they’d detract clarity from the point I’m trying to get across, but for the actual code I’d just go with whatever style the existing functions use.)

Pharap · November 10, 2018, 10:15am

At the moment, yes.

It could be changed, but that would mean rewriting it and either breaking backwards compatibility or having separate export modes depending on which overload of drawBitmap you were intending to use.

((frameIndex + 1) * frameSize) is the index of the first byte of the next frame,
so logically (((frameIndex + 1) * frameSize) - 1) is the last byte of the current frame.

(It looks like I may have missed an open bracket in the original.)

So just:

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frameIndex = 0)

instead of:

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap)
void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frameIndex)

?

Agreed.

Perhaps in the future someone will be able to do for Gamebuino what I’m doing for Arduboy,
so we could deprecate all the Gamebuino compatibility functions.

Then isn’t sourceWidth redundant?
And wouldn’t you also need a vertical height for the sake of vertical clipping?

This seems like one of those cases where an outer function could do the clipping and the inner function could do the actual operation.

template<std::size_t size>
void bitBlit(const uint8_t (&bitmap)[size], std::size_t sourceX, std::size_t sourceY, std::size_t width, std::size_t height, int x, int y)
{
	// Clip x
	// Clip y
	// Clip width
	// Clip height
	
	bitBlitUnsafe(bitmap, sourceX, sourceY, clippedWidth, clippedHeight, clippedX, clippedY);
}

void bitBlit(const uint8_t * bitmap, std::size_t sourceX, std::size_t sourceY, std::size_t width, std::size_t height, std::size_t x, std::size_t y)
{
	// Actual drawing
}

FManga · November 10, 2018, 12:44pm

Pharap:

So just:

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frameIndex = 0)

instead of:

void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap)
void Display::drawBitmap(int16_t x, int16_t y, const uint8_t * bitmap, uint8_t frameIndex)

?

Would this work?

template<std::size_t size>
void drawBitmap(int x, int y, const char (&bitmap)[size], std::size_t frameIndex = 0){
  // check if frameIndex in range
  drawBitmap(x, y, bitmap, frameIndex);
}
void drawBitmap(int x, int y, const char *bitmap, std::size_t frameIndex ){
 // to-do: do screen bounds checks

 int sourceX = 0, sourceY = 0, 
     sourceWidth = bitmap[0], sourceHeight = bitmap[1],
     sourceStride = bitmap[0];

 const char *frame = bitmap + 2 + sourceWidth*sourceHeight*frameIndex;
 if( x < 0 ){
   sourceX = -x;
   sourceWidth += x;
   x = 0;
 }
 if( x + sourceWidth > screenWidth ){
   sourceWidth = screenWidth - x;
 }

 if( y < 0 ){
   sourceY = -y;
   sourceHeight += y;
   y = 0;
 }
 if( y + sourceHeight > screenHeight ){
   sourceHeight = screenHeight - y;
 }

 if( m_colordepth == 1 ) 
    blit1bpp( frame, sourceX, sourceY, sourceWidth, sourceHeight, sourceStride, x, y );
 // to-do other blit calls
}

Does this also answer the second question?

Pharap · November 10, 2018, 1:40pm

There might be some typing issues regarding int vs std::size_t. This might be slightly better:

template<std::size_t size>
void drawBitmap(int x, int y, const char (&bitmap)[size], std::size_t frameIndex = 0){
  // check if frameIndex in range
  drawBitmap(x, y, bitmap, frameIndex);
}

void drawBitmap(int x, int y, const char *bitmap, std::size_t frameIndex ){
 // to-do: do screen bounds checks

 const std::size_t sourceWidth = bitmap[0];
 
 std::size_t sourceX = 0;
 std::size_t drawWidth = sourceWidth;
 std::size_t destinationX = static_cast<std::size_t>(x);

 if( x < 0 ){
   sourceX = static_cast<std::size_t>(-x);
   drawWidth -= static_cast<std::size_t>(sourceX);
   destinationX = 0;
 }

 if( (destinationX + sourceWidth) > screenWidth ){
   drawWidth = static_cast<std::size_t>(screenWidth - destinationX);
 }
 
 const std::size_t sourceHeight = bitmap[1];

 std::size_t sourceY = 0;
 std::size_t drawHeight = sourceHeight;
 std::size_t destinationY = static_cast<std::size_t>(y);

 if( y < 0 ){
   sourceY = static_cast<std::size_t>(-y);
   drawHeight -= static_cast<std::size_t>(sourceY);
   destinationY = 0;
 }

 if( (destinationY + sourceHeight) > screenHeight ){
   drawHeight = static_cast<std::size_t>(screenHeight - destinationY);
 }

 const std::size_t frameSize = (sourceWidth * sourceHeight);
 const char *frame = &bitmap[2 + (frameSize * frameIndex)];

 const std::size_t sourceStride = sourceWidth;
 if( m_colordepth == 1 ) 
    blit1bpp( frame, sourceX, sourceY, drawWidth, drawHeight, sourceStride, destinationX, destinationY );
 // to-do other blit calls
}

I admit, we won’t be having to worry about int being capable of holding the ranges that are needed,
but it probably won’t hurt to be cautious about typing.
(I got bitten pretty badly when porting Arduboy2 because someone was relying on sizeof(int) == 2.)

The size and performance should be roughly the same (maybe slightly better, maybe slightly worse).

Hopefully this version is slightly clearer and possibly has better locality.

Yes. The width is clipped, the ‘stride’ is not.

I probably would have called it something like sourceRowWidth because that’s more descriptive of what it actually is.
Or renamed sourceWidth to width and sourceStride to sourceWidth or dataWidth.

‘Stride’ only makes sense by analogy to ‘walking’ over the data.
(And it conjures images of the ministry of silly walks. :P)

FManga · November 10, 2018, 2:04pm

I use stride because, as a data structure term, it represents how much you have to increment to move from one element (in this case, a row) to the next. I’ve seen this use in libraries like OpenGL. I find it’s clearer because it dispels concerns the reader may have with a term like “width”, such as whether it should be clipped or not.

And any code that conjures images of monty python is good code.

Pharap · November 10, 2018, 2:21pm

It’s used sometimes, but it’s not quite ubiquitous.
I find ‘step’ and ‘increment’ are more common.

At the very least I think it should be rowStride or maybe rowIncrement to emphasise that it’s related to the rows in the image.

Either way I think the blit function is going to need to be well commented.

So you’re saying we should rewrite everything in python? :P

FManga · November 10, 2018, 2:31pm

I’m alright with any of the above. sourceRowWidth is way too wordy.

Unfortunately, Python only conjures images of legless reptiles for me.
If it’d been named “Monty”, on the other hand…