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)

0001-Reinterpolate-when-rotation-is-not-done-after-one-tu.patch (1.8 KB ) - added by sbte 11 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by sanderd17, 11 years ago

Component: Core engineUI & Simulation

comment:2 by sbte, 11 years ago

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

Fixes visual actor interpolation, handling position changes more cleanly, based on patch by sbte. Fixes rally point lines not updating, fixes #1865

That's r13323

Edit: Attached a patch that fixes it for me

Last edited 11 years ago by sbte (previous) (diff)

comment:3 by sbte, 11 years ago

Keywords: patch review added
Milestone: BacklogAlpha 14
Summary: Ranged units turning only 90 degrees when launching projectile[PATCH] Units turn at most 90 degrees when attacking

comment:4 by michael, 11 years ago

Priority: Should HaveRelease Blocker

comment:5 by historic_bruno, 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:6 by ben, 11 years ago

Owner: set to ben
Resolution: fixed
Status: newclosed

In 13489:

Fixes units not rotating correctly during e.g. attacks, patch by sbte, fixes #1906

comment:7 by leper, 11 years ago

Keywords: review removed

comment:8 by historic_bruno, 11 years ago

Resolution: fixed
Status: closedreopened

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 :(

comment:9 by historic_bruno, 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?

Last edited 11 years ago by historic_bruno (previous) (diff)

comment:10 by historic_bruno, 11 years ago

I'm at least going to try that and see what happens.

in reply to:  9 comment:11 by historic_bruno, 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 sanderd17, 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:13 by ben, 11 years ago

In 13500:

Reverts interpolation changes from r13489, r13323, r13251, r13244, r13243, r13223. Refs #1846, #1858, #1865, #1906

comment:14 by historic_bruno, 11 years ago

Milestone: Alpha 14
Resolution: invalid
Status: reopenedclosed
Note: See TracTickets for help on using tickets.