fix some crashes.

[kyle-wannacott]

No description provided.

[peter-kish]

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.

[kyle-wannacott]

@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: godotengine/godot#73896

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

[peter-kish]

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

• I haven't tested scene switching with my plugin at all.
• I might have had some misconceptions about if obj==null and if obj checks. I was pretty confident that they are the same, but reading through that PR that doesn't seem to be the case (also Inconsistency in boolean validation of [Freed Object] godotengine/godot#59816).

[peter-kish]

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

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?

[kyle-wannacott]

@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 #160 😅

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]

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

[peter-kish]

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: 3f784f8 (I might include a few more fixes in v2.4.2 before I merge it to master).

[peter-kish]

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