-
Notifications
You must be signed in to change notification settings - Fork 301
Update ex_palette to build with emscripten #1318
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
Conversation
ex_prim_wrap_pixel.glsl | ||
ex_prim_wrap_pixel.hlsl | ||
ex_shader_multitex_pixel.glsl | ||
ex_shader_multitex_pixel.hlsl | ||
ex_shader_palette_pixel.glsl | ||
ex_shader_pixel.glsl |
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 had a really old branch, so when I rebased I had a conflict here (it was simple). Anyway, before the rebase I saw these files were alphabetical (in my older checkout), and so after the rebase I again sorted alphabetically in my IDE. Looking at the diff now, obv it wasn't alpha sorted in master... anyway, I can revert the order changes if requested.
@@ -115,45 +94,30 @@ int main(int argc, char **argv) | |||
} | |||
} | |||
|
|||
al_set_new_bitmap_format(ALLEGRO_PIXEL_FORMAT_ANY); | |||
pal_bitmap = al_create_bitmap(255, 7); |
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.
OK, so I came across some very unexpected behavior here. Perhaps it is a bug? I'll explain.
I knew that allegro imposes a minimum texture size (16 for opengl. or maybe it is for any gfx backend?). So, originally I just had 16
here for the bitmap height, knowing I'd only use 7 rows and would need to divide by 15 in the fragment shader for the indexing (instead of 6, if the texture really were 7 pixels tall). All good, conceptually. But in practice, setting this value to 16
resulted in the entire screen being black.
I debugged via Spector.js and saw the texture in memory was padded in an unexpected way.
Here it is when you create the bitmap with height 7
:
And this is height 16
:
The bug is that I expected the additional padding that allegro would provide to go on the bottom and the right of the texture, leaving my "intended" texture in the top-left corner of the actual texture. The reality is this is reversed, resulting in the shader only ever getting black pixels (as the rest of the bitmap I'm not using it simply black).
I'm not sure if this misconception comes from my lack of familiarity with graphics pipelines (does padding in this manner make sense if you know more about how the system works?). And I'm sure changing this behavior is not a reasonable request. At least, it'd be nice if this behavior was documented here
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 think what you're seeing here is that the texture is stored upside down. When the height is 7, it's padded on the bottom. When the height is 16 there's no padding, but the data is placed on the bottom because the texture is upside down.
|
||
al_color_rgb_to_hsl(r, g, b, &h, &s, &l); | ||
if (j == 6) { |
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.
Oh, almost forgot about this. If you leave this code in, every odd row in the palette array will be all black! I didn't really dig into why this was happening, I just deleted all the branches here and kept the interesting part, the hue shifting.
Looking at what I assume is the old version ( https://www.youtube.com/watch?v=Shk8GBCXQq4&ab_channel=EliasPschernig ) I do notice some differences (the corner sprites strobe, and there's a moment all the colors get really washed out) but IMO simpler is better here for this demo.
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.
Ah, I hadn't realized the hsl conversion functions expect rgb values to be 0-1, so this makes sense now.
Here it is with the original hsl shifting: https://unique-van.surge.sh/ex_palette.html
al_set_shader_float_vector("pal", 3, | ||
pals[(int)t % 20 > 15 ? 6 : 0], 256); | ||
al_set_shader_float("pal_set_1", (int)t % 20 > 15 ? 6 : 0); | ||
al_set_shader_float("pal_interp", 0); |
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'd expect the corner sprites to swap palettes over time, but they remain the same colors... I'll have to look into this more tomorrow.
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.
It's because the 0th and. the 6th palettes were the same colors, both being hue shifted at equivalent values (0 and 360). I change the hue shifter to be not a factor of the period, and now it alters between two colors as expected.
I was able compile allegro on my mac, and verified this works on OSX too. |
This is great, thanks! |
Fixes #1315
Demo: https://tedious-porter.surge.sh/ex_palette.html
ex_palette
not portable via emscripten. Instead of using a uniform to store the palette, and updating/interpolating it on the CPU every cycle, I changed the program to instead pre-generate a texture with all the colors and added uniforms to do the interpolation on the GPU.Please note I cannot build natively on my Mac (Unrelated to these changes, I can't get the build to work, first error being:
error: use of undeclared identifier 'ALLEGRO_TIMEOUT_SDL'
). I can attempt to build it on my Windows machine tomorrow (unless a reviewer shares that it worked for them).