[Need Help] increase the player stat index

[illwieckz]

Required by https://github.com/Unvanquished/Unvanquished/pull/1182.

I need help to increase MAX_STATS as seen here:

https://github.com/DaemonEngine/Daemon/blob/393e1829d5acffc0f0fde14c87fd8dd9e7141445/src/engine/qcommon/q_shared.h#L1764-L1766

To add one field there:

https://github.com/Unvanquished/Unvanquished/blob/f79ea6e9b10f6d0d0c32b59f9ed371e099c52b9a/src/shared/bg_public.h#L224-L244

@Viech it looks like you added a comment 7 years ago about a very similar array to increase the same way:

https://github.com/Unvanquished/Unvanquished/blame/f79ea6e9b10f6d0d0c32b59f9ed371e099c52b9a/src/shared/bg_public.h#L327

Is it something you know how to do?

[Viech]

Nope I don't know how to extend this, I just updated the comment to reflect the new number of free slots. It would probably increase (break) the network protocol version to add more. The rule of thumb was that you optimize your use of these fields as much as you can. Four fields for build point budgets is already a lot and maybe you can still get away with four with what you are planning, otherwise just add a fifth while there are still two fields left.

[Viech]

I just realized you mean STAT, not PERS. If you just need one or two bits have a look at STAT_STATE and STAT_STATE2 first. If these are full then you can consider using two bits off STAT_TAGSCORE and leave six for whatever it's doing now. I'd also look at STAT_ACTIVEITEMS to see if it's still doing something now that you don't scroll through an inventory any more.

[illwieckz]

I just realized you mean STAT, not PERS.

Yes, I was just hoping someone would have skills to do what the comment says about PERS, because the solution for it could be copy-pasted for STAT.

If these are full then you can consider using two bits off STAT_TAGSCORE and leave six for whatever it's doing now. I'd also look at STAT_ACTIVEITEMS to see if it's still doing something now that you don't scroll through an inventory any more.

Unfortunately I need a full int… not just some bits.

That is the issue: initially the "how much you have charged your lucifer cannon” (= weapon charge) was stored as an int named STAT_MISC. Other charging or discharging weapons used this int, like dragoon pounce (charge), tyrant trample (charge and discharge), mantis dash discharge). But also, the discharging time you had to wait after having constructed or deconstructed a buildable was stored in that STAT_MISC int. The current issue is that with the “hold right mouse click to deconstruct buildable feature” needs to use the weapon charge mechanism, while we would still need a discharging time for the construction/deconstruction. It means we need two ints to store the two various charging time

Note that at that point it's not yet done but it's the next step, is that when you deconstruct using "hold mouse button", the discharging time for deconstruction would be substracted by the time you already spent holding the button.

however if you just want to remove a building if you hold down a key for long enough, maybe that's something that can be done on client side just as for E vs. shift+E.

It does not look a good idea to make it entirely client side, that would be an open door to exploits by sending the signal to deconstruct the buildable immediately and save deconstruction time.

When I will have implemented the substraction, the implementation would take the same time to deconstruct buildable whatever it is done by holding a key or by sending deconstruct marked command.

[Viech]

What if the buildable entity has the countdown? Then you wouldn't have a deconstruction cooldown but you'd need to spend some time in front of the buildable.

[illwieckz]

How would you reset the countdown by moving out of range of the given buildable?

[illwieckz]

Hmmm, you made me realize that with my implementation I can start the countdown on a buildable and finish it on another one if I manage to jump from one to another without interruption. I managed to do it in game, by building a drill in front of an armoury, crounching near them and starting the countdown on the drill and finishing it on the armoury, the armoury get deconstructed.

[illwieckz]

Anyway, using the weapon thing is nice as I can leverage all the existing code: weapon load hud, etc.

[illwieckz]

Ping @Kangz, @Amanieu, @DolceTriade or @slipher. I need help from either an engine wizard either a C/C++ wizard… or both. :sunglasses:

