Introduce extracted/VERSION, with text extracted there #1730

Dragorn421 · 2024-02-07T04:04:42Z

The plan ™️ for assets,
(at least it's what I suggested over a year ago and there are no concerns nor other suggestions)
is to extract from the rom into extracted/, and commit assets/**/*.c files #includeing from there.

extracted/ is fully gitignored.

With multiversion I extended this to extract into extracted/VERSION/. extracted/VERSION is added to the include path so that files can include from there using #include "..."

This PR also moves extracting text to extracted/VERSION/text. There are also some changes to the message extraction/build logic I had to make to accommodate for that change.

The extracted text extracted/gc-eu-mq-dbg/text/message_data.h, extracted/gc-eu-mq-dbg/text/message_data_staff.h is included by now-committed (one line include each) assets/text/message_data.h and assets/text/message_data_staff.h (respectively).
Note that this means in a modding context messages can be added into git-tracked assets/text/message_data.h, and the vanilla messages can be removed simply by removing the include.

Discussion/suggestions welcome!

Yanis42 · 2024-02-07T12:10:17Z

I don't understand what's the point of having the version_assets folder when you can extract assets to _extracted/VERSION, seems like you include this path in the makefile so I don't see any reason why this wouldn't work

am I missing something? maybe some edge case I'm not seeing here? or some future change that will make this necessary? 🤔

Dragorn421 · 2024-02-07T17:39:00Z

I think it's more obvious where an include is pulling from as
#include "version_assets/text/message_data.h"
rather than
#include "text/message_data.h"

Currently I'm not even sure the latter would work given the include path also has -Iassets and assets/text/message_data.h also exists
(Note I think we want to no longer -Iassets eventually, so that could be addressed)

cadmic · 2024-02-07T19:03:08Z

I agree with this in principle (extracting assets for different versions into different directories) but I would like to bikeshed the path a bit also.

First, why the underscore in _extracted? I understand the desire to mark this dir as "generated", but we don't do this for any of our other versioned directories (expected, build and baserom segments).

Second, assets/_extracted/$(VERSION)/version_assets is too long for my taste. I'd most like it to be extracted into assets/$(VERSION), since that has a nice symmetry with build/$(VERSION) and baseroms/$(VERSION). The version_assets part should not be necessary if the include paths are in the right order, which might be confusing temporarily but not in the long run if we plan to get rid of -Iassets. For the non-gitignored stuff currently in assets/: assets/text probably doesn't need to exist (it could be generated by the text extractor, especially to deal with NTSC vs PAL), and assets/xml could be moved somewhere else (e.g. a zapd dir), although it's probably fine to leave them where they are for now.

I'd also be okay with a different toplevel directory name other than assets/, although I couldn't think of a good one. But I would like it if the extracted asset dir was topleveldir/$(VERSION).

fig02 · 2024-02-07T19:08:28Z

I have not read this PR in depth yet, but would like to say I agree that I dont think a leading underscore is needed

tools/msgenc.py

assets/text/message_data.h

Dragorn421 · 2024-02-07T20:40:35Z

For the purpose of clarification

So here is the layout proposed in this PR, with text only

assets/
  _extracted/  # everything besides assets/_extracted/** is committed
    gc-eu-mq/
      version_assets/
        text/
          message_data.h
          message_data_staff.h
    gc-eu-mq-dbg/
      version_assets/
        text/
          message_data.h
          message_data_staff.h
  text/
    *.c -> assets/text/message_data(.enc).h, assets/text/message_data_staff(.enc).h
    message_data.h -> version_assets/text/message_data.h
    message_data_staff.h -> version_assets/text/message_data_staff.h

it would generalize to other assets, using the gameplay_keep object as an example, as

assets/
  _extracted/  # everything besides assets/_extracted/** is committed
    gc-eu-mq/
      version_assets/
        objects/
          gameplay_keep/
            hilite_1.rgba16.png
            gLinkPauseChildJointTable.inc.c
        text/
          message_data.h
          message_data_staff.h
    gc-eu-mq-dbg/
      version_assets/
        objects/
          gameplay_keep/
            hilite_1.rgba16.png
            gLinkPauseChildJointTable.inc.c
        text/
          message_data.h
          message_data_staff.h
  objects/
    gameplay_keep/
      gameplay_keep.c -> version_assets/objects/gameplay_keep/hilite_1.rgba16.inc.c, version_assets/objects/gameplay_keep/gLinkPauseChildJointTable.inc.c
      gameplay_keep.h
  text/
    *.c -> assets/text/message_data(.enc).h, assets/text/message_data_staff(.enc).h
    message_data.h -> version_assets/text/message_data.h
    message_data_staff.h -> version_assets/text/message_data_staff.h

EDIT: fixed forgotten assets/_extracted/gc-eu-mq-dbg/version_assets/objects/gameplay_keep/

Dragorn421 · 2024-02-16T11:49:06Z

Updated to "future layout v3"

Productive discussion on discord happened
from https://discord.com/channels/688807550715560050/688851317593997489/1207388581639880745
to https://discord.com/channels/688807550715560050/688851317593997489/1208006804127088692 + possibly ongoing

The general result is a new top level folder extracted/ for extracted things, including assets, text and baserom segments

Include path would be like -Ibuild/VERSION -Iextracted/VERSION

in the following structure layouts:
[first/part/]second/part paths are where the file does #include "second/part" and CPP resolves it to first/part/second/part

I compiled the changes to "future layout v2" (outdated):

# v2
assets/  # note: everything under assets/ committed
  objects/
    gameplay_keep/
      gameplay_keep.c -> [extracted/VERSION/]objects/gameplay_keep/gameplay_keepVtx_00C0A0.vtx.inc , [extracted/VERSION/]objects/gameplay_keep/deku_stick.i8.inc.c
      gameplay_keep.h
  text/
    nes_message_data_static.c -> [extracted/VERSION/]text/message_data.enc.h
  xml/
    objects/
      gameplay_keep.xml
extracted/  # .gitignore'd
  VERSION/
    baserom/
      gameplay_keep
    objects/
      gameplay_keep/
        deku_stick.i8.png  # written on make setup for reference, but otherwise unused
        deku_stick.i8.inc.c  # written on make setup (does *not* depend on the png)
        gameplay_keepVtx_00C0A0.vtx.inc
    text/
      message_data.h
      message_data.enc.h  # processed from ./message_data.enc.h
build/
  VERSION/
    baserom/
      gameplay_keep.o  # objcopy'd from extracted/VERSION/baserom/gameplay_keep

but realized while updating this PR I oversimplified message_data.h handling. Here's the "future layout v3" that fixes that:

# v3
assets/  # note: everything under assets/ committed
  objects/
    gameplay_keep/
      gameplay_keep.c -> [extracted/VERSION/]objects/gameplay_keep/gameplay_keepVtx_00C0A0.vtx.inc , [extracted/VERSION/]objects/gameplay_keep/deku_stick.i8.inc.c
      gameplay_keep.h
  text/
    nes_message_data_static.c -> [build/VERSION/]assets/text/message_data.enc.h
    message_data.h -> [extracted/VERSION/]text/message_data.h
  xml/
    objects/
      gameplay_keep.xml
extracted/  # .gitignore'd
  VERSION/
    baserom/
      gameplay_keep
    objects/
      gameplay_keep/
        deku_stick.i8.png  # written on make setup for reference, but otherwise unused
        deku_stick.i8.inc.c  # written on make setup (does *not* depend on the png)
        gameplay_keepVtx_00C0A0.vtx.inc
    text/
      message_data.h
      message_data.enc.h  # processed from ./message_data.enc.h
build/
  VERSION/
    baserom/
      gameplay_keep.o  # objcopy'd from extracted/VERSION/baserom/gameplay_keep
    assets/
      text/
        message_data.enc.h  # processed from assets/text/message_data.h

diff from v2:

--- dirlayout_future_v2.txt     2024-02-16 12:40:29.688695211 +0100
+++ dirlayout_future_v3.txt     2024-02-16 12:40:35.436757360 +0100
@@ -1,27 +1,31 @@
-# v2
+# v3
 assets/  # note: everything under assets/ committed
   objects/
     gameplay_keep/
       gameplay_keep.c -> [extracted/VERSION/]objects/gameplay_keep/gameplay_keepVtx_00C0A0.vtx.inc , [extracted/VERSION/]objects/gameplay_keep/deku_stick.i8.inc.c
       gameplay_keep.h
   text/
-    nes_message_data_static.c -> [extracted/VERSION/]text/message_data.enc.h
+    nes_message_data_static.c -> [build/VERSION/]assets/text/message_data.enc.h
+    message_data.h -> [extracted/VERSION/]text/message_data.h
   xml/
     objects/
       gameplay_keep.xml
 extracted/  # .gitignore'd
   VERSION/
     baserom/
       gameplay_keep
     objects/
       gameplay_keep/
         deku_stick.i8.png  # written on make setup for reference, but otherwise unused
         deku_stick.i8.inc.c  # written on make setup (does *not* depend on the png)
         gameplay_keepVtx_00C0A0.vtx.inc
     text/
       message_data.h
       message_data.enc.h  # processed from ./message_data.enc.h
 build/
   VERSION/
     baserom/
       gameplay_keep.o  # objcopy'd from extracted/VERSION/baserom/gameplay_keep
+    assets/
+      text/
+        message_data.enc.h  # processed from assets/text/message_data.h

For reference, here is "current zeldaret/main layout v1":

-Ibuild/gc-eu-mq-dbg

assets/
  objects/  # .gitignore'd
    gameplay_keep/
      gameplay_keep.c -> ./gameplay_keepVtx_00C0A0.vtx.inc , [build/gc-eu-mq-dbg/]assets/objects/gameplay_keep/deku_stick.i8.inc.c
      gameplay_keep.h
      deku_stick.i8.png
      gameplay_keepVtx_00C0A0.vtx.inc
  text/
    message_data.h  # .gitignore'd
    nes_message_data_static.c -> [build/gc-eu-mq-dbg/]assets/text/message_data.enc.h
  xml/
    objects/
      gameplay_keep.xml
baseroms/
  gc-eu-mq-dbg/
    segments/
      gameplay_keep  # written on make setup
build/
  gc-eu-mq-dbg/
    assets/
      objects/
        gameplay_keep/
          deku_stick.i8.inc.c  # processed from assets/objects/gameplay_keep/deku_stick.i8.png on make (depends on the png)
      text/
        message_data.enc.h  # processed from assets/text/message_data.h
    baserom/
      gameplay_keep.o  # objcopy'd from baseroms/gc-eu-mq-dbg/segments/gameplay_keep on make

Makefile

assets/.gitignore

Makefile

Dragorn421 · 2024-02-17T11:46:25Z

I found a small issue specific to text processing due to the encoding step done by msgenc.py,
errors in message_data.h are reported as being in message_data.enc.h at random lines,
due to message_data.enc.h being processed from message_data.h without linemarkers (with -P cpp flag)

e.g. given assets/text/message_data.h :

#include "text/message_data.h"

DEFINE_MESSAGE(0x1111, TEXTBOX_TYPE_BLUE, TEXTBOX_POS_BOTTOM,
hi
,
""
,
""
)

the following error happens on make:

cfe: Error: build/gc-eu-mq-dbg/assets/text/message_data.enc.h, line 38939: Syntax Error
 const char _message_0x1111_nes[sizeof( hi )] = {  hi  "\x02"     }; 
 -----------------------------------------------------------------^
The token read was unexpected.
cfe: Warning 625: build/gc-eu-mq-dbg/assets/text/message_data.enc.h, line 38939: Empty declaration
 const char _message_0x1111_nes[sizeof( hi )] = {  hi  "\x02"     }; 
 ------------------------------------------------------------------^
Empty declarations are invalid in standard C.
Failed to compile file assets/text/nes_message_data_static.c.

Note it says build/gc-eu-mq-dbg/assets/text/message_data.enc.h, line 38939 instead of a more helpful hypothetical assets/text/message_data.h at line 4

I fixed (worked around) this with Dragorn421@42e4d4a (not in this PR) but it's not super great. Especially hardcoding the ido path because CC is asm-processor-wrapped. And though it works, msgenc.py wasn't written with handling cpp'd input in mind (e.g. take care not to touch linemarker filename strings)

Note there is a similar issue in main anyway, errors also report message_data.enc.h and msgenc.py may shift lines due to replacing comments / joining continuation lines

I think I'd probably want to rewrite msgenc.py as well.

Handle this in this PR or just live with errors on text stuff compile being reported in message_data.enc.h for now?

fig02 · 2024-02-20T12:44:57Z

Re: message_enc stuff above
If the problem already exists in main and is not a product of the changes in this PR, seems best to handle later, especially if you are just going to rewrite the script

fig02

My approval is from the standpoint of "I agree with the idea of these changes" and "works on my machine"

Do encourage build system-savy people to continue to review the internals of this, like is happening above

Thar0

Text system nitpick time

assets/text/nes_message_data_static.c

…FINE_MESSAGE_NES` to handle its special behavior

Thar0 · 2024-02-29T01:56:30Z

assets/text/message_data.h

@@ -0,0 +1 @@
+#include "text/message_data.h"


In view of adding includes below this one for modding, the vanilla text debugger won't recognize them anymore since it stops searching at message 0xFFFD. I suggest adding messages 0xFFFC and 0xFFFD to this header instead of in the extracted header

If we still want to extract them, could possibly just add them to a separate message_data_end.h that gets included below message_data.h. Then custom text data could go in between.

Yeah I was considering a footer include as well as an option, extracted or not

Not sure yet what to do, anyone feel free to chip in
I won't get to this immediately

I think including them in the committed header works fine

d75522b Move messages 0xFFFC, 0xFFFD to committed message_data.h

and explanation comment

cadmic

I don't have an opinion on message 0xFFFC, but the build system and structure look good to me

Introduce assets/_extracted/VERSION, with text extracted there

4210927

Dragorn421 added Needs first approval Needs second approval labels Feb 7, 2024

Dragorn421 mentioned this pull request Feb 7, 2024

Hacks for building nes_font_static and nes_message_data_static with gc-eu-mq #1721

Merged

cadmic reviewed Feb 7, 2024

View reviewed changes

tools/msgenc.py Outdated Show resolved Hide resolved

assets/text/message_data.h Outdated Show resolved Hide resolved

Dragorn421 added 2 commits February 16, 2024 11:53

Merge branch 'main' into meta_assets_extracted_dir

49fac3d

move to extracted/text/

3b9ced8

Dragorn421 changed the title ~~Introduce assets/_extracted/VERSION, with text extracted there~~ Introduce extracted/VERSION, with text extracted there Feb 16, 2024

Dragorn421 added 2 commits February 16, 2024 12:53

Update gitignore s

9a630fd

rework args for msgenc.py

f36ccd2

cadmic reviewed Feb 16, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

assets/.gitignore Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

fig02 approved these changes Feb 20, 2024

View reviewed changes

Thar0 suggested changes Feb 28, 2024

View reviewed changes

assets/text/nes_message_data_static.c Outdated Show resolved Hide resolved

Dragorn421 added 4 commits February 28, 2024 22:44

Merge branch 'main' into meta_assets_extracted_dir

03bcaf1

put mkdir with others, until theyre all moved at once

553dcbe

move 0xFFFC back to being extracted, making it use specific macro `DE…

fe78130

…FINE_MESSAGE_NES` to handle its special behavior

prettier gitignore

8822955

Dragorn421 mentioned this pull request Feb 28, 2024

New assets system #1904

Open

Thar0 reviewed Feb 29, 2024

View reviewed changes

cadmic approved these changes Mar 1, 2024

View reviewed changes

Dragorn421 added 2 commits March 1, 2024 22:21

Merge branch 'main' into meta_assets_extracted_dir

313f4f5

Move messages 0xFFFC, 0xFFFD to committed message_data.h

d75522b

fig02 approved these changes Mar 1, 2024

View reviewed changes

Thar0 approved these changes Mar 1, 2024

View reviewed changes

Dragorn421 removed the Needs first approval label Mar 1, 2024

hensldm approved these changes Mar 2, 2024

View reviewed changes

fig02 merged commit a6f646d into zeldaret:main Mar 2, 2024
1 check passed

Dragorn421 deleted the meta_assets_extracted_dir branch March 2, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce extracted/VERSION, with text extracted there #1730

Introduce extracted/VERSION, with text extracted there #1730

Dragorn421 commented Feb 7, 2024 •

edited

Loading

Yanis42 commented Feb 7, 2024

Dragorn421 commented Feb 7, 2024

cadmic commented Feb 7, 2024 •

edited

Loading

fig02 commented Feb 7, 2024

Dragorn421 commented Feb 7, 2024 •

edited

Loading

Dragorn421 commented Feb 16, 2024

Dragorn421 commented Feb 17, 2024

fig02 commented Feb 20, 2024

fig02 left a comment

Thar0 left a comment

Thar0 Feb 29, 2024 •

edited

Loading

hensldm Mar 1, 2024 •

edited

Loading

Dragorn421 Mar 1, 2024

fig02 Mar 1, 2024

Dragorn421 Mar 1, 2024

cadmic left a comment

Introduce extracted/VERSION, with text extracted there #1730

Introduce extracted/VERSION, with text extracted there #1730

Conversation

Dragorn421 commented Feb 7, 2024 • edited Loading

Yanis42 commented Feb 7, 2024

Dragorn421 commented Feb 7, 2024

cadmic commented Feb 7, 2024 • edited Loading

fig02 commented Feb 7, 2024

Dragorn421 commented Feb 7, 2024 • edited Loading

Dragorn421 commented Feb 16, 2024

Dragorn421 commented Feb 17, 2024

fig02 commented Feb 20, 2024

fig02 left a comment

Choose a reason for hiding this comment

Thar0 left a comment

Choose a reason for hiding this comment

Thar0 Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

hensldm Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Dragorn421 Mar 1, 2024

Choose a reason for hiding this comment

fig02 Mar 1, 2024

Choose a reason for hiding this comment

Dragorn421 Mar 1, 2024

Choose a reason for hiding this comment

cadmic left a comment

Choose a reason for hiding this comment

Dragorn421 commented Feb 7, 2024 •

edited

Loading

cadmic commented Feb 7, 2024 •

edited

Loading

Dragorn421 commented Feb 7, 2024 •

edited

Loading

Thar0 Feb 29, 2024 •

edited

Loading

hensldm Mar 1, 2024 •

edited

Loading