search

leudz / shipyard

Entity Component System focused on usability and flexibility.
Other
754 stars 46 forks source link

fix: Removed delete short-circuiting logic #184

Closed jack-cooper closed 1 year ago

jack-cooper commented 1 year ago

It looks like the the impl of Delete for tuples joins together the results with ||, which means that after the first delete succeeds the short-circuiting behaviour prevents any further deletes from actually taking place.

Swapping || for && is possibly the least invasive fix that would solve the problem in the expected case, where all views do in fact have an entry for the provided EntityId, but I'm not sure I love the idea of any kind of short-circuiting behaviour for Delete. For me at least, the most natural assumption is that it will simply try to delete every component from every view that you call it upon. As such, I've simply swapped it out for |.

I'm in 2 minds about whether or not I prefer & or | for the return value, i.e. returning true if all vs any components were deleted. Returning a simple bool is perhaps a little less than ideal for the case of multiple deletes, but I guess that's a problem that already exists and I'm not even convinced that I feel over-complicating the return value of delete is worth any significant changes.

I've also added a test case which fails on current master and passes after this change to hopefully catch this going forwards :smile:

leudz commented 1 year ago

Thanks for spotting this and adding a test for it. I agree that a single bool is not ideal but it's still possible to call delete on individual views to get a bool per delete.