Details
- Reviewers
bb temple - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP21337: Learn males chopped wood is not a weapon to fight with.
- Trac Tickets
- #4932
If you have replays, run them. Otherwise play the game.
I think this should be committed asap regardless of actually testing that it fixes it, unless we have enough replays to verify.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Besides the gather thing, patch looks correct and complete, leaving the heal/trade/repair as is for now is fine with me (maybe make a trac ticket of it so we won't need to fix the same bug twice)
minimal replay showing the bug:
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
|---|---|---|
| 2106 ↗ | (On Diff #5396) | don't get this one, when approaching some gather target we should show the resources right? |
| 2225 ↗ | (On Diff #5396) | (so this one can be removed) |
| 2486 ↗ | (On Diff #5396) | We do not have healers that can gather, but in principle the same can happen here, the way to fix that is having a seperate "healer" case in the variant select function, but that might be meh for now |
| 2674 ↗ | (On Diff #5396) | kinda same reasoning as for the healer |
| 2718 ↗ | (On Diff #5396) | Also same as healer |
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
|---|---|---|
| 2106 ↗ | (On Diff #5396) | Hm, yeah, seems I messed up some copy paste here. |
| 2225 ↗ | (On Diff #5396) | I'd rather not actually, in case we ever change the default behaviour to be sufficiently different gfrom the gathering behaviour. TBH ideally we'd reset on all states as you said. |
| 2486 ↗ | (On Diff #5396) | Yeah didn't bother assuming they used the default variant, but it could be changed too. |
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
|---|---|---|
| 2225 ↗ | (On Diff #5396) | Maybe you are right, but found this bug while testing Now the walk animation does not render properly Couldn't reproduce with female |
Planning changes until we're sure this is fixing all bugs.
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
|---|---|---|
| 2225 ↗ | (On Diff #5396) | Mh, that's odd. I'll try to reproduce and amend this revision if necessary. |
The sliding issue got fixed in rP21219, tested with removing L2106, everything looks ok, so acceptance stands
Can reproduce bb's issue, but with or without the patch. It shows the idle animation for a bit before then using the walking animation. I haven't figured out why yet, but I don't think it's related to the patch. I'm okay with it after removing the gather part.
The issue is that if the animation names are the same (which they are here, both walk), then it doesn't update the animation id right away (in SetAnimationState of UnitAnimation.cpp). E.g. carry_food doesn't have an id for walk but base_hoplite has walk1 and walk2. So then switching from walking & fighting (which uses hoplite) to walking (which uses carry) tries to find the old walk1 or walk2 id which doesn't exist, so it uses the idle animation by default, hence the gliding for a second or two before the animation id's finally updated. However, if there's no id then it takes all possible animations (in GetAnimations of ObjectEntry.cpp) so going from walking (using carry) to attacking (using hoplite) is okay. Women's walk doesn't have an id so there's no problem switching between walking & fighting and walking.
So we probably need some way to update the animation id earlier.