Adds support for per scene and project wide DMG palettes #1601

Q-Bert-Reynolds · 2024-10-03T02:55:35Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

I guess this is more of a feature than a bug fix. Builds had an unexpected behavior, so I changed the editor to leverage that behavior instead of changing the engine to get rid of it.

What is the current behavior? (You can also link to an open issue here)

When you play a build on a DMG, palettes are reset on scene load. You can set BGP, OBP0, and OBP1 via GBVM or engine eject, but your changes are overwritten when you load the next scene. I thought this was an odd behavior, so I went digging and found out that monochrome palette data is written per scene at build time to a fixed value, and those values can't be configured. This is especially problematic for scenes where you want the player to go behind objects, but you also want black to be the color the player is in front of. I could easily eject the engine and prevent the palette loading, but it was painful looking at inverted colors in the editor.

What is the new behavior (if this is a feature change)?

This PR introduces a series of quality of life improvements for the DMG and GBP. It allows you to select project wide or per scene monochrome palettes (ie. BGP, OBP0, and OBP1). Your selected monochrome palettes are reflected in the scene, background, and sprite views when in mono mode or when previewing as mono. To that end, I've also added the option to preview as mono in the background and sprite views.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Nope. It doesn't touch the engine code at all. It only added 3 optional fields to scene save data (no migration needed). Most of the editor code was left untouched. Probably the most radical changes were to ColorizedImage.

Other information:

I've never used TypeScript or React or Electron. I have no idea what I'm doing. I would love to know if there are better ways of going about some of this stuff - particularly with forms and styling.

In the future, I'd like to make the default monochrome colors configurable and add Super Game Boy preview options, but I decided those were beyond the scope of this PR.

Here's some screenshots:

…ot GUI)

…lement and what the fuck is this language

… a component out of this

…e view

…rite palettes in scene view

…ng pal fields to the scene view

…t caused values not to save

…xpectations

Q-Bert-Reynolds · 2024-10-04T22:04:51Z

src/shared/lib/helpers/color.ts

