Opened 4 years ago

Closed 4 years ago

#2285 closed defect (fixed)

Damage helper holds simulation aware data in static function

Reported by: sanderd17 Owned by: Josh
Priority: Release Blocker Milestone: Alpha 15
Component: UI & Simulation Keywords:
Cc: Josh Patch:

Description

The Damage helper script creates a new entity on the first missile fired and missed

Damage.dummyTargetEntity = Engine.AddEntity('special/dummy');

As the script is static, the dummyTargetEntity is always remembered, even if the game is terminated (without the engine closed). You can see this in Atlas:

  • Start on an empty map
  • place at least two enemy archers on the map, close to each other
  • start the simulation and let them fire until at least one arrow missed target
  • reset the simulation
  • Add a new entity, somewhere else on the map
  • When the simulation is started, the unit jumps to all positions of missed arrows, as it got the ID of the dummyTarget, and the id of the dummyTarget wasn't deleted.

Depending on the number of entities in previous games, it can even cause OOS problems if people didn't restart the entire game before starting a new game.

This should be solved properly, by adding a method to RangeManager? that allows a range query around a position instead of just around an entity.

Attachments (1)

damage.diff (7.6 KB) - added by sanderd17 4 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by Josh

Priority: Release BlockerShould Have

In r14254 I triaged the problem, but that also entailed removing any sort of entity caching so it's now less efficient then before. Long-term solution would be to add a new RangeManager? method.

comment:2 Changed 4 years ago by JoshuaJB

Owner: set to JoshuaJB
Resolution: fixed
Status: newclosed

In 14256:

Fix #2285 by using the original design with additional checks.

comment:3 Changed 4 years ago by sanderd17

Priority: Should HaveRelease Blocker
Resolution: fixed
Status: closedreopened

There was a report about an OOS problem after rejoining the game on IRC last night: http://irclogs.wildfiregames.com/2013-11-30-QuakeNet-%230ad.log (around 23:12)

It's quite clear that this problem has the same source. One player still has the Damage script running, with an entity ID attached to it, while that ID isn't serialized, so if the second player joins again, a different ID is used. Which causes different IDs for all following entities.

comment:4 Changed 4 years ago by Josh

Hmm, well I'll try to look at adding another method to the range manager again. Last time I got rather lost, but I know C++ far better now than I did a few months ago so I should be able to figure it out.

comment:5 Changed 4 years ago by Josh

Owner: changed from JoshuaJB to Josh
Status: reopenednew

Changed 4 years ago by sanderd17

Attachment: damage.diff added

comment:6 Changed 4 years ago by sanderd17

The attached diff should work, but I have no idea if the style is good enough.

comment:7 Changed 4 years ago by Josh

Looks good to me, except you're losing some of the validation in PerformQuery? which should be added back.

if (!cmpSourcePosition || !cmpSourcePosition->IsInWorld())
    return; 

comment:8 Changed 4 years ago by sanderd17

Well, now the method that calls PerformQuery? is supposed to give the position. And those methods also do the in-world checking (I can't check if the entity is in world if the position is already given).

All those methods already needed the entity position for themselves, as they needed to sort on that position after running the PerformQuery?. I just made the query for the position a bit earlier.

So it should be fine.

comment:9 Changed 4 years ago by Josh

Alright then, after another quick review, I would just recommend committing it.

comment:10 Changed 4 years ago by sanderd17

Resolution: fixed
Status: newclosed

In 14283:

  • Add method to rangemanager to query around a position instead of an entity
  • Use that method in the splash damage calculation

Fixes #2285

Note: See TracTickets for help on using tickets.