Try / OpenGothic

Reimplementation of Gothic 2 Notr
MIT License
1.17k stars 85 forks source link

Fix Gothic 1 melee blocking #630

Closed thokkat closed 5 months ago

thokkat commented 6 months ago

Problem was a character confusion. For G2 the number 0 is used in T_1HPARADE_0 but in G1 the letter O is used in T_1HPARADE_O.

What I'm unsure about is the FIXME: seems like name check is not needed part. Testing shows this is necessary because the DefWindow can be reached during attack animations. But DefWindow refers to a combo window and DefParWindow should be the correct one to choose. But then again this one is always empty.

For now I checked against DefParWindow and consider empty window as blocking always enabled during animation.

Fixes https://github.com/Try/OpenGothic/issues/557

Try commented 5 months ago

After testing this PR:

It looks to me that in opengothic block is handled conceptually wrong.

My suggesting is to:

  1. BS_PARADE test instead of name test + isDefParWindow (old implementation) for normal block
  2. BS_PARADE + lack of defParFrame for jump-back block

case 2 will help with Gothic1 and also makes sense in G2. Not sure about animals in G1

thokkat commented 5 months ago
  1. BS_PARADE test instead of name test + isDefParWindow (old implementation) for normal block
  2. BS_PARADE + lack of defParFrame for jump-back block

Do you mean DefWindow? Asking because defParFrame is always empty and thus Animation::Sequence::isDefParWindow returns always false.

Vanilla is using t_1hParadeJumpB instead of t_1hJumpB for jump block. Animation is the same but t_1hParadeJumpB comes with a DEF_WINDOW so suggested check for jump wouldn't work then which means vanilla must have some different jump detection. Maybe go back to name testing for jump only and check BS_PARADE for isDefence?

Try commented 5 months ago

Do you mean DefWindow?

Yes def-window, sorry. Maybe it's easier to post the code:

bool Pose::isDefence(uint64_t tickCount) const {
  for(auto& i:lay) {
    if(i.bs==BS_PARADE && i.seq->isDefWindow(tickCount-i.sAnim))
      return true;
  return false;
  }

Asking because defParFrame is always empty

This will fall-thru to jump-back case. Here My assumption that vanilla script is slightly buggy, and engine should interpret such cases as jump-back (aka unconditional block)

bool Pose::isJumpBack() const {
  for(auto& i:lay) {
    if(i.bs==BS_PARADE && i.seq->data->defParFrame.empty())
      return true;
    }
  return false;
  }

Reason, why I'm thinking that this can be correct way: no name checks + simplicity of code. Only issue here, is propagating BS in Pose::processLayers: "T_2HJUMPB" has stand animation as next, resulting into infinite stand-block. Probably it's a bug with BS propagation here.

thokkat commented 5 months ago

BS_PARADE is now set if npc is in jump back animation and in melee fight mode. Technically this is wrong because in G1 for humans jump back is always BS_RUN and non blocking. For G1 it looks like only monsters block in jump back but haven't tested much more. I'd leave that for later.

I let animation names as is because it already works and avoids the bug you mentioned. One more reason is waran in G1 uses both JUMPB and PARADEJUMPB which makes it unclear when to pick which.

Try commented 5 months ago

I've added

        if(lay[i].bs==BS_PARADE)
          lay[i].bs = BS_RUN;

to animation solver, to avoid issues with T_2HJUMPB, rest looks good

Try commented 5 months ago

Merged, thanks!