fix: remove the concept of first step#3642
Conversation
Auto walk should follow thinking constraints, including thinking time and ticks. Currently, the first step of an auto walk action was treated differently. It ignored all constraints and were immediately executed. While this create a good feeling of quick reactions, it felt unnatural that creatures were instantly reacting to changes. Also, there was a bug where, for small paths (e.g. when following someone) every step taken was considered a first step. To emulate more realistically the reaction time, and global behaviors, this commit removes the concept of first step, treating every step equally and respecting the thinking constraints of creatures behaviors
| if (ticks == 1) { | ||
| onCreatureWalk(); | ||
| safeCall([this] { | ||
| const int64_t ticks = getEventStepTicks(); |
There was a problem hiding this comment.
Moving this inside just to make sure that ticks are calculated once the actual action is called.
Calculating it before safeCall would expose it to race conditions or delays in the scheduling.
E.g.: ticks before dispatching are 1500ms, the server is busy and the function took ~300ms to be executed, the step will be schedule with the original 1500ms and will end up being 300ms delayed.
While it is virtually the same - we don't expect huge delays on dispatching, and that the practical impacts are negligible, I believe that this is safer.
| safeCall([this, ticks]() { | ||
| // Take first step right away, but still queue the next | ||
| if (ticks == 1) { | ||
| onCreatureWalk(); |
There was a problem hiding this comment.
Removing this will lead to slightly less responsive auto walking start.
We can re-think some quicker responsiveness for long walks in the future, without impacting short paths (e.g. a creature close following another).
|
dudantas
changed the title
|
At certain moments, I noticed that the player "stutters" while walking. It happened a few times, especially when the player is high-level. |
Thanks a lot for testing it! Could you attach a short screen recording? Also, which level/speed and whether you were following closely vs. long path would help. |
At the beginning of the video, the EK is without haste spell (1621), and the second time he is using utani tempo hur (2940). vocat EK: 1397 0904.mp4In the video, you can clearly see it kind of "lags" the movement and stutters before moving to the next tile. |
|
In my opinion, this isn't random. I noticed this effect when entering houses. I therefore think it triggers when you enter a PZ. I'm not sure why |
When we enter a PZ, a bunch of things get triggered by the server, and even the minimap blinks. |
|
This PR is stale because it has been open 45 days with no activity. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMethod signatures simplified: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
This PR is stale because it has been open 45 days with no activity. |
|
@gabrielew @roys82 thanks again for the reports and the video/details. The branch has been updated with the latest Could you please test the latest PR head when you have a chance? The most useful checks would be:
Any confirmation, video, or reproduction notes would help us decide whether this is ready. |
There was a problem hiding this comment.
Pull request overview
This PR removes the “special first step” behavior from creature auto-walk scheduling so that movement steps obey the normal walk/thinking timing constraints, preventing instant AI reactions and fixing repeated “first step” behavior on short/recomputed paths.
Changes:
- Introduces
Creature::WalkStartPolicyto explicitly control first-step scheduling behavior (RespectDelayvsImmediateWhenReady). - Updates
Creature::startAutoWalk,addEventWalk, andgetEventStepTicksto use the new policy and compute timing at scheduling time (insidesafeCall). - Updates player movement entry points to pass an explicit start policy (preserving responsiveness for certain player inputs).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/game/game.cpp | Passes an explicit WalkStartPolicy when starting player autowalk/move actions. |
| src/creatures/creature.hpp | Adds WalkStartPolicy and updates walk scheduling APIs/docs to remove the legacy “first step” flag. |
| src/creatures/creature.cpp | Implements policy-based scheduling and moves tick calculation inside safeCall to avoid stale timing. |
|
Auto walk should follow thinking constraints, including thinking time and ticks.
Currently, the first step of an auto walk action was treated differently. It ignored all constraints and were immediately executed.
While this create a good feeling of quick reactions, it felt unnatural that creatures were instantly reacting to changes.
Also, there was a bug where, for small paths (e.g. when following someone) every step taken was considered a first step.
To emulate more realistically the reaction time, and global behaviors, this commit removes the concept of first step, treating every step equally and respecting the thinking constraints of creatures behaviors.
Behaviour
Actual
Every auto walk starts instantly and do not respect turns.
Expected
Auto walk respects turns.
Type of change
Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested
Summon 3 creatures and lure them, the reaction should differ depending on when you execute your walk. The creature will take longer or shorter to follow you, depending on where in their thinking cycle the change happened.
Before the changes this would always happen immediately, no room for thinking to happen.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.