samdze / godot-native-bullets-plugin

Efficiently spawn and move high amounts of objects like bullets for bullet hells, particles and more.
MIT License
179 stars 15 forks source link

BulletID marshalled as System.Int32[] (-1, 0, 0) in C# and GDScript #9

Closed jondewoo closed 1 year ago

jondewoo commented 1 year ago

On macos, when calling obtain_bullet from C# using Godot.Object.Call(), the bullet is successfully created and shows up on screen, but the BulletID is marshalled as a System.Int32[] of value (-1, 0, 0).

While I'm not getting errors in subsequent calls to the API using a (-1, 0, 0) BulletID (e.g. set_bullet_property, which is a bit confusing), these calls have no effect.

Being new to both Godot and the plugin, it looks like the only two avenues to remedy to this would be:

Or maybe I missed something.

samdze commented 1 year ago

Will look into this, can you post a minimal reproduction project or the code? Does this only happen using C#?

jondewoo commented 1 year ago

Here is a repro repo: https://github.com/jderrough/godot-gdnative-struct-marshalling-repro It happens in both C# and GDScript using Godot 3.5.2 Stable on macOS Ventura 13.4.1. The plugin's code is the latest on GitHub, I believe.

Let me know if I can help further.

jondewoo commented 1 year ago

Looking at the plugin's code, it looks like every bullet in the pool has a shape_index/index set to -1 and a cycle set to 0 upon creation. The shape_index is only updated if collisions_enabled is true in AbstractBulletsPool<Kit, BulletType>::_init. The set part of the BulletID is assigned the pool's set_index, 0 in my case. So this is why every bullet has a BulletID of value (-1,0,0). Why not use the bullet's RID for index?

Also BulletIDs are tested for validity when trying to get or set properties, and ignored if invalid. Maybe an error should be thrown or, at least, logged?

I'd like your opinions on both points before I make changes and open a PR, if you don't mind.

samdze commented 1 year ago

Sorry for the delay. So, yeah, you're right, shape_index is set only if collisions_enabled is true and that breaks the IDs of every non colliding bullet.

I didn't check if this works, but maybe it would be enough to just assign shape_index even if collisions_enabled is false.

Why not use the bullet's RID for index?

This would not work well due to how the plugin organizes bullets and indices to optimize for various operations the user can do (assign properties, check existence, get from collision shape, etc.)

Agree on emitting log errors when trying to set or get something that doesn't exist.