better/human readable C code for resources

[untoxa]

Is your feature request related to a problem? Please describe. GB Studio generates enormous blobs of data in byte arrays that are not readable. And sometimes all that is working by a happy coincidience. Current solution is very fragile, you can't move or insert anything into the data files even between the declarations. Compiler may also just throw out unused objects, luckuly SDCC does not bother itself with that at the moment.

Describe the solution you'd like Refactoring that stuff.

Describe alternatives you've considered Lets examine the ejected sources of the sample project. In src/music/music_bank_11.c we see a bunch of arrays music_track_*__Data that are not referenced from anywhere, and there is:

const unsigned int music_tracks[] = {
0x6C1D, 0x6C37, 0x6C41, 0x6C45, 0x6C59, 0x6C5D, 0x6C67, 0x6C6B, 0x6C71, 0
};

and the only place where that music_tracks is used:

gbt_play((void*)music_tracks[index], music_bank, 7);

where it casts integers into pointers.

after compiling, we see this in the symbol file:

0b:6c1d _music_track_101__Data

so, that 0x6C1D happens to be a pointer to _music_track_101__Data. why the hell calculating those offsets?! why not just:

extern const void * const music_tracks[];

and:

const void * const music_tracks[] = {
&music_track_101__Data, &music_track_102__Data, &music_track_103__Data, &music_track_104__Data, &music_track_105__Data, &music_track_106__Data, 
&music_track_107__Data, &music_track_108__Data, &music_track_109__Data, 0
};

?!

very nice ASM is emitted for that:

_music_tracks:
    .dw _music_track_101__Data
    .dw _music_track_102__Data
    .dw _music_track_103__Data
    .dw _music_track_104__Data
    .dw _music_track_105__Data
    .dw _music_track_106__Data
    .dw _music_track_107__Data
    .dw _music_track_108__Data
    .dw _music_track_109__Data
    .dw #0x0000

There are many other places where calculation of offsets can be heavied onto the compiler. THAT DOES NOT SLOW COMPILATION. That simplifies the JS part of GB Studio. That's a valid C code at last, that has no magic inside!

That also opens possibility of efficient bank stuffing before linking stage as described here: https://github.com/chrismaltby/gb-studio/pull/588#issuecomment-723925904

[untoxa]

what i started to do:

1. i removed BankDataPtr macro. it is useless, just yields 0x4000 which is a bit dumb, offsets will contain that if they reference the other symbol. also there was some code all over the core like: active_script_ctx.script_ptr = (BankDataPtr(script_ctxs[ctx].active_script_ctx.script_ptr_bank)) + events_ptr->offset;, that yields 0x4000 + events_ptr->offset, does not matter what clever code you write as a parameter of a macro. removing that simplified everything.

2. i defined BankPtr as:

   typedef struct _BankPtr {
   UBYTE bank;
   UBYTE * offset;
   } BankPtr;

3. arrays in data_ptrs.c become:

   const BankPtr tileset_bank_ptrs[] = {
   {0x06,&bank_6_data[0x2585]}, {0x06,&bank_6_data[0x2FF6]}, {0x07,&bank_7_data[0]}, {0x07,&bank_7_data[0x0681]}
   };

and... it breaks. when debugging it loads graphics data fine into vram, but then something goes wrong inside ScriptRunnerUpdate(), not sure what happens here.

i also noticed mixing of big-endian and little-endian offsets: LoadImage((*(data_ptr++) * 256) + *(data_ptr++)); and: actors[i].movement_ptr.offset = *(data_ptr++) + (*(data_ptr++) * 256);

that adds some mess

[chrismaltby]

Thanks untoxa, this all looks well needed.

On the topic of the BankDataPtr macro I've been meaning to remove that for a while, it's left over from an earlier build where I was cross compiling using Emscripten to build a version for debugging. There was previously an #ifdef __EMSCRIPTEN__ around that point to handle loading the banked data differently for that build. I've long since abandoned that version though so you're right it's not needed at all anymore.

[untoxa]

i gave it one more try, and... it worked! look: 8.zip

now look at the data_ptrs.c -- much better. there is still BankDataPtr definition, and it is used in some places, seems that some offsets, emitted by JS, require that.

[untoxa]

all in-script poiter stuff is big-endian and all require to be corrected by 0x4000, am i right?

[chrismaltby]

Yeah it looks like it's mostly big-endian, though like you say there's a few places where it's inconsistent right now. Would it be a good idea to switch it all to be little-endian instead (which I believe the Z80 is using internally), would that help with performance, or is it better to just be consistent?

Also I was always adding that 0x4000 offset to get to the right memory address, what you've got there in data_ptrs.c is much nicer!

