Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old October 31, 2019, 17:30   #1
Marco
Rookie
 
Marco's Avatar
 
Join Date: Oct 2019
Location: Italy
Age: 58
Posts: 7
Marco is on a distinguished road
Smile Datafiles, refactoring parsers and game initialization?

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.
Attached Files
File Type: txt Angband-4.2.0-Datafiles.txt (4.5 KB, 15 views)
Marco is offline   Reply With Quote
Old November 1, 2019, 00:27   #2
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 54
Posts: 7,849
Donated: $60
Nick will become famous soon enough
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:

Quote:
Originally Posted by Marco View Post
  • 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).

Quote:
Originally Posted by Marco View Post
  • 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.

Quote:
Originally Posted by Marco View Post
  • 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.

Quote:
Originally Posted by Marco View Post
  • 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.
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is online now   Reply With Quote
Old November 2, 2019, 16:33   #3
Marco
Rookie
 
Marco's Avatar
 
Join Date: Oct 2019
Location: Italy
Age: 58
Posts: 7
Marco is on a distinguished road
Thanks a lot Nick.


Quote:
Originally Posted by Nick View Post
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!


Quote:
Originally Posted by Nick View Post
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).


Quote:
Originally Posted by Nick View Post
That seems completely sensible and much simpler.
Got it!


Quote:
Originally Posted by Nick View Post
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...
Marco is offline   Reply With Quote
Old November 5, 2019, 16:47   #4
Gwarl
Knight
 
Join Date: Jan 2017
Posts: 750
Gwarl is on a distinguished road
Quote:
Originally Posted by Marco View Post
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.
Gwarl is offline   Reply With Quote
Old November 5, 2019, 21:11   #5
Marco
Rookie
 
Marco's Avatar
 
Join Date: Oct 2019
Location: Italy
Age: 58
Posts: 7
Marco is on a distinguished road
Quote:
Originally Posted by Gwarl View Post
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 is offline   Reply With Quote
Old November 6, 2019, 09:18   #6
Marco
Rookie
 
Marco's Avatar
 
Join Date: Oct 2019
Location: Italy
Age: 58
Posts: 7
Marco is on a distinguished road
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.
__________________
/*
* Don't let the perfect be the enemy of the good
*/
Marco is offline   Reply With Quote
Old November 6, 2019, 11:54   #7
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 54
Posts: 7,849
Donated: $60
Nick will become famous soon enough
Quote:
Originally Posted by Marco View Post
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.
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is online now   Reply With Quote
Old November 6, 2019, 13:03   #8
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 5,391
Donated: $40
Pete Mack is on a distinguished road
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
....
Pete Mack is offline   Reply With Quote
Old November 7, 2019, 02:02   #9
Marco
Rookie
 
Marco's Avatar
 
Join Date: Oct 2019
Location: Italy
Age: 58
Posts: 7
Marco is on a distinguished road
Quote:
Originally Posted by Nick View Post
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.

Quote:
Originally Posted by Nick View Post
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.
__________________
/*
* Don't let the perfect be the enemy of the good
*/
Marco is offline   Reply With Quote
Old November 7, 2019, 02:10   #10
Marco
Rookie
 
Marco's Avatar
 
Join Date: Oct 2019
Location: Italy
Age: 58
Posts: 7
Marco is on a distinguished road
Quote:
Originally Posted by Pete Mack View Post
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.

Quote:
Originally Posted by Pete Mack View Post
$ 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.
__________________
/*
* Don't let the perfect be the enemy of the good
*/

Last edited by Marco; November 7, 2019 at 02:15.
Marco is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Just checking - starting a new game won't affect a saved game with another char? Petoften Vanilla 1 July 11, 2018 10:15
Commit c06af23: error in refactoring PowerWyrm Development 1 October 16, 2016 00:25
Commit 878cd0d: error in refactoring PowerWyrm Development 1 October 12, 2016 22:55
[3.5-dev] Monster opening/bashing refactoring PowerWyrm Vanilla 3 June 27, 2013 13:19
[3.5-dev] Store refactoring PowerWyrm Vanilla 5 May 27, 2013 14:25


All times are GMT +1. The time now is 01:43.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2019, vBulletin Solutions Inc.