Datafiles, refactoring parsers and game initialization?

[Marco]

Hi ,

as my first post, I have quite a lot of things to tell. First of all thanks to all the people who contributed and mantained Angband over the years.

I met the V Angband code around the 3.x versions, on my glorious Amiga. I planned in the past to do great solo things in Angband development, but you know what is the end of great individual plans, moreover if one don't knows at all what a rogue-like game is , so all I did was visiting sometimes the sites and the forum, downloading some updated versions, and having a look at the evolution of the code.

In these days I'm sick at home, so I started again to play with Angband source code with a more realistic approach... I setup a dev environment on my Win PC (Mingw/Cygwin, CodeBlocks, local SVN), forked my copy on GitHub, subscribed the Forum and checked the IRC channel. I'm experienced in C/C++ and other languages development, I like O.O. programming, code organization and refactoring. I hope I can contribute on the last two themes, if I will not be completely distracted again by future life and work events. BTW, over the years I had no advancement in undestanding what a rogue-like game is . I'm also totally new at Git/GitHub and, surprisingly, IRC.

Sorry for the long OT introduction. Turning back to the subject, I took a look at the game initialization and datafiles parsing. I had to write down a list/map of all this stuffs, because I got lost several times wandering the code:

I don't know if what follows is a concern on not, debated or less, and if someone is working on this. IMO, a few points:

• The stores and dungeon init/parse functions could be merged at the tail of the others array initialization and cleanup, in the pl[] array (init.c). It seems to work. The init_modules store_ and generate_ would no longer be necessary
• All the File Parsers could be grouped in a few init-something files, like i.e. the z-something and ui-something groups, just to better separate and identify this support functionality from the main game logic. The issue, I think, is moving things away from old files, where people is used to found them, to other or new files . It is a problem to increase the number of .c/.h files?
• The FilePaser struct could comprise both the filename to parse (the current unused 'name' field) and the description to print (Initalizing... /Cannot initialize ...). Or both could be placed in the pl[] array? I implemented the first solution.
• The grab_something functions could be moved elsewhere, because they don't deal directly with datafiles, but I can't figure where. Maybe in parser.c, as ancillary parsing functions?
• I also would like to better clarify in the code the roles/relations of (basic) Parser and FileParser, the latter sitting on top of the former one. There is something that escapes me, but I can't grasp exactly what. My fault maybe

Ok, it is more than enough. Thank you for following me until here . Any comment is really appreciated .

If any of the previous points is of interest, I need some help to clear up minor doubts and eventually submit the changes. Anyway, I had my fun and pleasure in exploring the always interesting and challenging Angband code, and in letting you know . Conversely, let me know if I can contribute in some other way.

Hugs, Marco.

[Nick]

Interesting post, and you make some good points. Since becoming maintainer most of the work I have done on datafile parsing has been adding new stuff to parse. I have done a little re-organisation here and there, but have mainly been adding to your diagram rather than simplifying it

Anything I say below is from my limited understanding; comment from takkaria and other devs might invalidate some of it.

Now, my comments:

• The stores and dungeon init/parse functions could be merged at the tail of the others array initialization and cleanup, in the pl[] array (init.c). It seems to work. The init_modules store_ and generate_ would no longer be necessary

