Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove Bitmap class #70

Open
pphaneuf opened this issue Aug 22, 2014 · 4 comments
Open

remove Bitmap class #70

pphaneuf opened this issue Aug 22, 2014 · 4 comments

Comments

@pphaneuf
Copy link
Member

No description provided.

@pphaneuf
Copy link
Member Author

There seems to be two uses for the Bitmap class. One of them is to store
images that are loaded, and SDL_Surface works just fine for that (I added a
Image::new_surface() method that create an SDL_Surface from the Image).

The other thing is to do source clipping on other Bitmap (I think, can't
be sure yet, since it actually takes a void pointer to raw pixel data).
This is rather iffy, because it makes it pretty hard to understand what is
actually happening, and how a Bitmap can suddenly be not good anymore (for
example, there's Bitmap::reload() that I wanted to remove, since I thought
it was simpler just to delete the old one and put the new one in place, but
that invalidated a whole bunch of sub-Bitmaps and made the game crash).

We should convert all of this to be just SDL_Surface, and use the source
rectangle of SDL_BlitSurface to do the work (made available by the new
version of Video_bitmap::put_surface I added recently). For blitting to a
Bitmap, we can use SDL_SetClipRect() to restrain the blit.

We should add a function to calculate the intersection of two SDL_Rect,
this would allow us to have an SDL_Rect where we had those nasty Bitmap,
and quickly calculate the SDL_Rect to use for Video_bitmap::put_surface.

@pphaneuf
Copy link
Member Author

Stéphane expressed his disagreement with this in person, yesterday. I'm not 100% sure
if I understand correctly, but his position seemed to be that it was good to have our
own abstractions.

I clarified my own feelings, which is that maybe it'd be okay to have a wrapper class
around SDL_Surface, yes (although I'm usually finding it okay to deal with directly,
maybe use a smart pointer to do SDL_FreeSurface automatically), but Bitmap is not a
wrapper, it's its own brand of evil, using way too much of the "create from existing
pixels", making the ownership and lifetime of the whole thing almost impossible to
understand.

Since it looks like most place that has a Bitmap to do this also has a pointer to
the thing that has the real bitmap, I think that if they had an SDL_Rect to pass to
Video_bitmap::put_surface would do just fine.

When I mentioned the refcounting that SDL_Surface could do, Rémi pointed out that
maybe we could use that, but I still think that's iffy. Shared ownership isn't really
what we want here (which is the #1 reason I didn't introduce shared_ptr, actually, I
think it's more dangerous than anything if you don't understand what you're doing,
and I sure don't!), when we get rid of the background image (which is the usual thing
we have in a Bitmap), what the others have is really a weak reference to that one, it
should go away now, and they should start doing whatever they were doing with the new
one (AKA, what they want is access to the current background surface, not a copy
of, or a reference to whatever happens to be the background surface when they were
created).

@pphaneuf
Copy link
Member Author

Update on this: SDL2 works best if you can put your pixel data in "textures", that you pretty much can't really access directly (you can lock them, but if you do, that overwrites the entire texture, as it does not download the existing texture from video memory, and will do a full upload afterward).

So the whole business of having pointers inside video memory and such, that's just not really the best (whether we wrap SDL or not), and things like Video_bitmap and Bitmap are all about the pixel access...

At the moment, the SDL2 port (currently at https://github.com/pphaneuf/quadra/tree/sdl2 but watch out, this is unstable, I rebase it often!) uses a single texture for the whole screen, and does a full update on every frame (this doesn't horribly suck probably because 640x480 is nothing nowadays), but it'd be neat if we could pre-render all the tetrominos (in each orientation) in a texture, ahead of time, then use the fast texture blitting to put them on screen. Anyway... :-)

@pphaneuf
Copy link
Member Author

Oh, there's a SDL_IntersectRect in SDL2 now, too, BTW.

@pphaneuf pphaneuf removed their assignment Jun 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant