search

SiliconStudio / xenko

Old repo for Xenko Game Engine. Please use https://github.com/xenko3d/xenko instead.
http://xenko.com
1.54k stars 343 forks source link

GameSystemCollection.Remove doesn't remove item from underlying updateableGameSystems and drawableGameSystems lists #666

Open guygodin opened 6 years ago

guygodin commented 6 years ago

The AddGameSystem call specifies a ProfilingKey as the KeyValuePair.Value while the Remove call specifies null as the value; therefore the item doesn't get removed.

https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/engine/SiliconStudio.Xenko.Games/GameSystemCollection.cs#L331

https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/engine/SiliconStudio.Xenko.Games/GameSystemCollection.cs#L343

Kryptos-FR commented 6 years ago

First of all, congrats for the #666 issue :smiling_imp:

Secondly, could you update the description and provide a link to the related file(s) in the sources?

guygodin commented 6 years ago

Woah wicked! Links added to issue.

stefnotch commented 6 years ago

More info regarding this issue:

The AddGameSystem call specifies a ProfilingKey as the KeyValuePair.Value

https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/engine/SiliconStudio.Xenko.Games/GameSystemCollection.cs#L390

Also, according to this answer, "[A KeyValuePair] is a struct. This means it uses the default value equality. This simply compares the values of the fields to test for equality." So, this is almost certainly a bug.

Lastly,updateableGameSystems is a normal C# list instead of a seemingly more fitting dictionary?

guygodin commented 6 years ago

It should be a list since it is iterated over a lot (iterating over lists performs better than dictionaries). The solution is to iterate over the items of the list in the Remove method and check for KeyValuePair.Key equality like it’s done elsewhere in my opinion.