-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
render: Preliminary support for paletted textures #6192
Conversation
I really don't want to get into paletted textures in the 2D API, fwiw; if there isn't a trivial way to upload them to a non-paletted format, we should add that as a generic thing, and let people that need this use a shader directly in the upcoming GPU API. |
Hi @icculus , SDL 1 supports paletted textures and for some projects like fheroes2 it is important to have such ability. Right now we do a hack to convert from 8-bit images to 32-bit images which leads to 20% performance drop. This is a very significant reduction which affects not only the gameplay but eats more resources and battery on portable devices. If you think about an easier way to add support for such feature then please explain it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ccawley2011, I roughly took a look at your changes and put several general comments here. Could you please take a look when you have time?
include/SDL_render.h
Outdated
* \param ncolors the number of entries to modify | ||
* \return 0 on success, or -1 if the texture is not valid. | ||
* | ||
* \since This function is available since SDL 2.0.22. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update this comment due to new releases being online.
/* | ||
case SDL_PIXELFORMAT_INDEX1LSB: | ||
case SDL_PIXELFORMAT_INDEX1MSB: | ||
case SDL_PIXELFORMAT_INDEX4LSB: | ||
case SDL_PIXELFORMAT_INDEX4MSB: | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this commented code to be present? If not then we can do:
if (format != SDL_PIXELFORMAT_INDEX8) {
SDL_SetError("Unsupported palette format");
return NULL;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to support 1-bit and 4-bit formats as well, I'm just not sure how locking should behave in these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this pull request is stuck for multiple months a good approach is to implement only for 1 format and add TODO comments in relevant places for other format. On top of this after this pull request is merged it would be wise to create an issue asking to add these missing formats.
src/render/SDL_palette_sw.c
Outdated
return NULL; | ||
} | ||
|
||
swdata = (SDL_SW_PaletteTexture *) SDL_calloc(1, sizeof(*swdata)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this project's way of coding but a good practice is to use variable declaration with its initialization:
SDL_SW_PaletteTexture *swdata = (SDL_SW_PaletteTexture *) SDL_calloc(1, sizeof(*swdata));
This way we reduce the chance of uninitialized pointers being used in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed declarations and code is only possible in C99, while SDL uses C89.
src = pixels; | ||
dst = (Uint8*)swdata->surface->pixels + rect->y * swdata->surface->pitch + rect->x; | ||
length = rect->w; | ||
for (row = 0; row < rect->h; ++row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should initialize row
within the loop as it would be easier to read and at the same time would be the same performance since this variable is still on stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only possible in C99.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Is there a restriction not to use C99?
src/render/SDL_palette_sw.c
Outdated
SDL_SW_LockPaletteTexture(SDL_SW_PaletteTexture * swdata, const SDL_Rect * rect, | ||
void **pixels, int *pitch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This place and other places in the code: please align pointers to have the same code style. For example, here we align them in the middle while in the include/SDL_render.h
we align to left.
|
||
typedef struct SDL_SW_PaletteTexture SDL_SW_PaletteTexture; | ||
|
||
SDL_SW_PaletteTexture *SDL_SW_CreatePaletteTexture(Uint32 format, int w, int h); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these functions miss Doxygen comments and also please add empty lines between them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doxygen comments don't generally seem to be used in internal headers such as this one.
} | ||
} | ||
|
||
/* vi: set ts=4 sw=4 expandtab: */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite of fact this line is present in other files this is an editor specific thing which shouldn't exist in the generic source code. This is a developer responsibility to adjust editors per their needs.
Also it would be important to know the target version of this change, whether this PR going to be a part of the source code or there are concerns about integrating it. If there are concerns what are they besides the fact of possible complexity of the code? |
4ebb0d3
to
e6b822d
Compare
The main blocker for this PR is the fact that bilinear filtering still doesn't work with backends that implement this using shaders. Once that's done it should be ready, but I'm not sure that I'm capable of doing this myself. If anyone else is interested, then feel free to take over this PR. If this is ever finished, I would quite like to get this into SDL2 as well as SDL3, but I don't know if new API calls are still being accepted there. |
Hi @ccawley2011 , bilinear interpolation cannot be done on indexed images. The only way to do it is to get values in RGB space and then find the nearest value in the palette to the result. If we think that RGB is a 3D space then it's a task of finding the smallest distance between 2 dots: the result value and existing palette value. The complexity of this algorithm is huge and bigger than to O(M*N). It would definitely be very slow. |
e6b822d
to
b46f5ca
Compare
b46f5ca
to
ee242a7
Compare
I've implemented bilinear downscaling for indexed images in the past here. That file has 2 functions, one does 32-bit scaling to arbitrary size, the other one downscales 8-bit indexed images by 50%. Downscaling of 8-bit indexed images is achieved by passing a |
You can handle the filtering in the shader method with some number of weighted lookups on the texture then blend. (I'm not sure this works for scaling down?).
For sdl2 I think this is maybe too much to add since it adds a lot to each rendering backend implementation, new indexed texture formats and specific shaders. Any use of this would likely need some shader customization anyway ontop - which SDL2 is not built for. I would rather see a generic data/byte type of image that can bind a generic/custom/utility shader to , something that could also be useful for tonemapping, luts etc. (thinking sdl3) |
This hasn't been updated in a while and we're cleaning house for SDL 3.0. Please feel free to reopen this if you'd like to clean it up and get it in! |
This extends the limited support for paletted textures in the DirectFB render driver to allow support to be added to other platforms, and to allow the palette to be changed after the texture has been created, which is useful in older games.
This is currently implemented in the Direct3D 9, OpenGL and OpenGL ES 2 drivers using shaders, but these currently don't work correctly when bilinear filtering is enabled. Fixing it would require using texture targets, but I'm not sure on the best way to implement this. A software-based fallback is also available, which doesn't have this problem.
testoverlay2 has been modified to make use of this feature, and another test program has been added to test palette rotation effects that may occur without updating the image data. I've also opened sergiou87/open-supaplex#41 to demonstrate this in a real-world application.