Opened 11 years ago
Closed 11 years ago
#1906 closed defect (invalid)
[PATCH] Units turn at most 90 degrees when attacking
Reported by: | sanderd17 | Owned by: | ben |
---|---|---|---|
Priority: | Release Blocker | Milestone: | |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description
As described on the forum here: http://www.wildfiregames.com/forum/index.php?showtopic=17225
Ranged units that run away from a melee unit turn around to attack it again. But the actor turns only 90 degrees instead of 180 degrees.
Attachments (1)
Change History (15)
comment:1 by , 11 years ago
Component: | Core engine → UI & Simulation |
---|
by , 11 years ago
Attachment: | 0001-Reinterpolate-when-rotation-is-not-done-after-one-tu.patch added |
---|
comment:3 by , 11 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 14 |
Summary: | Ranged units turning only 90 degrees when launching projectile → [PATCH] Units turn at most 90 degrees when attacking |
comment:4 by , 11 years ago
Priority: | Should Have → Release Blocker |
---|
comment:5 by , 11 years ago
The patch seems to fix the bug, I've tested numerous times and don't see archers pointed the wrong direction anymore. It's somewhat hacky because it's sending a position changed message when that's not strictly true.
However, the performance gain of all these changes is still relatively significant, especially on maps with lots of stationary entities (for example on my system, sim interpolation on Gambia River map with no AIs, map revealed, is about 9 ms/frame prior to r13223, but only 2.5-3 ms/frame with SVN trunk+this patch).
What I'll do is add a comment about the hack, so we can find it again easily if we decide on a different optimization approach later.
comment:7 by , 11 years ago
Keywords: | review removed |
---|
comment:8 by , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I just realized the patch sends a simulation message in response to unsynchronized float data, that's bad for two reasons:
- floats aren't supposed to affect the simulation state
- unsynchronized state will break multiplayer games
That's unsafe, we can't trust that other components receiving the PositionChanged
message won't change the simulation state when receiving it, and the OOS checks could fail. I'm actually surprised I didn't get a serialization test failure while testing the patch, maybe I didn't let it run long enough or the serialization test affects the timing so that case is less likely to be hit.
The other bug reported is that rotation will now "jump" because it waits until the start of a turn to force the interpolation with that hack, but the above issue is the new release blocker :(
follow-up: 11 comment:9 by , 11 years ago
Interesting comment I found in CCmpPosition.cpp:
// TODO: if this component turns out to be a performance issue, it should // be optimised by creating a new PositionStatic component that doesn't subscribe // to messages and doesn't store LastX/LastZ, and that should be used for all // entities that don't move
What if we had a CCmpPositionStatic
, and CCmpVisualActor
tested a new IsStatic()
method before interpolating? This static position would be used for trees, mines, decorative objects, buildings, etc. (construction previews would still need interpolation, maybe we could make them use the dynamic position component) The interface would be the same, they'd both be instances of ICmpPosition
.
There would still be some unnecessary interpolation in CmpVisualActor
, e.g. stationary units, but we'd also have no interpolation in all the entities using CmpPositionStatic
, and the nice thing is we save ourselves from all these bugs.
Any thoughts on this?
comment:11 by , 11 years ago
Well a CmpPositionStatic
couldn't be used for entities that decay (like destroyed structures) but it especially breaks in Atlas :( There would need to be a way to force CmpVisualActor
to reinterpolate when functions like JumpTo()
are called. I wonder if we won't end up having the same issues or worse by the time all the special cases are figured out (like terrain alteration)... Another option would be to re-use CmpPosition
and add a <Static/>
flag that could be changed at runtime or set in the templates, and override that flag in Atlas. I don't know, there doesn't seem to be any nice way of doing this...
Peformance-wise the CmpPositionStatic
approach is very similar to what we have now, except it doesn't help with stationary units.
comment:12 by , 11 years ago
I have a patch for terrain alteration (#1988). So if you want to test how that influences performance, you can apply the patch for testing.
comment:14 by , 11 years ago
Milestone: | Alpha 14 |
---|---|
Resolution: | → invalid |
Status: | reopened → closed |
I'll have a look
Did a git bisect:
cb4f9194a663b34eb82c468a6c3fbfa1cffd6af1 is the first bad commit commit cb4f9194a663b34eb82c468a6c3fbfa1cffd6af1 Author: ben <ben@3db68df2-c116-0410-a063-a993310a9797> Date: Sat Mar 23 17:59:54 2013 +0000
That's r13323
Edit: Attached a patch that fixes it for me