On the topic of using macros to build the scripts, I've started having a play around with doing something like this

#define FADE_OUT(speed) 0x0C,speed
#define FADE_IN(speed) 0x0D,speed
#define WAIT(frames) 0x0B,frames
#define END_SCRIPT 0x0

const unsigned char bank_6_data[] = {
...
FADE_OUT(5),
WAIT(10),
FADE_IN(2),
WAIT(60),
FADE_OUT(6),
END_SCRIPT,
...
};

Ideally it would probably be something more like:

const unsigned char scene5_init[] = {
  FADE_OUT(5),
  WAIT(10),
  END_SCRIPT
}

with each of the scripts broken up into smaller named arrays but it's a step in the right direction.

To do this fully I'll need to make quite a few changes to the JS as it was previously handling scripts as the big array of numbers to calculate when to split the script into a new bank. I'll just need to make a mapping of each macro to how many bytes it uses or something like that. I've been wanting to refactor that part for quite a while as it's one of the few remaining sections of the application that I've not yet rewritten in Typescript and this would be a good opportunity to write some more tests around the build process.

[untoxa]

@chrismaltby can you please, at first, remove that 0x4000 compensation in the generator (completely remove BankDataPtr macro)?

little endian may also add performance in some cases, when you *((UBYTE **)data_ptr)

and if you declare far pointers as dwords (bank << 16) | offset, then you can return them from functions, that's useful too (but requires additional byte)

[untoxa]

you also don't need to calculate any sizes (only ensure that single object is smaller than 16k) if automatic packing is implemented: https://github.com/chrismaltby/gb-studio/pull/588#issuecomment-723925904

can we speak about that somehow? please, ping me in discord when you have time.

[untoxa]

maybe it is better to generate asm scripts. that will look like this:

scene5_init.s:

    .include    "scripts.s"

    .globl ___bank_Tiles, _Tiles        ; external symbols used

    .globl __bank_scene5_init       ; for constructing far pointer to scene5_init
___bank_scene5_init = 10            ; bank 10, must match area name

    .area   _CODE_10            ; bank 10

_scene5_init::
    FADE_OUT    5
    WAIT        10
1$:
    SET_TILES   ___bank_Tiles, _Tiles   ; construct far pointer
    FADE_IN     2
    WAIT        60
    FADE_OUT    6
    LOOP        1$
    END_SCRIPT

scripts.s:

OP_SET_TILES    = 10
OP_LOOP     = 11

.macro SET_TILES bank, offset
    .db OP_SET_TILES
    .db bank
    .dw offset
.endm
.macro LOOP label
    .db OP_LOOP
    .dw (. - label - 1)         ; calculate relative offset in bytes to the passed label
.endm

scene5_init.h

#ifndef __scene5_init_h_INCLUDE
#define __scene5_init_h_INCLUDE

extern void __bank_scene5_init;
extern char scene5_init[];

#endif

far_ptr.h:

#ifndef __FAR_PTR_H_INCLUDE
#define __FAR_PTR_H_INCLUDE

typedef struct FAR_PTR {
    char bank;
    void * ptr;
} FAR_PTR;

#define __BANK_PREFIX(A) _bank_##A
#define TO_FAR_PTR(A) {.bank = (char)&(__BANK_PREFIX(A)), .ptr = (void *)&(A)}

#endif

then somewhere:

const FAR_PTR scene5_init_far_ptr = TO_FAR_PTR(scene5_init);

[chrismaltby]

Thanks for this @untoxa it's really useful and helped me understand a lot. Piecing this together with what you've written in https://github.com/chrismaltby/gb-studio/pull/588#issuecomment-723925904 so it sounds like if I made GB Studio generate files like this

scene5_init.s:

    .include    "scripts.s"

    .globl __bank_Tiles, _Tiles     ; external symbols used

    .globl __bank_scene5_init       ; for constructing far pointer to scene5_init
__bank_scene5_init = 255            ; bank 255, must match area name

    .area   _CODE_255       ; bank 255

_scene5_init::
    FADE_OUT    5
    WAIT        10
1$:
    SET_TILES   __bank_Tiles, _Tiles    ; construct far pointer
    FADE_IN     2
    WAIT        60
    FADE_OUT    6
    LOOP        1$
    END_SCRIPT

which would make output an object file like...

scene5_init.o:

XL3
H 2 areas 5 global symbols
S __bank_scene5_init Def0000FF
S .__.ABS. Def000000
S _Tiles Ref000000
S __bank_Tiles Ref000000
A _CODE size 0 flags 0 addr 0
A _CODE_255 size 12 flags 0 addr 0
S _scene5_init Def000000
...

Just before linking this would go through a post processing script to determine where to pack data using size 12 as how much space this script takes.

