Ladder Progress #325

thokkat · 2022-09-21T05:57:21Z

Climbing upstairs works now. Might require some adaptions once downstairs climbing is possible.

I tested only in the swamp/-camp. I guess in the Mines there could be some problems sometimes thanks to the steep edges.

#120

Try · 2022-09-21T19:21:20Z

Hi, @thokkat and thanks for PR!

I have had a quick look at the code, one thing that is suspicious:

- void Interactive::nextState(Npc& npc)

Why this function is no longer needed? What about other interactive objects?

PS: will look more closely at the end of the week

thokkat · 2022-09-22T00:31:52Z

It was only used in the processLadder function.

Try · 2022-09-25T20:50:33Z

Testing right now(Jarkentar only):
can't climb down - ladder is not interactive. Can be unrelated issue
when holding UpKey, upon climbing, there is a point when key-repeat stops working, at begin at climb

Try · 2022-09-25T21:55:39Z

game/world/objects/interactive.cpp

+      pl.setPosition(pos);
+      return;
+      }
+    if (state==0 || state==stateNum)


when holding UpKey, upon climbing, there is a point when key-repeat stops working, at begin at climb

Should be state==-1 ?

Actually this doesn't happen for me. Can it be keyboard related? Anyway you are right, changing this to -1, why else should it be excluded in the first if statement.

Try · 2022-09-25T22:02:15Z

game/world/objects/interactive.cpp

+      float rot = pl.rotationRad();
+      float s   = std::sin(rot), c = std::cos(rot);
+      Tempest::Vec3 dp(s,0.0f,-c);
+      pos += dp*6.f*float(dt);


6.f is arbitrary, right? While it's achieving the purpose, I have droughts about correctness.
On previous iteration, (back at nextState days), I found that position of npc changes after detaching from ladder. We should probably exam closely animations here.

Currently it's identical to marvin k. I tested some ladders in the G1 swamp camp and one only worked with 6f.

Hmm i have no other idea than "cheating" the character upwards. I only remember that devs said ladders were a nightmare so they left them out of G2 :D. My test ladder in vanilla G1 funnily didn't work too and dropped the character back down at the top.

I look into some other ladders in vanilla to see whats happening there

Try · 2022-09-25T22:03:49Z

game/game/playercontrol.cpp

