makspll / bevy_mod_scripting

Bevy Scripting Plugin
Apache License 2.0
390 stars 31 forks source link

Fix removal of script contexts #81

Closed zwazel closed 11 months ago

zwazel commented 11 months ago

This should fix #79.

Okay i found following issue: the system script_remove_synchronizer handles removing the contexts of the script. https://github.com/makspll/bevy_mod_scripting/blob/32eba12bc5e2d87ab05cfbf4a2dd14063d3842a2/bevy_mod_scripting_core/src/systems.rs#L94-L103

The problem here is, that .remove_context expects the script ID, as context_entities uses it as the Key. https://github.com/makspll/bevy_mod_scripting/blob/32eba12bc5e2d87ab05cfbf4a2dd14063d3842a2/bevy_mod_scripting_core/src/hosts.rs#L267-L271

Which results in a very unreliable removing of the scripts. especially once the entities have multiple scripts and not just one.

I have updated the script_remove_synchronizer system, to loop through all context_entities and find those with a matching entity id. The problem I see with this solution is that we have to loop through everything, as we can't break after one found match. because each entity could have multiple scripts, so we need to loop through all of them to find all. https://github.com/makspll/bevy_mod_scripting/blob/745c20bb9e22d18761da7c50e96ee7f4766e790f/bevy_mod_scripting_core/src/systems.rs#L94-L112

zwazel commented 11 months ago

I just added some changes to the context_entities. It now takes Entity as a key and holds a Vec of Script IDs. https://github.com/makspll/bevy_mod_scripting/blob/9cf9e7f781ecfa5119e44e7f4ea5ad1b2337b07d/bevy_mod_scripting_core/src/hosts.rs#L267-L271 I would love to hear some feedback from you, I personally think it makes more sense to use the entity as a key, instead of the script ID.

I'll now work on fixing the tests, and the other CI issues.

zwazel commented 11 months ago

I see, haven't considered that! In that case I'll quickly revert back to how it was before!

zwazel commented 11 months ago

Merge Request is back at only fixing the bug in removing script context

makspll commented 11 months ago

No worries, much appreciated! Just one code style comment and I am happy!