Implementing the restructure changes

[Nick]

Thanks, done and done.

[PowerWyrm]

The draw_rectangle() method is badly written (and there's a nice typo), considering that generate_mark() already iterates on a rectangular area. It should be:

    for (y = y1; y <= y2; y++)
    {
        square_set_feat(c, y, x1, feat);
        square_set_feat(c, y, x2, feat);
    }
    if (flag)
    {
        generate_mark(c, y1, x1, y2, x1, flag);
        generate_mark(c, y1, x2, y2, x2, flag);
    }

    for (x = x1; x <= x2; x++)
    {
        square_set_feat(c, y1, x, feat);
        square_set_feat(c, y2, x, feat);
    }
    if (flag)
    {
        generate_mark(c, y1, x1, y1, x2, flag);
        generate_mark(c, y2, x1, y2, x2, flag);
    }

[PowerWyrm]

Currently implementing the removal of the FEAT_WALL constants in profit of SQUARE_WALL flags. Found a little error in mutate_cavern(): FEAT_WALL_SOLID was replaced by FEAT_GRANITE, but square_set_feat was not replaced by set_marked_granite, so the SQUARE_WALL_SOLID flag is never set. The code should probably be:

    for (y = 1; y < h - 1; y++)
    {
        for (x = 1; x < w - 1; x++)
        {
            if (temp[y * w + x] == FEAT_GRANITE)
                set_marked_granite(c, y, x, SQUARE_WALL_SOLID);
            else
                square_set_feat(c, y, x, temp[y * w + x]);
        }
    }

[Nick]

Before the commit that nuked tval_to_attr, the default attr for inventory display was put into a pref file (font-xxx.prf), which clearly could be changed by the player. All other info bits, especially breakage chance and object flags, are clearly game core info and should NOT be changed by the player.

For my variant's perspective, the font-xxx.prf info was parsed on the client and transmitted to the server, which then used that info to display items in inventory for each player. The object_base.txt info was parsed on the server and therefore was used for all players.

I consider this a bug to be fixed - presumably by restoring font-xxx.prf and a function equivalent to tval_to_attr.

[Carnivean]

No, because the txt files are tied to game core, not game preferences. You cannot let players choose core properties like object or monster flags, it would make no sense (player edits monster.txt, set Morgoth hp to 1...).

I should have been clearer, I meant just for the display properties that you were talking about.

[fizzix]

I think it highlights a failure of the core-ui split where display information is encoded in the edit files alongside gameplay information. I guess for now your solution is best, parse the player information and only pass over the display stuff.

I'm not sure if splitting everything up into two separate files is the way to go. Then you would need to make edits in two places anytime you wanted to add or remove an object or item, which is a bit of a pain.

[PowerWyrm]

I'm not 100% sure what you're talking about, but can't you have the server deal with the default, and then have the client side interpret the value from the server into whatever the player has configured? Or vice versa?

No, because the txt files are tied to game core, not game preferences. You cannot let players choose core properties like object or monster flags, it would make no sense (player edits monster.txt, set Morgoth hp to 1...).

[Carnivean]

- parse object_base.txt on the server and set the same default attr for inventory display for all players

I'm not 100% sure what you're talking about, but can't you have the server deal with the default, and then have the client side interpret the value from the server into whatever the player has configured? Or vice versa?

[PowerWyrm]

Nevermind. The txt files in /lib/edit are server-side files and should not contain any player configurable info. Therefore the only thing I can do is implement option #1 and set the same default attr for inventory display for all players.

[PowerWyrm]

Hmm there's something that's bothering me about object_base.txt, even more when I look from my variant's perspective.

Before the commit that nuked tval_to_attr, the default attr for inventory display was put into a pref file (font-xxx.prf), which clearly could be changed by the player. All other info bits, especially breakage chance and object flags, are clearly game core info and should NOT be changed by the player.

For my variant's perspective, the font-xxx.prf info was parsed on the client and transmitted to the server, which then used that info to display items in inventory for each player. The object_base.txt info was parsed on the server and therefore was used for all players.

At this point I have two unsatisfactory options:
- parse object_base.txt on the server and set the same default attr for inventory display for all players
- parse object_base.txt on both client and server and only transfer the attr to the server

For now I'll implement option #2.

[Nick]

Why is this parsing code even using an explicit state machine? It seems like it would be a lot simpler and easier to understand (and maintain!) if it were a simple recursive descent parser? (It should obviously (ideally) have tests either way, but IME it's also a lot easier to tell at a glance if a recursive descent parser is actually correct.)

molybdenum wrote it, it works, and I've had plenty of other things to do

[AnonymousHero]

So the plan is you can have an expression with actual dice (not just a base), and then still have an m_bonus? And in V where there are no &'s, the only change is the extra H in the G row, which has no effect because there are no expressions of that type (but presumably we could use those now).

I kind of suspect the whole thing was just cooked up by you and molybdenum to confuse me, but you have failed

Why is this parsing code even using an explicit state machine? It seems like it would be a lot simpler and easier to understand (and maintain!) if it were a simple recursive descent parser? (It should obviously (ideally) have tests either way, but IME it's also a lot easier to tell at a glance if a recursive descent parser is actually correct.)

[PowerWyrm]

A little omission when factoring out check_state() and adding timed flags display to character flag screen: TMD_BOLD doesn't trigger OF_RES_FEAR (little "!" next to pFear). Happens when quaffing !heroism for example (mushroom of clear mind, for example, correctly triggers "!" next to pConf).

Also TMD_TERROR doesn't trigger OF_AFRAID (but TMD_AFRAID does).

[PowerWyrm]

Another glitch in calc_bonuses (but this one was already there before the restructure):

old code

if (player_has(PF_BLESS_WEAPON) && !check_state(p_ptr, OF_BLESSED, p_ptr->state.flags) &&

new code

if (player_has(PF_BLESS_WEAPON) && !player_of_has(player, OF_BLESSED) &&

Since at this point we try to fill a "state" structure, it's logical to use this structure instead of the player->state one...

The code should then be:

if (player_has(PF_BLESS_WEAPON) && !of_has(state->flags, OF_BLESSED) &&

[PowerWyrm]

A little omission when factoring out check_state() and adding timed flags display to character flag screen: TMD_BOLD doesn't trigger OF_RES_FEAR (little "!" next to pFear). Happens when quaffing !heroism for example (mushroom of clear mind, for example, correctly triggers "!" next to pConf).