TIC-80 icon indicating copy to clipboard operation
TIC-80 copied to clipboard

Suggestion: Refactor to avoid naming confusion with core/memory

Open joshgoebel opened this issue 3 years ago • 4 comments

Here is just a little work I did... and this would be a much larger endeavor... but I don't want to invest the time unless you're 100% behind this... basically the single starter goal would be to clean up all the "what object are we passing" confusion. We could start with only obvious places like core.c (ignoring the API functions for now)... ie, code like this:

void tic_core_close(tic_mem* memory)
{
    tic_core* core = (tic_core*)memory;

    core->state.initialized = false;

    tic_close_current_vm(core);

    blip_delete(core->blip.left);
    blip_delete(core->blip.right);

    free(memory->samples.buffer);
    free(core);
}

What is confusing here:

  • This code is in the core.c file, yes does not work with cores.
  • This is a "TICCore" "class" function (in C terms) because of it's tic_core_* naming, yet it takes a tic_mem* as input.
  • It mostly doesn't care about tic_mem* at all, using only a single field.
  • ...instead casting it to tic_core* and using that the entire time...

IE, this would be much more clearly written as:

void tic_core_close(tic_core* core)
{
    core->state.initialized = false;

    tic_close_current_vm(core);

    blip_delete(core->blip.left);
    blip_delete(core->blip.right);

    free(core->memory.samples.buffer);
    free(core);
}

IE, this is:

  • a tic_core_* (TICCore) "class" function
  • it takes a tic_core * as input
  • it does actual work on that tic_core *
  • enclosed in core.c... where it should be

This same logic would apply to every tic_core_* function, etc... I didn't dig fully into fixing the naming here but we'd also apply consistent naming everywhere:

  • core would refer to tic_core instances, etc
  • mem would refer to tic_mem instance, etc

The generic tic would be used much less as it's ambiguous about what it holds (state? core? data? mem?)

joshgoebel avatar Jan 10 '22 20:01 joshgoebel

I'd probably prefer just using the larger struct when we can vs creating a local pointer.

static inline void updpal(tic_core* core, tic_blitpal* pal0, tic_blitpal* pal1)
{
    tic_mem* mem = (tic_mem*)core;
    *pal0 = tic_tool_palette_blit(&vbank0(core)->palette, mem->screen_format);
    *pal1 = tic_tool_palette_blit(&vbank1(core)->palette, mem->screen_format);
}

after

static inline void updpal(tic_core* core, tic_blitpal* pal0, tic_blitpal* pal1)
{
    *pal0 = tic_tool_palette_blit(&vbank0(core)->palette, core->memory.screen_format);
    *pal1 = tic_tool_palette_blit(&vbank1(core)->palette, core->memory.screen_format);
}

At least in simple cases where it's just as easy to see what's going on.

joshgoebel avatar Jan 10 '22 20:01 joshgoebel

I got your idea and this will work, the only thing I worry about is tic_core struct visibility, we have to hide its declaration for external members like the studio, so the studio shouldn't know anything about tic_core and tic_core_state_data and other internal structs.

nesbox avatar Jan 11 '22 06:01 nesbox

Why? Is this more about "class" field privacy or separation of concerns?

the studio shouldn't know anything about tic_core

It kind of knows a lot actually since it's calling a zillion tic_core_ functions... the only thing you're hiding is the internals of tic_core (which I guess is something)... it feels to me like Studio is orchestrating FAR too much though... if it's not supposed to be know about core, then it shouldn't need to call any tic_core_ functions. To me (just because of the calls themselves) core and studio are already very entangled. Studio in many ways is acting as the "OS" layer...

If studio is meant to be "just another app" then a lot could be done to make that boundary a lot sharper I think.

Let me try to abstract one piece and show you...


What sounds does studio make? I see studioSound... are there sounds with clicking any UI or does this exist just for the sfx editor?

joshgoebel avatar Jan 11 '22 14:01 joshgoebel

Why? Is this more about "class" field privacy or separation of concerns?

It's more about "class" field privacy, pls keep all the tic_core members hidden from the outside.

are there sounds with clicking any UI or does this exist just for the sfx editor?

There are also sounds in the Surf and Game menu, also we have a sound when TIC80 starts, look at the playSystemSfx() function.

nesbox avatar Jan 12 '22 06:01 nesbox