-export const indexSpriteColour = (g: number, objPalette: ObjPalette) => {
-  if (g < 65) {
-    return 3;
-  }
-  if (g < 130) {
-    return objPalette === "OBP1" ? 2 : 3;
-  }
-  if (g < 205) {
-    return objPalette === "OBP1" ? 2 : 1;
-  }
-  return 0;
+//NOTE: this assumes GB Studio's sprite image requirements are met... https://www.gbstudio.dev/docs/assets/sprites#image-requirements
+export const indexSpriteColour = (g: number, objPalette: [number, number, number, number]) => {
+  if (g < 130) return objPalette[3];
+  if (g < 205) return objPalette[2];
+  return objPalette[1];


This also fixes a bug. Because "white" is always used as transparency, only 3 colors can actually be mapped. The previous version of this function only returned 3 different numbers (either 0,1,3 or 0,2,3), but it did so with 4 different inputs. This resulted in editor previews of sprites set to OBP1 that were different than what you would see in builds. Of course, you could only see this preview in the sprite sheet select in the game world view, so I don't suppose most folks would have noticed.

…verywhere

… scene's mono palettes changed

Q-Bert-Reynolds · 2024-10-05T14:27:49Z

src/components/backgrounds/BackgroundPreviewSettings.tsx

+  const value = useAppSelector((state) => {
+    if (state.project.present.settings.previewAsMono) return "0";
+    const sceneId = state.editor.previewAsSceneId;
+    if (sceneId === "") return "1";
+    return sceneId;
+  });


I added a mono mode, similar to the one found on the world page, to the "preview as" drop down. Previously, only one non-scene option was available in this drop down, "Default Colors", which was used when previewAsSceneId was "". To facilitate more options, I'm just using the stringified index of the non-scene values. To ensure a consistent experience when switching between pages, I set the default state to "0" when previewAsMono and "1" when previewAsSceneId is unset.

Q-Bert-Reynolds · 2024-10-05T14:30:31Z

src/components/backgrounds/BackgroundPreviewSettings.tsx

  const onChange = (newValue: string) => {
+    dispatch(
+      settingsActions.editSettings({
+        previewAsMono: (newValue == "0"),
+      })
+    );
    dispatch(editorActions.setPreviewAsSceneId(newValue));
  };


I'm also using the "preview as" drop down selection to drive the previewAsMono editor setting so if you select it here or in the sprites page, the scenes will be in monochrome when you return to the world page.

Q-Bert-Reynolds · 2024-10-05T14:36:09Z

src/components/backgrounds/BackgroundPreviewSettings.tsx

@@ -190,7 +201,8 @@ const BackgroundPreviewSettings = ({
              ? l10n("FIELD_PREVIEW_AS_SCENE", {
                  sceneName: sceneName(scene, sceneIndex),
                })
-              : l10n("FIELD_PREVIEW_AS_DEFAULT")}
+              : ([l10n("FIELD_PREVIEW_AS_MONO"), l10n("FIELD_PREVIEW_AS_DEFAULT")][+value])


As someone who has never used TypeScript before, I found the [+value] syntax a little strange, but it's what StackOverflow told me to do. I'm considering revisiting the weird stringified index hack when I add SGB preview support anyway, but this is a little too hacky for your tastes now, let me know.

Q-Bert-Reynolds · 2024-10-05T14:44:27Z

src/components/editors/SceneEditor.tsx

-            {colorsEnabled && (
              <SidebarColumn>
                <FormRow>
                  <FormField
                    name="playerSpriteSheetId"
                    label={
                      <>
-                        {l10n("FIELD_SCENE_BACKGROUND_PALETTES")}


Now that there's a palette to edit even in monochrome mode, I moved the colorsEnabled check after the label. I had a strong urge to update the indentation everywhere, but I left it alone to avoid a massive unreadable diff. There are a few bits that looked too odd without any indentation changes, but it's probably not a bad idea to fully correct the indentation levels for consistency after the bulk of the code review is done.

Q-Bert-Reynolds · 2024-10-05T14:46:34Z

src/components/editors/SceneEditor.tsx

@@ -816,6 +853,25 @@ export const SceneEditor = ({ id }: SceneEditorProps) => {
                    name="playerSpriteSheetId"
                    label={l10n("FIELD_SCENE_SPRITE_PALETTES")}
                  >
+                  {monoEnabled &&
+                    <PaletteButtons>


Perhaps PaletteButtons was the wrong wrapper node to use here, but I used it as a quick and dirty way of keeping the OBP0 and OBP1 palette pickers on the same row.

Q-Bert-Reynolds · 2024-10-05T14:54:00Z

src/components/forms/DMGPalettePicker.tsx

+
+  const settings = useAppSelector((state) => state.project.present.settings);
+  const dmgColors = [settings.customColorsWhite, settings.customColorsLight, settings.customColorsDark, settings.customColorsBlack];
+  const fields = isSpritePalette ? [1, 2, 3] : [0, 1, 2, 3];


At one point, I was showing all 4 input boxes for sprites. Then, I realized the lowest two bits are never used anyway and it felt silly to show them. I didn't want to change the underlying data structure from [number, number, number, number] to number[] though because dealing with arrays of different sizes complicated the logic further and didn't line up as well with the byte that it represented in a build.

Q-Bert-Reynolds · 2024-10-05T15:18:35Z

src/components/forms/DMGPalettePicker.tsx

+  const width = showName ? `max-width: ${isSpritePalette ? 121 : 245}px;` : "width: 100%";
+  const DMGPalettePickerStyle = styled.div`
+    display: flex;
+    ${width}
+    box-sizing: border-box;
+  `
+
+  const DMGPaletteTextStyle = styled.div`
+    width: 9px;
+    color: grey;
+    font-size: 9px;
+    text-align: center;
+    transform-origin:50% 50%;
+    transform: rotate(90deg) translate(-7px,8px);


There's a lot of hacky stuff going on with the styling:

I wanted the pickers to be full width in the settings screen, but only as wide as the color palette buttons and for OBP0 and OBP1 to be on the same row in the scene editor.

I wanted there to be text labels in the scene editor, but I wanted them to be tiny, greyed out, unobtrusive, and vertical.

I killed off the spacing between numerical inputs so the elements look more like they belong to each other, but it makes the red selection outline look a little funky since it expects a 5px spacing.

In the getRows function below, I explicitly set the background and text colors from the default color palette. Didn't know of a cleaner way of doing that. Also, the text color seems fine, but using an offset index to lookup the palette color may be problematic in the future if the default DMG palette is ever configurable.

I also changed the min size of NumberInput to allow both sprite palettes to fit on a single row and only be as wide as the palette buttons below it. I didn't notice any issues, but if there's a way to override the styling of child elements instead of just changing the min-width globally, I'd rather do that.

Q-Bert-Reynolds · 2024-10-05T15:22:54Z

src/components/forms/ObjPaletteSelect.tsx

+  const settings = useAppSelector((state) => state.project.present.settings);
+  const obp0 = settings.defaultOBP0;
+  const obp1 = settings.defaultOBP1;
+  const dmgPal = [settings.customColorsWhite, settings.customColorsLight, settings.customColorsDark, settings.customColorsBlack];


I used the custom colors from settings instead of DMG_PALETTE.colors because I thought for sure I'd be making those configurable in this PR. I still plan on doing that, but I realized how cumbersome the task was when the various workers complained about trying to access settings off the main thread, so I abandoned ship there.

Q-Bert-Reynolds · 2024-10-05T15:32:20Z

src/components/forms/SceneSelect.tsx

  ...selectProps
 }) => {
  const scenes = useAppSelector((state) => sceneSelectors.selectAll(state));
+  const optionalScenes = (optionalLabels || []).map((optionalLabel, i) => ({ value: i.toString(), label: optionalLabel }) ) as SceneOption[];


When there was only one non-scene option, a SceneOption was created with a value of "". To facilitate more non-scene options, I use their index as the value. It's pretty hacky, but it's simple, easy to work with, and required minimal changes.

Q-Bert-Reynolds · 2024-10-05T15:52:50Z

src/components/ui/form/NumberInput.tsx

-  min-width: 45px;
+  min-width: 10px;


I did this to allow for a smaller DMGPalettePicker, and I couldn't figure out a way to override the style. I don't think it causes problems, but I need to do some more digging/testing.

…to present them the same way as Thumbnail

chrismaltby · 2024-10-10T09:26:25Z

Hi @Q-Bert-Reynolds (great username btw) just getting over a bit of illness so I've been away for a while but looks like you've been pretty busy on this. From a real quick glance through the code seems reasonable, maybe a few things I'd tidy up and appreciate all the comments you've added in the PR will definitely help me reviewing this.

Just in the middle of a quite big change myself (adds a Plugin Manager + a few new plugin types) so I'll want to get that merged before reviewing/merging this but from what I see we shouldn't have any conflicts. When I get to this will let you know if I have any questions. Thanks.

Q-Bert-Reynolds added 16 commits October 1, 2024 18:38

dmg palettes (BGP, OBP0, OBP1) assignable in project settings file (n…

0c5dc0b

…ot GUI)

stubbs out settings page for DMG palettes

27c9f71

functional but ugly... how the fuck do you change the color of a UI e…

a840e83

…lement and what the fuck is this language

ok, that looks decent... now for scenes... and I should probably make…

feb87db

… a component out of this

obj pal select shows configured palette

5c1579b

can now see custom BGP in scene view

8af70eb

OBP palette selection shows on sprites... something seems off though

b979450

fixes issue with incorrect sprite palletization

e65305e

fixes issue where preview as mono showed wrong sprite pallete in scen…

491cc10

…e view

fixes issue where switching between mono and color wouldn't update sp…

6f01ceb

…rite palettes in scene view

adds preview as monochrome option to sprite and image pages

0e8ac08

adds dmg palette picker to tidy up settings page and prepare for addi…

ebbad21

…ng pal fields to the scene view

adds dmg palette pickers to scene editor, fixes issue with picker tha…

adfdd17

…t caused values not to save

better looking... maybe

a5b470a

uglier code, prettier editor

8cd7007

updates sprite colorization to be more consistent with current user e…

dbb4514

…xpectations

Q-Bert-Reynolds commented Oct 4, 2024

View reviewed changes

Q-Bert-Reynolds added 3 commits October 4, 2024 22:23

add MonoPalette type instead of using [number,number,number,number] e…

ffcbf8d

…verywhere

fixes issue where scene actors weren't updating their colors when the…

7a59195

… scene's mono palettes changed

replaces hard-coded UI text with language keys for english and spanish

3237f33

Q-Bert-Reynolds commented Oct 5, 2024

View reviewed changes

fixes regression where color sprites had incorrect palettes in editor

591b584

Q-Bert-Reynolds force-pushed the dmg-scene-palettes branch from ef1aaac to 591b584 Compare October 5, 2024 16:57

Q-Bert-Reynolds added 3 commits October 6, 2024 01:43

actor and scene editor sprites now display in color

53209d2

background select now uses scene colors where appropriate

736c0da

giving up on colorized backgrounds, they work but I have no idea how …

1df68d9

…to present them the same way as Thumbnail

small refactor of react hooks... I think this is better

d334907

Q-Bert-Reynolds marked this pull request as ready for review October 14, 2024 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for per scene and project wide DMG palettes #1601

Adds support for per scene and project wide DMG palettes #1601

Q-Bert-Reynolds commented Oct 3, 2024 •

edited

Loading

Q-Bert-Reynolds Oct 4, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

Q-Bert-Reynolds Oct 5, 2024

chrismaltby commented Oct 10, 2024

Adds support for per scene and project wide DMG palettes #1601

Are you sure you want to change the base?

Adds support for per scene and project wide DMG palettes #1601

Conversation

Q-Bert-Reynolds commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismaltby commented Oct 10, 2024

Q-Bert-Reynolds commented Oct 3, 2024 •

edited

Loading