As far as I can see, this leaves the modules for non-datafile game data, and combines all the datafile data into the arrays module. This seems like a good idea, and also suggests to me that some of the non-datafile stuff should make its way into datafiles (messages is one I've long wanted to do, but it is a big job).

• All the File Parsers could be grouped in a few init-something files, like i.e. the z-something and ui-something groups, just to better separate and identify this support functionality from the main game logic. The issue, I think, is moving things away from old files, where people is used to found them, to other or new files . It is a problem to increase the number of .c/.h files?

This also seems like a reasonable idea. Originally all the parsing was in one file (well, two, but that's another story), and breaking it up as far as it currently is was probably a good idea. However, I think having init as a prefix (so init-obj rather than obj-init) is a decent plan. It's probably not a good idea to break up completely into one init file per datafile; object.txt, ego_item.txt and artifact.txt, for example, have enough things used in common (arrays of flags, etc) that it probably makes sense to keep them all in one file.

• The FilePaser struct could comprise both the filename to parse (the current unused 'name' field) and the description to print (Initalizing... /Cannot initialize ...). Or both could be placed in the pl[] array? I implemented the first solution.

That seems completely sensible and much simpler.

• The grab_something functions could be moved elsewhere, because they don't deal directly with datafiles, but I can't figure where. Maybe in parser.c, as ancillary parsing functions?
• I also would like to better clarify in the code the roles/relations of (basic) Parser and FileParser, the latter sitting on top of the former one. There is something that escapes me, but I can't grasp exactly what. My fault maybe

So my understanding is this:

• parser.c is the low level file which sets up functions like parser_getint() that the individual file parsers can use, and functions that create and destroy individual parsers;
• datafile.c I made from pulling higher level stuff out of parser.c and lower level stuff out of the various file parsers; it's meant to be all the code that's more Angband-specific than parser.c, but applicable across lots of datafiles;
• datafile.c also contains file handling routines for reading (usually by calling functions from parser.c) and writing datafiles;
• Then the individual datafile parsers are the highest level.

So maybe datafile.c should contain just the file handling stuff, and the grab_*() functions could be moved to another file - init-util.c, maybe, if we're using init-*.c for intialisation files?

Thinking about this has been very helpful for me - I hope it has helped you a bit too.

[Marco]

Thanks a lot Nick.

As far as I can see, this leaves the modules for non-datafile game data, and combines all the datafile data into the arrays module. This seems like a good idea, and also suggests to me that some of the non-datafile stuff should make its way into datafiles (messages is one I've long wanted to do, but it is a big job).

You explained it more more clearly than I thought BTW messages is really a big job!

Originally all the parsing was in one file (well, two, but that's another story), and breaking it up as far as it currently is was probably a good idea. However, I think having init as a prefix (so init-obj rather than obj-init) is a decent plan. It's probably not a good idea to break up completely into one init file per datafile; object.txt, ego_item.txt and artifact.txt, for example, have enough things used in common (arrays of flags, etc) that it probably makes sense to keep them all in one file.

Yes, I can remember the old init1.c and init2.c . A good trade off IMO could be having a few files, just renaming obj-init and mon-init and adding the siblings for player, dungeon&stores, and the game as a whole (or misc stuffs).

That seems completely sensible and much simpler.

Got it!

So maybe datafile.c should contain just the file handling stuff, and the grab_*() functions could be moved to another file - init-util.c, maybe, if we're using init-*.c for intialisation files?

It seems to me like a good solution, if it is good for you and the other developers also. However, I have to admit that it is not strictly necessary, at least as a first step. We could start with the first three bullet of my original post, and see the result. Sorry, I'm always tempted to reorganize things a little too much, and someone once told to me that the "best" is enemy of the good.

All your other explanations have also been very helpful to me! Now it's time for me to contiune writing some code, I suppose...

[Gwarl]

someone once told to me that the "best" is enemy of the good.

The English proverb is "Don't let the perfect be the enemy of the good".

Not trying to be pedantic, I thought you might appreciate knowing the exact wording of the proverb.

[Marco]

The English proverb is "Don't let the perfect be the enemy of the good".

Not trying to be pedantic, I thought you might appreciate knowing the exact wording of the proverb.

Yes Gwarl, I appreciate a lot, you guessed it Thanks!

[Marco]

Just to let you know it, I made a commit in my angband branch for the first step of refactoring datafile parsers organization. I think that who is interested can explore the changes on my GitHub branch (MarcoAngband), or I have to do a merge request?
Beyond the .c/.h modifications, the makefiles are yet to be updated. I can try to do this.
As you can see I also have a new signature I apologize for my English, sorry.

[Nick]

Just to let you know it, I made a commit in my angband branch for the first step of refactoring datafile parsers organization. I think that who is interested can explore the changes on my GitHub branch (MarcoAngband), or I have to do a merge request?
Beyond the .c/.h modifications, the makefiles are yet to be updated. I can try to do this.

I suggest getting your branch to a state where it compiles and runs before committing. Probably the only thing you need to change is Makefile.src; you need to remove files from the list that you've deleted, and add your new ones.

When you've done that, you can either make a pull request, or just mentio it here and I can pull your code in directly.

[Pete Mack]

I also suggest stsrting with an unmodified branch, and rename the files you started from so we can see your changes. Right now, it is very hard to figure out what you have done.

$ git mv monster-init.c init-monster.c
...
$ git commit
$ git checkout init-monster.c
....

[Marco]

I suggest getting your branch to a state where it compiles and runs before committing. Probably the only thing you need to change is Makefile.src; you need to remove files from the list that you've deleted, and add your new ones.

Currently the branch I commited compiles as a CodeBlock project upon a MinGW compiler, hopefully with the same compile/link options of the Angband makefile. It runs apparently without errors, but I tested it only a little, playing for a few minutes, so this is not a guarantee.
I searched all the configure/make files for occurences of sourcefile names, and I found only two of them worthy of attention, Makefile.src and Makefile.inc, but I'm not sure which one (or both) to change, following what you outline.

When you've done that, you can either make a pull request, or just mentio it here and I can pull your code in directly.

Thanks. I realized that I need some more time to better understand how to correctly setup my development environment and how to use/combine git commands (see also the helpful Pete Mack's reply). I was already reading extensively the Forum, and also some git tutorial, but git is still really hard to me.

The idea of maintaining a local SVN to track changes locally and committing only the final result on GitHub was not so good, or maybe not good at all. I guess I can work directly on my local (PC) copy of my branch.

[Marco]

I also suggest stsrting with an unmodified branch, and rename the files you started from so we can see your changes. Right now, it is very hard to figure out what you have done.

I agree with you. I understand that dev team and the maintainer need to fully evaluate if the changes are desirable/reliable or not.
I had already chosen to separate this refactoring in two phases, the first to reorganize the code between the source files (2^ and 4^ bullet of my original post), with minimal changes to the code, and the second to review the activation of the parsers by the init modules (1^ and 3^ bullet). But the first phase is quite tricky.

$ git mv monster-init.c init-monster.c
...
$ git commit
$ git checkout init-monster.c
....

Thanks for the snippet. Sorry, but I used the GitHub Desktop to commit all the changes as a whole, because I am still very confused by the git terminology and commands. It's time for me to study this, I see. Every moment I'm afraid of making mistakes.

In this first phase, I need to mv two sourcefiles (mon_ & obj_init.c), deleting the respective .h, and create three others new init-xxx.c, then copy the code of some file_parsers in the new destinations and delete it from the old. In this process, some other changes are required, and I will do my best to keep them at the minimum, even if my editor discards the whitespaces at EOL. I plan to keep only one single init.h, the same way there is only a single cmds.h. I would like to incorporate hint.h into store.h and then delete it, but this is not strictly necessary.

How can I do all that? It's not only a matter of bare git commands syntax knowledge, but how I can use them to better clarify what I'm doing. Maybe I missed some guidelines, my fault.

I can undo my current commit and restart anew? I can mv and rm in a single pass the above files, then issue one commit? Then I can create the new files, move the code, modify the makefiles and finally issue another commit? I think I can group the commits in a single pull request.

[Marco]

I searched all the configure/make files for occurences of sourcefile names, and I found only two of them worthy of attention, Makefile.src and Makefile.inc, but I'm not sure which one (or both) to change, following what you outline.

I realized that Makefile.inc is generated by the depgen make target. In my case:

mingw32-make -f Makefile.win depgen

So the only file I need to update manually is Makefile.src.

But running that command on the current (unmodified) angband src, I get the following diffs

$ diff Makefile.inc Makefile.new
223,224c223,224
<  list-mon-spells.h init.h mon-make.h mon-spell.h mon-util.h mon-msg.h \
<  list-mon-message.h player-util.h store.h cmd-core.h trap.h \
---
>  list-mon-spells.h init.h [COLOR="Yellow"]mon-group.h[/COLOR] mon-make.h mon-spell.h mon-util.h \
>  mon-msg.h list-mon-message.h player-util.h store.h cmd-core.h trap.h \
991c991
<  list-square-flags.h list-terrain-flags.h ui-keymap.h
---
>  list-square-flags.h list-terrain-flags.h ui-keymap.h [COLOR="yellow"]snd-win.h[/COLOR]

The first occurs in the dependencies of gen-cave.o, the second in the dependencies of sound-core.o. I expected to obtain no differences.

I am making several progress with git, although not everything is still clear to me, and I have understood that I must be more disciplined and have a more atomic and restricted approach to the modifications to Angband's sources, let me say homeopathic. There were several mistakes in what I did before.
I hope that my contribution that I anticipated, properly revised, is always welcome.