Skip to content

Commit

Permalink
Fix linked list corruption bug when spawning a new entity
Browse files Browse the repository at this point in the history
There is a rare linked list bug in that will drop an entity if an
entity's `Process` routine does the following:

  1) Spawns a new entity whose listId is the same as the current listId
  2) The entity changes its list on the same frame it spawned the entity

The problem is that the Entity GameLoop was holding the address of the
previous entity, and the Entity Spawn routines add the newly spawned
entity to the beginning of the entity list.  When the new entity is
spawned onto the currently looping list, `_previousEntity` has the
potential to be invalidated, causing an entity to be dropped.

We solve this dilemma by having a special list that temporarily stores
the newly spawned entities and prevents `_previousEntity` from being
invalidated.  Then, at the start of each frame the entities will be
moved into their appropriate list and the entity loop continues as
normal.  Luckily, this behaviour already exists in
`Entity.ProcessGameLoop`, the `tryToActivateAgain` list.

Some unit tests that relied on the old behaviour have also been updated
to pass successfully.
  • Loading branch information
undisbeliever committed Oct 20, 2018
1 parent 4847ea8 commit 2722765
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 40 deletions.
9 changes: 7 additions & 2 deletions src/entity/_variables.inc
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,13 @@ namespace Entity {
allocate(deactivated, wram7e, 2)
constant _DEACTIVATED_INDEX = deactivated - FIRST

// Entities that could not be activated in this frame
// - Gameloop will try and reactivate them on next frame
// List of entities that are to be activated at the start of the
// next frame.
//
// This list contains either:
// A) Entities that could not be activated in the previous
// frame, and
// B) Newly spawned active entities.
allocate(tryToActivateAgain, wram7e, 2)
constant _TRY_TO_ACTIVATE_AGAIN_INDEX = tryToActivateAgain - FIRST

Expand Down
64 changes: 26 additions & 38 deletions src/entity/spawn.inc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
//
// Entity Spawning Routines.
//
// NOTE: Newly spawned entities are not immediately activated or added to their
// entity list when spawned. Instead the newly spawned entity will be
// activated and moved to its entity list at the start of the next frame.
//
//
// This file is part of the UnTech Game Engine.
// Copyright (c) 2016 - 2017, Marcus Rowe <undisbeliever@gmail.com>.
Expand Down Expand Up @@ -125,44 +129,36 @@ allocateTmpWord(yPos)
tax
a16()
lda.l EntityAlwaysActiveList - 1,x
bmi IsActive
bmi SkipActiveWindowTest

BranchEntityOutsideActiveWindow(IsInactive)

IsActive:
// entity is activate.
// Activate and insert into the entity list

jsr MetaSprite.Activate
bcc CouldNotActivate
SkipActiveWindowTest:
// We cannot move the entity into its entity list while
// the entity-loop is running.
// Instead we move it to the `tryToActivateAgain` list,
// where it will be activated and moved to the correct
// entity list at the start of the next frame.

lda.b BaseEntity.listId
and.w #0xff
asl
tax
lda.w lists.tryToActivateAgain
sta.b BaseEntity.next
tdc
sta.w lists.tryToActivateAgain

bra MoveToListX
bra EndIf


InvalidEntityListId:
break(INVALID_ENTITY_LIST_ID)

CouldNotActivate:
ldx.w #lists._TRY_TO_ACTIVATE_AGAIN_INDEX
bra MoveToListX

IsInactive:
ldx.w #lists._DEACTIVATED_INDEX

MoveToListX:
// Insert the entity into the given list Id index
// Insert the entity into the deactivated list

lda.w lists.FIRST,x
lda.w lists.deactivated
sta.b BaseEntity.next
tdc
sta.w lists.FIRST,x

bra EndIf
sta.w lists.deactivated

EndIf:

Expand Down Expand Up @@ -492,24 +488,16 @@ SpawnProjectile:
lda.w tmp_entityDoingSpawning
jsr (BaseEntityFunctionTable.Init,x)

// Assume projectiles always activate, even if it causes glitches.
jsr MetaSprite.Activate


// Insert projectile into the entity list

lda.b BaseEntity.listId
and.w #0xff
cmp.w #Entity.lists.N_LISTS
beq InvalidEntityListId

asl
tax
// We cannot move the entity into its entity list while the
// entity-loop is running.
// Instead we move it to the `tryToActivateAgain` list, where it
// will be activated and moved to the correct entity list at the
// start of the next frame.

lda.w lists.FIRST,x
lda.w lists.tryToActivateAgain
sta.b BaseEntity.next
tdc
sta.w lists.FIRST,x
sta.w lists.tryToActivateAgain

ldx.w MetaSprite.ActionPoint.address

Expand Down
2 changes: 2 additions & 0 deletions unit_tests/src/tests/entity/counters.inc
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ function _SetupCounterTest {
jsr Entity.Spawn
bcc Fail

jsr Entity.ProcessGameLoop

sec
rts

Expand Down
12 changes: 12 additions & 0 deletions unit_tests/src/tests/entity/spawn.inc
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ function Spawn {
cmp.w #Entity.N_ENTITIES - 1
bne Fail

// Must process the game loop to move the new entity into the entity list
jsr Entity.ProcessGameLoop

// Test first list length
jsr _FirstListLength
cmp.w #1
Expand Down Expand Up @@ -92,6 +95,9 @@ constant tmp = Test.tmp
bne Fail
stz.w Warnings.current

// Must process the game loop to move the new entities into their entity lists
jsr Entity.ProcessGameLoop

// Test free list is 0
jsr _FreeListLength
bne Fail
Expand Down Expand Up @@ -157,6 +163,9 @@ constant tmp = Test.tmp
bne Fail


// Must process the game loop to move the new entities into their entity lists
jsr Entity.ProcessGameLoop

// test list decreased by N_LISTS
jsr _FreeListLength
cmp.w #Entity.N_ENTITIES - Entity.lists.N_LISTS * 2
Expand Down Expand Up @@ -223,6 +232,9 @@ constant ONSCREEN = 0x1080
cmp.w #Entities.BlankEntity.EntityId_0 + Entity.lists.N_LISTS
bcc Loop

// Must process the game loop to move the new entities into their entity lists
jsr Entity.ProcessGameLoop

// Verify list sizes

jsr _FreeListLength
Expand Down

0 comments on commit 2722765

Please sign in to comment.