Fix removal of script contexts

[zwazel]

This should fix #79.

Okay i found following issue:
the system script_remove_synchronizer handles removing the contexts of the script.

bevy_mod_scripting/bevy_mod_scripting_core/src/systems.rs

Lines 94 to 103 in 32eba12

	pub fn script_remove_synchronizer<H: ScriptHost>(
	mut query: RemovedComponents<ScriptCollection<H::ScriptAsset>>,
	mut contexts: ResMut<ScriptContexts<H::ScriptContext>>,
	) {
	query.iter().for_each(|v| {
	// we know that this entity used to have a script component
	// ergo a script context must exist in ctxts, remove all scripts on the entity
	contexts.remove_context(v.index());
	})
	}

The problem here is, that .remove_context expects the script ID, as context_entities uses it as the Key.

bevy_mod_scripting/bevy_mod_scripting_core/src/hosts.rs

Lines 267 to 271 in 32eba12

	pub struct ScriptContexts<C> {
	/// holds script contexts for all scripts given their instance ids.
	/// This also stores contexts which are not fully loaded hence the Option
	pub context_entities: HashMap<u32, (Entity, Option<C>, String)>,
	}

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.

bevy_mod_scripting/bevy_mod_scripting_core/src/systems.rs

Lines 94 to 112 in 745c20b

	pub fn script_remove_synchronizer<H: ScriptHost>(
	mut query: RemovedComponents<ScriptCollection<H::ScriptAsset>>,
	mut contexts: ResMut<ScriptContexts<H::ScriptContext>>,
	) {
	for v in query.iter() {
	// we know that this entity used to have a script component
	// ergo a script context must exist in ctxts, remove all scripts on the entity
	let mut script_ids = Vec::new();
	for (script_id, (entity, ..)) in contexts.context_entities.iter() {
	if entity.index() == v.index() {
	script_ids.push(*script_id);
	}
	}
	for script_id in script_ids {
	contexts.remove_context(script_id);
	}
	}
	}

[zwazel]

I just added some changes to the context_entities.
It now takes Entity as a key and holds a Vec of Script IDs.

bevy_mod_scripting/bevy_mod_scripting_core/src/hosts.rs

Lines 267 to 271 in 9cf9e7f

	pub struct ScriptContexts<C> {
	/// holds script contexts for all scripts given their instance ids.
	/// This also stores contexts which are not fully loaded hence the Option
	pub context_entities: HashMap<Entity, Vec<(u32, Option<C>, String)>>,
	}

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.

[makspll]

While I see the benefits of keeping entities as the keys in change detection, it adds load to the hot path, i.e. iterating through all the scripts on top of decreasing cache locality with a nested Vec.

I'd much rather have to iterate through all the contexts to find the right scripts to remove than have to do some additional work when handling events :)

[zwazel]

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

[zwazel]

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

[makspll]

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