I need to add at least one field to an array which has its size limited for some reason to 16, it looks like there is a relation between the array indexes and some bits in some bitfields somewhere else, so writing 17 would not work. I would like to see the feature requiring this being released soon™ to greatly increase the accessibility of the game, but that issue is blocking. :confused:

[Amanieu]

This will require making a breaking change to the network protocol. See daemon/src/engine/qcommon/msg.c MSG_WriteDeltaPlayerstate and MSG_ReadDeltaPlayerstate. You'll need to change MSG_{Read|Write}Short to MSG_{Read|Write}Bits when reading/writing statsbits.

[Amanieu]

You'll also want to bump PROTOCOL_VERSION in qcommon.h.

[illwieckz]

Interesting:

https://github.com/DaemonEngine/Daemon/blob/393e1829d5acffc0f0fde14c87fd8dd9e7141445/src/engine/qcommon/msg.cpp#L1782-L1784

It looks like at some point in the past the array was larger.

You'll also want to bump PROTOCOL_VERSION in qcommon.h.

I really don't like that. :confused:

Edit: the reason I don't like that is that it may break third party programs like server browsers. I don't mind breaking our protocol since we have a small user base at this point and it would not be hard to get all servers updated.

[illwieckz]

That old comment says:

back to short since weapon bits are handled elsewhere now

And I all I want is to add weapon data… but I don't see where those weapon bits would be…

[Amanieu]

I think it's referring to these in playerState_t in q_shared.h:

        // weapon info
        int weapon; // copied to entityState_t->weapon
        int weaponstate;

[Viech]

You should try the buildable entity route first before you break protocol for a minor gameplay improvment. We didn't do such a network field increase before because we've been looking for efficient use of them all the time. Storing the count on a buildable also means that two players can deconstruct together. It also makes more sense that this is something you do with the buildable as opposed to something you do to yourself.

[illwieckz]

You should try the buildable entity route first before you break protocol for a minor gameplay improvment.

While I agree that if there is an non-intrusive alternative way to implement it it may be interesting too look at it, that's clearly not a “minor gameplay improvement”, it's the fix to the biggest struggle newcomers face when discovering the game, a flaw that encourages them to teamkill their own buildables and get teamkilled by retaliation by other peoples (if not kicked by server owner), if there was kick vote newplayers would just be kicked out by the crowd. A fix for this accessibility issue is required whatever the cost.

[illwieckz]

Note: I know that's the biggest struggle by:

• observing newcomers on servers and talking to them when they face trouble;
• making random people around me to test the game (basically childs of friends);
• testing the game myself with empty profile.

I'll deeply recommend anyone of us to test the game with empty profiles. My workflow while writing https://github.com/Unvanquished/Unvanquished/pull/1182 is to reset the configuration anytime I start the game.

At this point we must consider our game does not provide a way to deconstruct buildables because there is no way to know how to deconstruct buildables in our game, and without having played Tremulous 15 years ago and without knowing how to tweak keybindings from command line or config file, there is no way for the user to deconstruct buildables.

[illwieckz]

Storing the count on a buildable also means that two players can deconstruct together.

@Viech I don't know what you mean there. Players can already deconstruct together. You mean deconstructing faster the same buildable by teaming up?

[illwieckz]

Another alternative would be to not store the player build timer in those player stat. I'm not looking for a new field to store the ckit charge, I store it in the same field other weapon store their charge. I'm looking for storing the player own build timer elsewhere than player stat.

[illwieckz]

All code reads STAT_BUILDTIME (i.e. STAT_MISC when used as build timer) from client->ps.stats (client being gclient_t type) but cgame/cg_rocket_progressbar.cpp that reads it from cg.snap->ps.stats (cg being cg_t type).

Is there any way to get access to client from cgame/cg_rocket_progressbar.cpp so I can delete STAT_BUILDTIME from playerstats?

Because basically my issue is that the game currently hijacks the field for weapon charge storage to store something else instead. And I need to store weapon charge.

[illwieckz]

I think it's referring to these in playerState_t in q_shared.h:

      // weapon info
      int weapon; // copied to entityState_t->weapon
      int weaponstate;