And once a bank was decided this file gets edited to change CODE_255 to _CODE_N and __bank_scene5_init Def0000FF to __bank_scene5_init Def00000N

And once all the data files have been packed and their object files updated then you'd run the linker.

Then I could do the same for things like the scene data potentially having something like

scene5.s:

    .globl __bank_scene5_init, _scene5_init
    .globl __bank_tiles2, _tiles2
    .globl __bank_palette5, _palette5
    .globl __bank_spritesheet8, _spritesheet8

    .globl __bank_scene5_data       ; for constructing far pointer
__bank_scene5_data = 255            ; must match area name

.area   _CODE_255

_scene5_data::
    .db 20      ; width
    .db 18      ; height
    .db __bank_tiles2 ; Tiles
    .dw _tiles2  
    .db __bank_palette5 ; Palette
    .dw _palette5      
    .db __bank_scene5_init ; Init script
    .dw _scene5_init
    .db 1       ; scene type = platformer
    .db 3       ; num actors
    .db 1       ; num triggers

    ; Actor 1
    .db __bank_spritesheet8
    .dw _spritesheet8
    .db 6       ; num frames
    .db 5       ; x
    .db 8       ; y
    .db 2       ; direction
    .db 4       ; move speed
    ; etc..

    ; ; Actor 2, 3
    ; ; etc..

    ; Trigger 1
    .db 20       ; x
    .db 18       ; y
    .db 4        ; width
    .db 1        ; height

Maybe using macros to make this more readable too?

Going to be a lot to change but I think you're right that in the end this will simplify both the JS/Typescript side and definitely make the engine easier to debug/hack on.

I've got a few days off work next week (been pretty busy recently) and want to start having a look at this then, I'll be in touch in Discord as soon as I get to this though :-)

[untoxa]

for the pure data, like tiles or maps you don't need to generate asm. just generate C structs/arrays as usual (not in blob! separate structures!), but in that your post-processing tool EMIT those __bank_<SYMBOL> def<BANK> global exports for every name inside _CODE_<BANK> area, if that __bank_<SYMBOL> does not exist already. don't forget to modify symbol count in H line of the object, and append symbols to the end of the list (in case if object has self-referencing to it's own globals indexes must be unchanged if you don't want to reindex each R line).

[untoxa]

i suggest to emit asm for vm bytecode, because in asm it is very easy to generate offsets and other address arithmetics for IFs, GOTOs and other stuff like that. too much asm causes allergies. :)

[untoxa]

and a very important thing: i suggest to make a standalone tool that is callable from make while compiling ejected sources. or else you can not compile that without GB Studio IDE.

[untoxa]

you don't need to make that tool too smart as i wrote before: https://github.com/chrismaltby/gb-studio/issues/663#issuecomment-731355560 to emit that __bank_<SYMBOL> using C use, just this construction:

#pragma bank 255
const void __at(255) __bank_tiles;
const unsigned char tiles[] = { ... }

[untoxa]

here is a full example:

#include <gb/gb.h>

// below into Test.c (generated by GBS IDE from some png)
const void __at(255) __bank_Test; 
const UINT8 Test[] = {0,1,2,3,4,5,6,7};

// below into BankData.h
#define __BANK_PREFIX(A) __bank_##A
#define TO_FAR_PTR(A) {.bank = (char)&(__BANK_PREFIX(A)), .ptr = (void *)&(A)}

typedef struct far_ptr_t {
    UINT8 bank;
    void * ptr;
} far_ptr_t;

// below into some header where VM stuff is defined
typedef struct actor_t {
    far_ptr_t spritesheet;
    UINT8 n_frames, x, y, dir, speed;
} actor_t;

typedef struct scene_t {
    UINT8 width, height;
    far_ptr_t tiles, palette, init;
    UINT8 type, n_actors, n_triggers;
    actor_t actors[];
} scene_t;

// below into some scene5.c (generated by GBS IDE), must include header that declares extern Test and __bank_Test
const void __at(255) __bank_scene5; 
const struct scene_t scene5 = {
    .width = 20, .height = 18,
    .tiles = TO_FAR_PTR(Test),
    .n_actors = 2,
    .n_triggers = 0,
    .actors = {
        {
            TO_FAR_PTR(Test),
            5, 10, 7, 2
        },
        {
            .spritesheet = TO_FAR_PTR(Test),
            .n_frames = 3, .x = 14, .y = 9
        }
    }
};

asm is exactly as you expect:

_scene5:
    .db #0x14   ; 20
    .db #0x12   ; 18
    .byte ___bank_Test      ; Tiles
    .dw _Test
    .byte #0x00         ; Palette
    .dw #0x0000
    .byte #0x00         ; Init
    .dw #0x0000
    .db #0x00   ; 0
    .db #0x02   ; 2
    .db #0x00   ; 0