-  if(key==KeyCodec::Forward) {
-    inter.nextState(pl);
+  if(key==KeyCodec::Forward && !isPressed(KeyCodec::Back)) {
+    inter.nextLadderMove(false);


It's probably better to stick with nextState notation, as regular MOBSI are also allowed to have multiple states.
Not in use in vanilla, but can be in mods.

Try · 2022-09-25T22:07:30Z

game/game/playercontrol.cpp

-  if(key==KeyCodec::Back) {
-    pl.setInteraction(nullptr);
+  if(key==KeyCodec::Back && !isPressed(KeyCodec::Forward)) {
+    inter.nextLadderMove(false,true);


false, true - is not good from readability point of view.
It's probably better to handle key-repeat in implMoveMobsi routine.

I kinda had that feeling but couldn't come up with something better.

thokkat · 2022-09-26T00:19:09Z

Yes non-interactive ladder upstairs was already the case. After some tries there's sometimes an interactive object found but not at the correct place, The climbing animation is played but the character doesn't move.

thokkat · 2022-09-29T18:59:26Z

When you climb up, reach the end of the ladder and press the action key after the climbing animation ends, you are now again attached to the ladder but a bit shifted. For most ladders i tested that point looks like the vanilla endpoint after climbing is finished, but could also be coincidental.

thokkat · 2022-09-30T14:57:31Z

I tried the original nextState like this

const int prev = state;
reverseState=reverse;
if(!setAnim(&npc,Anim::In))
  return;
if (reverse)
  setState(std::max(0,state-1));
else
  setState(std::min(stateNum,state+1));
loopState = (prev==state);

but it shows micro stuttering because the animation for the next state is only started after a key press triggers it. Stuttering was also happening before but less noticeable. Also removed the marvin stuff. Like you said it's artificial. Got a bit tricked by memory but vanilla shows it's a smooth process.

Try · 2022-10-03T21:58:56Z

Hi, @thokkat !
Sorry, for late response, bit to much code review in flight :)
for debugging, you can use F9/F10, to have frame-by-frame debugging:
F9 - pause
F10 - next frame

Setting MAX_FPS to low values in gothic.ini file can also be useful.

thokkat · 2022-10-13T18:34:48Z

@Try, everything works so far, i think you can review. Vanilla G2 crashes for me when entering Jharkendar so i don't know if G2 behavior is replicated correctly.

Still to do but i don't know how:

Stuttering issue. At the end of an animation the character is lifted for like 2 frames and then moved back. This results in mini stuttering, less noticeable if fps are high and causes a bit of camera shaking sometimes after climbing upstairs is finished.
AI

Try

Look very good in game already - left some comments, need to cleanup code a little

game/world/objects/interactive.cpp

game/graphics/mdlvisual.cpp

Try · 2022-10-15T23:42:12Z

game/game/movealgo.cpp

+  auto pos = npc.position();
+  if(npc.interactive()->isLadder()) {
+    auto rotY=-npc.rotationY();
+    if (std::abs(rotY)>0.001 && std::abs(dp.y)>0.001) {


What this code do? I've tested without it - no notifiable difference.

For tilted ladders. Character fails to properly climb up at top. And if the ladder is attached to a wall, the character climbs into the wall if climbing down. There is one close to the south gate in old camp.

Let's not do it for now - in test Jarkendar ladder works most of the time. Top-most ladder sometimes works poorly, but it same as in vanilla.

They shouldn't be affected by it. There are only a few in G1.

This is quite awkward code still :(
Why std::abs? There is seemingly no divisions downstream or something. And cos/sin should be way faster than the if

AFAIR this is G1 only, right? Can we decouple this change into another PR. This *=-2 fell like fine tuning; Vec3(s,0,-c) - in other places it's usually Vec3(c,0,s)

I only want to test if both values are nonzero, is there a better way? If rotY=0 only a zero vector is added, so this check could be left out. std::abs(dp.y)>0.001 is needed to know if the character is moving and don't add anything if not.

Vec3(s,0,-c) stuff is copied from marvinF8/K routines. Factor 2 is needed to make downstairs climbing look nice and yeah tilted ladders occur only in G1.

game/world/objects/npc.cpp

game/world/spaceindex.cpp

game/world/objects/npc.cpp

Try · 2022-10-15T23:48:48Z

game/world/objects/interactive.h

@@ -151,6 +151,7 @@ class Interactive : public Vob {
    int                          stepsCount = 0;

    int32_t                      state         = -1;
+    bool                         wait          = false;


Can it be moved to PlayerControl ? Having this for every interactive object is unnecessary

I could use a variable that already exists like isLockCracked? It even does the same just for chests and it should be safe since interactive can't be chest and ladder at the same time.

Idea is to have cleaner code by keeping player interaction stuff in PlayerControl and common stuff (npc and player) in Interactive.

I just don't see how. For chest

if(needToLockpick(npc)) { ... return; // chest is locked - need to crack lock first }

is checked to prevent playing the chest opening animation too early. The same is needed for ladder to check if player has up or down key pressed.

Edit: Ok going back to original "nextState" may work but then i basically have to copy half of the implTick routine and tailor it to ladder needs.

Just a question about key repeat. For action key, onKeyPressed is called on press and onKeyReleased on release. But for arrow keys these function calls are spammed when holding the key. Is this intended?

Is this intended?

No, it's not. There is keyPress - ketRepeat - keyRelease workflow, should work same for any key-type.

thokkat · 2022-10-16T21:25:11Z

Some footage so you know what i mean with tilted ladders.

ladderup.mp4

ladderdown.mp4

ladderFixed.mp4

thokkat · 2022-10-19T16:34:00Z

I made the aforementioned changes. Tell me if there*s more to do.

thokkat · 2022-11-07T17:12:54Z

@Try, reminder for review, now that phoenix has been merged ;) . G2 works as expected. In G1 there's now a general bug so animations aren't played.

Try

movealgo trigonometry is still suspicious. Other than that look good.

game/game/playercontrol.cpp

Try · 2022-11-07T19:17:07Z

game/game/playercontrol.cpp

@@ -196,12 +198,13 @@ void PlayerControl::tickFocus() {
  currentFocus = findFocus(&currentFocus);
  setTarget(currentFocus.npc);

-  if(!ctrl[Action::ActionGeneric])
+  if(!ctrl[Action::ActionGeneric] || Gothic::inst().world()->player()->interactive()!=nullptr)


nullptr checks - there is no guaranies , that player always will be around

What is the purpose if this change?

Again for G1 controls. Some ladders showed buggy behavior when interacting at top. This change prevents interaction, if player already interacts.

Should this kind of check then be placed into Focus World::findFocus(const Npc &pl, const Focus& def)?
if player already interacts with something, then search can simply early-out.

Try · 2022-11-07T19:20:39Z

game/game/movealgo.cpp

+  auto pos = npc.position();
+  if(npc.interactive()->isLadder()) {
+    auto rotY=-npc.rotationY();
+    if (std::abs(rotY)>0.001 && std::abs(dp.y)>0.001) {


This is quite awkward code still :(
Why std::abs? There is seemingly no divisions downstream or something. And cos/sin should be way faster than the if

AFAIR this is G1 only, right? Can we decouple this change into another PR. This *=-2 fell like fine tuning; Vec3(s,0,-c) - in other places it's usually Vec3(c,0,s)

Try · 2022-11-07T19:45:29Z

game/ui/inventorymenu.cpp

@@ -105,7 +105,7 @@ void InventoryMenu::close() {
  }

 void InventoryMenu::open(Npc &pl) {
-  if(pl.isDown() || pl.isMonster() || pl.isInAir() || pl.isSlide() || (pl.interactive()!=nullptr))
+  if(pl.isDown() || pl.isMonster() || pl.isInAir() || pl.isSlide() || pl.interactive()!=nullptr)


nitpick: code-style only change - not related to PR

So this should be a separate PR?

No - I'll fix it as separated commit, as followup

thokkat · 2022-11-09T20:12:34Z

@Try, I cleaned up the abs stuff. Vec3(c,0,s) moves the character leftwards and i only found it in camera.cpp. Vec3(s,0,-c) is copied from marvinF8/K routines,

Try · 2022-11-09T23:16:28Z

Merged, thanks!

Try reviewed Sep 25, 2022

View reviewed changes

thokkat force-pushed the ladder branch 2 times, most recently from f87dea7 to 09b9000 Compare October 9, 2022 22:50

thokkat force-pushed the ladder branch 2 times, most recently from 9bcf975 to f99ed9d Compare October 13, 2022 18:14

Try reviewed Oct 15, 2022

View reviewed changes

thokkat force-pushed the ladder branch from 07173c8 to fe435b7 Compare October 19, 2022 16:24

thokkat added 2 commits November 6, 2022 20:16

ladder

cfcb435

refactor

03606c6

thokkat force-pushed the ladder branch from fe435b7 to 03606c6 Compare November 6, 2022 21:02

Try reviewed Nov 7, 2022

View reviewed changes

ladder2

8920384

Try merged commit 46dcdc7 into Try:master Nov 9, 2022

thokkat deleted the ladder branch November 12, 2022 17:01

thokkat mentioned this pull request Mar 21, 2023

Ladder for pathfinding #425

Closed

Ladder Progress #325

Ladder Progress #325

Conversation

thokkat commented Sep 21, 2022 • edited Loading

Try commented Sep 21, 2022

thokkat commented Sep 22, 2022 • edited Loading

Try commented Sep 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thokkat Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

thokkat commented Sep 26, 2022

thokkat commented Sep 29, 2022

thokkat commented Sep 30, 2022

Try commented Oct 3, 2022

thokkat commented Oct 13, 2022 • edited Loading

Try left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thokkat Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thokkat Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thokkat Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thokkat commented Oct 16, 2022

thokkat commented Oct 19, 2022

thokkat commented Nov 7, 2022

Try left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thokkat commented Nov 9, 2022

Try commented Nov 9, 2022

thokkat commented Sep 21, 2022 •

edited

Loading

thokkat commented Sep 22, 2022 •

edited

Loading

thokkat Sep 25, 2022 •

edited

Loading

thokkat commented Oct 13, 2022 •

edited

Loading

thokkat Oct 16, 2022 •

edited

Loading

thokkat Oct 16, 2022 •

edited

Loading

thokkat Oct 16, 2022 •

edited

Loading