@Amanieu do you think it's possible to store weapon charge as an int in playerState_t the same way? I tried but that does not work correctly: some data seems to not be sync between client and server, so I would know if it's theoretically feasible and I'm just doing it badly, or if it's not feasible at and I'm wasting my time.

[Amanieu]

According to the netcode that field is only 8 bits (or rather only 8 bits are synced).

static netField_t entityStateFields[] =
{
    { NETF( eType ),             8              , 0 },
    { NETF( eFlags ),            24             , 0 },
    { NETF( pos.trType ),        8              , 0 },
    { NETF( pos.trTime ),        32             , 0 },
    { NETF( pos.trDuration ),    32             , 0 },
    { NETF( pos.trBase[ 0 ] ),   0              , 0 },
    { NETF( pos.trBase[ 1 ] ),   0              , 0 },
    { NETF( pos.trBase[ 2 ] ),   0              , 0 },
    { NETF( pos.trDelta[ 0 ] ),  0              , 0 },
    { NETF( pos.trDelta[ 1 ] ),  0              , 0 },
    { NETF( pos.trDelta[ 2 ] ),  0              , 0 },
    { NETF( apos.trType ),       8              , 0 },
    { NETF( apos.trTime ),       32             , 0 },
    { NETF( apos.trDuration ),   32             , 0 },
    { NETF( apos.trBase[ 0 ] ),  0              , 0 },
    { NETF( apos.trBase[ 1 ] ),  0              , 0 },
    { NETF( apos.trBase[ 2 ] ),  0              , 0 },
    { NETF( apos.trDelta[ 0 ] ), 0              , 0 },
    { NETF( apos.trDelta[ 1 ] ), 0              , 0 },
    { NETF( apos.trDelta[ 2 ] ), 0              , 0 },
    { NETF( time ),              32             , 0 },
    { NETF( time2 ),             32             , 0 },
    { NETF( origin[ 0 ] ),       0              , 0 },
    { NETF( origin[ 1 ] ),       0              , 0 },
    { NETF( origin[ 2 ] ),       0              , 0 },
    { NETF( origin2[ 0 ] ),      0              , 0 },
    { NETF( origin2[ 1 ] ),      0              , 0 },
    { NETF( origin2[ 2 ] ),      0              , 0 },
    { NETF( angles[ 0 ] ),       0              , 0 },
    { NETF( angles[ 1 ] ),       0              , 0 },
    { NETF( angles[ 2 ] ),       0              , 0 },
    { NETF( angles2[ 0 ] ),      0              , 0 },
    { NETF( angles2[ 1 ] ),      0              , 0 },
    { NETF( angles2[ 2 ] ),      0              , 0 },
    { NETF( otherEntityNum ),    GENTITYNUM_BITS, 0 },
    { NETF( otherEntityNum2 ),   GENTITYNUM_BITS, 0 },
    { NETF( groundEntityNum ),   GENTITYNUM_BITS, 0 },
    { NETF( loopSound ),         8              , 0 },
    { NETF( constantLight ),     32             , 0 },
    { NETF( modelindex ),        MODELINDEX_BITS, 0 },
    { NETF( modelindex2 ),       MODELINDEX_BITS, 0 },
    { NETF( frame ),             16             , 0 },
    { NETF( clientNum ),         8              , 0 },
    { NETF( solid ),             24             , 0 },
    { NETF( event ),             10             , 0 },
    { NETF( eventParm ),         8              , 0 },
    { NETF( eventSequence ),     8              , 0 },  // warning: need to modify cg_event.c at "// check the sequencial list" if you change this
    { NETF( events[ 0 ] ),       8              , 0 },
    { NETF( events[ 1 ] ),       8              , 0 },
    { NETF( events[ 2 ] ),       8              , 0 },
    { NETF( events[ 3 ] ),       8              , 0 },
    { NETF( eventParms[ 0 ] ),   8              , 0 },
    { NETF( eventParms[ 1 ] ),   8              , 0 },
    { NETF( eventParms[ 2 ] ),   8              , 0 },
    { NETF( eventParms[ 3 ] ),   8              , 0 },
    { NETF( weapon ),            8              , 0 },
    { NETF( legsAnim ),          ANIM_BITS      , 0 },
    { NETF( torsoAnim ),         ANIM_BITS      , 0 },
    { NETF( generic1 ),          10             , 0 },
    { NETF( misc ),              MAX_MISC       , 0 },
    { NETF( weaponAnim ),        ANIM_BITS      , 0 },
};

