Skip to content

Scripting: Commands to read / write palettes#3692

Draft
Jengolus wants to merge 5 commits intomgba-emu:masterfrom
Jengolus:master
Draft

Scripting: Commands to read / write palettes#3692
Jengolus wants to merge 5 commits intomgba-emu:masterfrom
Jengolus:master

Conversation

@Jengolus
Copy link

This PR adds scripting commands to manipulate color palettes and corresponding tests. They can be used as follows:

local palette = emu:readPalette(0)
print("Colors of palette 0:")
print("Index", "Red", "Green", "Blue")
for idx, color in ipairs(palette) do
    local values = util.unpackColor(color)
    print(idx, values.red, values.green, values.blue)
    -- Or equivalently:
    --print(idx, values[1], values[2], values[3])
end

local red = util.packColor(255, 0, 0)
-- Does nothing on GBA
emu:writePalette(0, palette[1], red, palette[3], palette[4])

Motivation: There isn't currently any way to directly modify palettes, especially for SGB games. In particular, I am working on a socket interface between the pixel art software Aseprite and mGBA to preview images directly in the games they should appear in, but I cannot change the colors on SGB.

Design choices:

  • packColor takes red, green, and blue from 0 to 255, not 0 to 31
  • unpackColor converts the channels as 255 * channel // 31, not 8 * channel
  • writePalette accepts the colors as four integers and thus doesn't work for GBA. I tried to make it work using a table, but couldn't access the table's values from C. I can change that if given instructions.

@endrift
Copy link
Member

endrift commented Jan 30, 2026

First and foremost: There are several peculiar consistency issues with your code that imply you didn't review it yourself before submitting it. Is this code AI generated? Please review your own code for these issues before asking us to review it. Please also ensure you adhere to the existing coding and style guides used elsewhere in the code.

Second, when it comes to opinionated matters:

  1. Do not scale the colors when reading/writing. Everyone has different opinions for how this should be handled, and it's a lossy operation, so we should not enforce it in the scripting API. The script itself can do whatever scaling it wants.
  2. Writing palettes should only write a single entry in the palette, not the entire palette at once. It would solve your table issue (which itself is strange considering you construct a table in C already).

Please bring this PR to non-draft quality before asking for further review.

@endrift endrift marked this pull request as draft January 30, 2026 00:16

if (palette == 0) {
return &mScriptValueNull;
return 0x8000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the logic behind returning 0x8000 here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my knowledge and testing, the highest bit is ignored, so I figured it would make a good error marker since readPalette cannot return a nil anymore.

}

return tbl;
return palette[index];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a LOAD_16.

palette[4*index + i] = values[i];
video->renderer->writePalette(video->renderer, 4*index + i, values[i]);
}
palette[index] = color;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a STORE_16, but it also needs to be before the ->writePalette calls.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for both readPalette and writePalette.

Comment on lines 694 to 696
mSCRIPT_DEFINE_DOCSTRING("Read a 16-bit value from the given palette index (0-indexed)")
mSCRIPT_DEFINE_STRUCT_METHOD(mCore, readPalette)
mSCRIPT_DEFINE_DOCSTRING("Set the colors of the palette of the given index (0-indexed). Takes four integers encoding the colors in 5 bits each. See util.packColor. Does nothing on GBA.")
mSCRIPT_DEFINE_DOCSTRING("Write a 16-bit value to the given palette index (0-indexed)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove mention of format and util.packColor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt that mimicked the other docstrings more closely; added it back in now.

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

Successfully merging this pull request may close these issues.

2 participants