; Actor1
    .byte ___bank_Test
    .dw _Test
    .db #0x05   ; 5
    .db #0x0a   ; 10
    .db #0x07   ; 7
    .db #0x02   ; 2
    .db #0x00   ; 0
; Actor2
    .byte ___bank_Test
    .dw _Test
    .db #0x03   ; 3
    .db #0x0e   ; 14
    .db #0x09   ; 9
    .db #0x00   ; 0
    .db #0x00   ; 0

you can't have several variable length arrays in one struct, but, if those substructs are of similar size, you can declare them as a union. or, maybe better, far pointers to arrays of actors, triggers and whatever.

note: that the smaller objects you make, the tighter banks are packed.

[untoxa]

#include <gb/gb.h>

// --- Test.c -------------------------
const void __at(255) __bank_Test; 
const UINT8 Test[] = {0,1,2,3,4,5,6,7};

// --- Test.h -------------------------
// extern const void __bank_Test;
// extern const UINT8 Test[];

// --- BankData.h ---------------------
#define __BANK_PREFIX(A) __bank_##A
#define TO_FAR_PTR(A) {.bank = (char)&(__BANK_PREFIX(A)), .ptr = (void *)&(A)}

typedef struct far_ptr_t {
    UINT8 bank;
    void * ptr;
} far_ptr_t;

// --- VM.h ---------------------------
// #include "BankData.h"
typedef struct actor_t {
    far_ptr_t spritesheet;
    UINT8 n_frames, x, y, dir, speed;
} actor_t;

typedef struct trigger_t {
    far_ptr_t script;
} trigger_t;

typedef struct scene_t {
    UINT8 width, height;
    far_ptr_t tiles, palette, init;
    UINT8 type, n_actors, n_triggers;
    far_ptr_t actors;
    far_ptr_t triggers;
} scene_t;

// --- scene5_actors.c ----------------
// #include "VM.h"
// #include "Test.h"
const void __at(255) __bank_scene5_actors; 
const actor_t scene5_actors[] = {
    {
        TO_FAR_PTR(Test),
        5, 10, 7, 2
    },
    {
        .spritesheet = TO_FAR_PTR(Test),
        .n_frames = 3, .x = 14, .y = 9
    }
};

// --- scene5_actors.h ----------------
// #include "VM.h"
// extern const void __bank_scene5_actors;
// extern const actor_t scene5_actors[];

// --- scene5_triggers.c --------------
// #include "VM.h"
// #include "Test.h"
const void __at(255) __bank_scene5_triggers;
const trigger_t scene5_triggers[] = {
    {
        .script = TO_FAR_PTR(Test)
    },
    {
        .script = TO_FAR_PTR(Test)
    },
    {
        .script = TO_FAR_PTR(Test)
    }
};

// --- scene5_triggers.h --------------
// #include "VM.h"
// extern const void __bank_scene5_triggers;
// extern const trigger_t scene5_triggers[];

// --- scene5.c -----------------------
// #include "VM.h"
// #include "Test.h"
// #include "scene5_actors.h"
// #include "scene5_triggers.h"
const void __at(255) __bank_scene5; 
const struct scene_t scene5 = {
    .width = 20, .height = 18,
    .tiles = TO_FAR_PTR(Test),
    .n_actors = 2,
    .n_triggers = 2,
    .actors = TO_FAR_PTR(scene5_actors),
    .triggers = TO_FAR_PTR(scene5_triggers)
};

[chrismaltby]

Thanks @untoxa I was going to say with the first scene example wasn't sure how to add triggers to the struct since there would be two dynamic length arrays, but this new example solves that problem and looks good, nice and readable :-)

I've made a standalone tool for packing the object files together https://github.com/chrismaltby/gbspack and I've compiled a release for windows/mac/linux at https://github.com/chrismaltby/gbspack/releases/tag/v1.0.0 seemed like a good excuse to finally try using Rust. Though I've noticed you had double underscores in front of the bank number references where I've only done one const void __at(255) _bank_scene5; 🤦 I'll sort that now (I like your version better) and build a new release!

It can just go before the linking step and you give it all the object files containing data and it packs them and updates the CODE_N and S __bank_XXX Def0000NN lines in-place.

[chrismaltby]

Actually never mind, the way it's written the tool works with both const void __at(255) _bank_scene5; and const void __at(255) __bank_scene5; so I only needed to update the README!

[untoxa]

two underscores to reduce probability of accidental usage of that prefix. if you put one underscore in C, then there will be two in OBJ. if two in C, then three in OBJ.