(0 in the middle column means a float field which has a special encoding)

[slipher]

hold right mouse click to deconstruct buildable feature

Why does this need to be stored on the server at all? Unless delaying people from deconstructing something is required for gameplay reasons, it could be implemented purely client-side.

[illwieckz]

I mean doing that:

        // weapon info
        int weapon; // copied to entityState_t->weapon
        int weaponCharge;
        int weaponstate;

And playing the cargo cult dance praying for not missing anything.

[illwieckz]

hold right mouse click to deconstruct buildable feature

Why does this need to be stored on the server at all? Unless delaying people from deconstructing something is required for gameplay reasons, it could be implemented purely client-side.

I want to substract the time spent to hold the key to the usual deconstruction time set for gameplay reason. This way holding the key or doing "deconstruct marked" would last the same.

[slipher]

What usual deconstruction time? It seems like I can deconstruct things as fast as I want.

Oh wait, I was testing with devmap, better try without that...

[illwieckz]

According to the netcode that field is only 8 bits (or rather only 8 bits are synced).

I've seen that, that gave me a question to ask: https://github.com/DaemonEngine/Daemon/issues/337

[illwieckz]

Also, about:

Why does this need to be stored on the server at all?

The thing is that I just reuse common weapon charging mechanism (like luci charging, goon jump, etc?) with all the infrastructure around (up to the librocket hud), so I'm not doing this in purpose to make it server side, I just reuse available features.

[illwieckz]

What usual deconstruction time? It seems like I can deconstruct things as fast as I want.

Oh wait, I was testing with devmap, better try without that...

Yes, it's disabled with devmap, also I would like to know where it is disabled when devmap is used because if I’m right there is a cvar cheat to disable build timer so I would make deconstruction use this cheat only if enabled… To help debugging. :-)

[illwieckz]

I mean doing that:

      // weapon info
      int weapon; // copied to entityState_t->weapon
      int weaponCharge;
      int weaponstate;

And playing the cargo cult dance praying for not missing anything.

@Amanieu it looks like I got something working on my end. At least it works locally. My issue was I forgot to copy the value in the client prediction code.

[Gireen]

At this point we must consider our game does not provide a way to deconstruct buildables because there is no way to know how to deconstruct buildables in our game, and without having played Tremulous 15 years ago and without knowing how to tweak keybindings from command line or config file, there is no way for the user to deconstruct buildables.

Wouldn't it be then easier to have a circle menu open when right clicking on buildings with the options to mark or deconstruct? It would also give a place to explain what marking even means. And for advanced players there is the options to have separate binds for mark and decon.

[illwieckz]

I welcome anyone to implement and submit the buildable-side implementation, and, good news: more than 90% of the code would be reused straight: just replace the per-weapon storage with a per-buildable one, we still need the weapon interaction, and drop the user weapon hud thing. :wrench:

The same goes for the circle menu thing, but I think would be less smooth for the user because a menu may be too much for just two actions and it's probably too much costly to break movement for so few options. And we know it will suffer from the same issues we face with the build menu, like the problem with wallwalking. Our UI thing is not really ready for that.

The thing is that this implementation works and I have to put my hands on other tasks.

One can say: if the engine is not able to provide weapon charging mechanism and a per-player cool down mechanism at the same time, the engine does not fit our need and has to be improved. Maybe we can also say that part of the engine needs a redesign if it is so closely tight to the game code.

I would like to get comments on #338 by the way. :smiley:

[illwieckz]

Obsoleted by #341, @slipher did magic to make this not required.