search

peter-kish / gloot

A universal inventory system for the Godot game engine.
MIT License
564 stars 21 forks source link

fix some crashes. #158

Closed LeeWannacott closed 7 months ago

peter-kish commented 7 months ago

Hey, can you give me some additional info about the crashes? I want to inspect the problem and see if there are other places that need fixing too.

LeeWannacott commented 7 months ago

@peter-kish Yes, sorry, this is just the same thing as the PR I closed. I'm not sure if you saw my comment on it.

Basically this happens when switching scenes (which deletes the current scene) because you are then trying to access something that doesn't exist in memory and has already been freed (because I assume it doesn't delete everything at once).

From my understanding, you have to check for null because if obj doesn't actually check if it is valid in memory (which you can do with is_instance_valid(object), or check for null) , although this is changing in 4.3 (Third bullet point from the bottom: Freed objects are now treated differently from null in comparison operators ([GH-73896](https://github.com/godotengine/godot/pull/73896)), which ensures consistency for programmers when managing instances and memory. https://godotengine.org/article/dev-snapshot-godot-4-3-dev-1/)

I would recommend reading through this PR for a better explanation: https://github.com/godotengine/godot/pull/73896

Also, All good if you just want to make the changes yourself if that's easier than me opening PRs.

peter-kish commented 7 months ago

@LeeWannacott Cool, thanks! This gives me some additional insights:

peter-kish commented 7 months ago

I put together a very simple example where I do scene switching between two scenes, but I was unable to get anything to crash ☹️

scene_switching

Even when I switch scenes in the middle of a drag&drop operation, everything seems to work fine 🤔. Do you have any additional ideas how to reproduce this?

LeeWannacott commented 7 months ago

@peter-kish here is your two bugs reproduced (one for the inventory grid and one for when item is in item slot) in minimal project with video attached: https://github.com/peter-kish/gloot/assets/49783296/3338fb57-b237-4157-907b-36ab04b2bfc0

Minimal reproduced project (WASD to move, you are a blue square your inventory is a blue square, the next world is a bluer square IGN reviews 10/10). inventory-bugs-minimal.zip

It also has the dragging item is no where near the cursor bug as a bonus https://github.com/peter-kish/gloot/issues/160 :sweat_smile:

Note: I deleted my prior comment because it was misleading there are two bugs not one (well three if you count the drag item one)

peter-kish commented 7 months ago

Great, thanks! Seems like some unnecessary refresh calls are causing the problem, but I'll investigate...

peter-kish commented 7 months ago

Ok, I figured out that the main reason why these crashes were happening in your examples but not in mine is the scene structure. The order/hierarchy of nodes determines the order of their deletion and there were some cases I didn't cover when testing (cases where you'd get a crash) because I was always using the same scene structure, which was different from yours. Checking if the nodes are freed before using them, of course, fixes the issue.

But I think I came up with a more general solution to this problem: Instead of adding such checks on places where the plugin crashes, I added the checks in all classes that can be created by the user and also depend on some nodes inside the scene tree to function properly. This is because it is difficult to guarantee that any of those nodes will be alive and in proper state at any point, especially when switching scenes.

Another thing I did was to do explicit is_instance_valid(obj) checks because they are far more readable than if obj or if obj!=null and emphasize that the validness of the object is also important, not just whether it's null.

I pushed the changes to the dev_v2.4.2 branch: https://github.com/peter-kish/gloot/commit/3f784f8154c231595d7c34c742a08e9ab599d001 (I might include a few more fixes in v2.4.2 before I merge it to master).

peter-kish commented 7 months ago

v2.4.2 has been merged. Will re-open this one if crashes still happen.