search

Phazorknight / Cogito

Immersive Sim Template Project for GODOT 4
MIT License
670 stars 72 forks source link

Wieldable nodes/models not clearing when loading a save that's within the same scene. #166

Closed Phazorknight closed 1 month ago

Phazorknight commented 2 months ago

Cogito and Godot Engine Version: Godot 4.2.1.stable | Cogito beta 202404.01

Description: When I load a save state that's within the same level scene the player is currently in while having a wieldable equipped. That wieldable mesh stays active while the wieldable I had equipped in the save state is also being equipped, resulting in doubled-up wieldables in the viewport.

Reproduction steps:

  1. Start a level scene, pick up a wieldable and equip it.
  2. Save the game (while wieldable is equipped/being held)
  3. load the game while still being in the same level scene and still holding a wieldable
  4. When loading is done, the wieldable that was being held when loading is still there, while the wieldable that was being held in the save state is getting equipped.

Expected behavior: Equipped wieldables should be cleared when a game is loaded / before the player state is being loaded/instantiated.

Tagging @brian-holsters

brian-holsters commented 2 months ago

The PR fixes the issue, however if I'm being completely honest, I'm not a big fan of the loading system as a whole, at a first glance a lot of it feels kind of spaghetti. A big part of this feeling is obviously my lack of knowledge on the project's codebase as a whole, but I do think there are some things that could be handled a bit better.

For instance I think any sort of loading should always have some sort of "reset" call on whatever is being loaded, to keep the responsibility of factory_reset a part of each object.

Also the fact that the scene_manager knows of the existence of the player, the save slots, the specific data loading implementation of both levels and player seems a bit too much for a single component.

Phazorknight commented 2 months ago

Hey, thanks for this, will probably only have time to take a proper look when I'm back from my trip in about 2 weeks.

I agree that the save/load system is probably a bit messy, would probably warrant a separate discussion to see how it can be improved from the ground up. The reason the scene manager knows about all of these, is because scene transitioning and loading scene/player states are pretty much the same operation, just using different resources. Most Cogito objects already have a "set state" method, that basically makes sure they're handling their own state, but I made all of this up as I went and introducing better consistencies would definitely be on the to-do list. A lot of this also depends on what kind of object we're talking about since they won't always exist within a scene for persistency reasons.

Phazorknight commented 2 months ago

@brian-holsters I think I was able to fix this myself in https://github.com/Phazorknight/Cogito/commit/85401bcd1f4b1dbb3d89a6503722fb4a8c126592

In the set_state() method of the PlayerInteractionComponent, I perform a clean up of all previously instanced wieldable_nodes and related references, then I equip the wieldable by calling the use method on it's item. This seems to work nicely, but please take a look if you see any issues.

Phazorknight commented 1 month ago

Closing this as it's currently no longer an issue.