From a010cf233cc847c9d3862776162c9ea44b1b95bb Mon Sep 17 00:00:00 2001 From: Scott Wadden Date: Thu, 12 Oct 2023 21:59:47 -0300 Subject: [PATCH] Object leak/free fixes --- enu.nimble | 2 +- src/controllers/node_controllers.nim | 9 ++-- src/controllers/script_controllers/worker.nim | 7 ++-- src/models/bots.nim | 3 ++ src/models/builds.nim | 3 ++ src/models/players.nim | 7 +++- src/models/signs.nim | 9 ++-- src/models/units.nim | 42 +++++++++++++------ 8 files changed, 55 insertions(+), 27 deletions(-) diff --git a/enu.nimble b/enu.nimble index 1660df19..e420f6ed 100644 --- a/enu.nimble +++ b/enu.nimble @@ -31,7 +31,7 @@ else: requires "nim >= 1.6.10", "https://github.com/arnetheduck/nim-results#f3c666a", "https://github.com/dsrw/godot-nim#892c482", - "https://github.com/dsrw/model_citizen 0.18.6", + "https://github.com/dsrw/model_citizen 0.18.8", "https://github.com/dsrw/nanoid.nim 0.2.1", "cligen 1.6.0", "https://github.com/treeform/pretty", diff --git a/src/controllers/node_controllers.nim b/src/controllers/node_controllers.nim index f4eb4c19..04713f13 100644 --- a/src/controllers/node_controllers.nim +++ b/src/controllers/node_controllers.nim @@ -19,7 +19,7 @@ proc remove_from_scene(unit: Unit) = unit.units.clear for child in units: child.remove_from_scene() - unit.parent = nil + if unit.node of BuildNode: BuildNode(unit.node).model = nil elif unit.node of BotNode: @@ -30,9 +30,8 @@ proc remove_from_scene(unit: Unit) = debug "removing node", unit_id = unit.id unit.node = nil - if unit of Build: Build(unit).destroy - elif unit of Bot: Bot(unit).destroy - elif unit of Sign: Sign(unit).destroy + unit.destroy + unit.parent = nil proc add_to_scene(unit: Unit) = debug "adding unit to scene", unit = unit.id @@ -103,7 +102,7 @@ proc find_nested_changes(parent: Change[Unit]) = find_nested_changes(change) elif Added in change.changes: # FIXME: this is being set for the worker thread in script_controller - change.item.parent = parent.item + change.item.fix_parents(parent.item) change.item.add_to_scene() elif Removed in change.changes: reset_nodes() diff --git a/src/controllers/script_controllers/worker.nim b/src/controllers/script_controllers/worker.nim index 5bcaaf8d..dac1af0b 100644 --- a/src/controllers/script_controllers/worker.nim +++ b/src/controllers/script_controllers/worker.nim @@ -68,7 +68,9 @@ proc change_code(self: Worker, unit: Unit, code: Code) = var edits = unit.shared.edits for id in edits.value.keys: if id != unit.id and edits[id].len == 0: + let edit = edits[id] edits.del id + edit.destroy unit.reset() state.pop_flag ConsoleVisible @@ -128,7 +130,7 @@ proc watch_units(self: Worker, body(unit, change, added, removed) if added: # FIXME: this is being set for the main thread in node_controller - unit.parent = parent + unit.fix_parents(parent) unit.frame_created = state.frame_count unit.collisions.track proc(changes: seq[Change[(string, Vector3)]]) = unit.script_ctx.timer = get_mono_time() @@ -267,9 +269,6 @@ proc worker_thread(params: (ZenContext, GameState)) {.gcsafe.} = while i < state.units.len: if state.units[i].id == \"player-{ctx_name}": var player = Player(state.units[i]) - if player.units.len > 0: - Sign(player.units[0]).owner = nil - player.units.clear state.units.del i else: i += 1 diff --git a/src/models/bots.nim b/src/models/bots.nim index d5301c82..4f5a086b 100644 --- a/src/models/bots.nim +++ b/src/models/bots.nim @@ -60,6 +60,9 @@ method reset*(self: Bot) = self.velocity = vec3() self.units.clear() +method destroy*(self: Bot) = + self.destroy_impl + proc init*(_: type Bot, id = "bot_" & generate_id(), transform = Transform.init, clone_of: Bot = nil, global = true, parent: Unit = nil): Bot = diff --git a/src/models/builds.nim b/src/models/builds.nim index 27f61bb1..4942fc8c 100644 --- a/src/models/builds.nim +++ b/src/models/builds.nim @@ -374,6 +374,9 @@ method ensure_visible*(self: Build) = self.start_color self.draw(vec3(), (Computed, color)) +method destroy*(self: Build) = + self.destroy_impl + proc init*(_: type Build, id = "build_" & generate_id(), transform = Transform.init, color = default_color, clone_of: Unit = nil, global = true, diff --git a/src/models/players.nim b/src/models/players.nim index 75e018f0..f912583b 100644 --- a/src/models/players.nim +++ b/src/models/players.nim @@ -10,7 +10,7 @@ proc init*(_: type Player): Player = input_direction_value: ~Vector3, cursor_position_value: ~((0, 0)) ) - result.init_unit + result.init_unit(shared = false) result.global_flags += Global method on_begin_turn*(self: Player, direction: Vector3, degrees: float, @@ -40,3 +40,8 @@ proc `open_code=`*(self: Player, code: string) = unit.message = code unit.global_flags += Visible return + +method destroy*(self: Player) = + if self.units.len > 0: + Sign(self.units[0]).owner = nil + self.units.clear diff --git a/src/models/signs.nim b/src/models/signs.nim index cc34cd78..00b67bda 100644 --- a/src/models/signs.nim +++ b/src/models/signs.nim @@ -9,16 +9,16 @@ proc init*(_: type Sign, message: string, more = "", owner: Unit, id: "sign_" & generate_id(), message_value: ~message, more_value: ~more, - glow_value: ~0.0, width_value: ~width, height_value: ~height, size_value: ~size, billboard_value: ~billboard, frame_created: state.frame_count, - color_value: ~action_colors[black], + start_color: action_colors[black], start_transform: transform, owner_value: ~owner, - text_only: text_only + text_only: text_only, + parent: owner ) self.init_unit result = self @@ -37,3 +37,6 @@ method main_thread_joined*(self: Sign) = elif Hover.removed: self.local_flags -= Highlight state.pop_flag ReticleVisible + +method destroy*(self: Sign) = + self.destroy_impl diff --git a/src/models/units.nim b/src/models/units.nim index 30823d1f..b5f916c9 100644 --- a/src/models/units.nim +++ b/src/models/units.nim @@ -3,6 +3,11 @@ import godotapi / spatial from pkg / core / godotcoretypes import Basis import core, models / [states, colors], libs / interpreters +proc fix_parents*(self: Unit, parent: Unit) = + self.parent = parent + for unit in self.units: + unit.fix_parents(self) + proc init_shared*(self: Unit) = ensure ?self.shared_value if ?self.parent: @@ -17,7 +22,7 @@ proc init_shared*(self: Unit) = let table = ~Table[Vector3, VoxelInfo] self.shared.edits[self.id] = table -proc init_unit*[T: Unit](self: T) = +proc init_unit*[T: Unit](self: T, shared = true) = with self: units = ~seq[Unit] transform_value = ~self.start_transform @@ -123,25 +128,35 @@ method on_collision*(self: Model, partner: Model, normal: Vector3) {.base, gcsaf method off_collision*(self: Model, partner: Model) {.base, gcsafe.} = discard -proc destroy*[T: Unit](self: T) = +method destroy*(self: Unit) {.base, gcsafe.} = + fail "override me" + +proc destroy_impl*(self: Bot | Build | Sign) = ensure ?self - if self of Sign: - # :( Sign(self) will fail to compile if T is a Build or a Bot. - Sign(Unit(self)).owner = nil + for unit in self.units: - if unit of Sign: - Sign(Unit(unit)).owner = nil + unit.destroy - # :( Parent isn't set properly for instances on the main thread - if self.parent == nil and "instance" notin self.id: + when self is Sign: + self.owner = nil + + if self.parent == nil: let shared = self.shared shared.edits.destroy self.shared = nil self.shared_value.destroy - Zen.thread_ctx.free(shared) + if Zen.thread_ctx.can_free(shared).freeable: + Zen.thread_ctx.free(shared) + else: + fail \"can't free shared {shared.id} for unit {self.id}" else: - self.shared = nil + if self.id in self.shared.edits: + let edit = self.shared.edits[self.id] + self.shared.edits.del(self.id) + edit.destroy + self.shared = nil + self.parent = nil for field in self[].fields: when field is Zen: # :( @@ -151,8 +166,9 @@ proc destroy*[T: Unit](self: T) = if state.open_unit == self: state.open_unit = nil - if Unit(state.open_sign) == self: - state.open_sign = nil + when self is Sign: + if state.open_sign == self: + state.open_sign = nil Zen.thread_ctx.free(self)