Skip to content

Implement update or insert operation on World and CommandBuffer#434

Closed
K-Adam wants to merge 1 commit intoRalith:masterfrom
K-Adam:feat/update-or-insert
Closed

Implement update or insert operation on World and CommandBuffer#434
K-Adam wants to merge 1 commit intoRalith:masterfrom
K-Adam:feat/update-or-insert

Conversation

@K-Adam
Copy link
Contributor

@K-Adam K-Adam commented Feb 28, 2026

We sometimes need an update-or-default pattern: if an entity already has a component, mutate it; otherwise insert a default value and then apply the update. This is especially useful for accumulator-style components or components that hold collections.

This can be implemented for World by trying entity_ref.get::<&mut C>() and falling back to insert_one. However, this does not work correctly with CommandBuffer, since querying the World while staging changes may yield incorrect results if an insert_one or remove_one has already been queued.

This PR adds an update_or_insert_one utility that handles this pattern safely by mutating the component if it exists, or lazily inserting it otherwise.

@K-Adam K-Adam force-pushed the feat/update-or-insert branch from 47989ac to 469e69c Compare February 28, 2026 16:55
Ok(())
}

fn update_or_insert_one_inner<C: Component, F: FnOnce(&mut C), D: FnOnce() -> C>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the additional _inner layer? It does not seem to be a cold path or have less generic arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, _inner isn’t for performance, its mainly an organizational split (preconditions in the public method, mutation in a helper), consistent with insert/insert_inner. Since it has one call site, I can inline it for simplicity.

@Ralith
Copy link
Owner

Ralith commented Feb 28, 2026

Thanks for the PR. While I appreciate that a use case exists, I don't think this feature makes a lot of sense to include, especially as implemented. If you're going to store a Vec<Box<dyn FnMut(&mut world)>>, then you might as well expose that full power to the user rather than a more complex, less capable narrow interface as written here. Even a directly exposed Vec<Box<dyn FnMut(&mut world)>> in CommandBuffer adds no significant value over the user managing such a structure entirely by hand: it doesn't save a significant amount of code or add any efficiency.

@K-Adam
Copy link
Contributor Author

K-Adam commented Feb 28, 2026

Thanks for the PR. While I appreciate that a use case exists, I don't think this feature makes a lot of sense to include, especially as implemented. If you're going to store a Vec<Box<dyn FnMut(&mut world)>>, then you might as well expose that full power to the user rather than a more complex, less capable narrow interface as written here. Even a directly exposed Vec<Box<dyn FnMut(&mut world)>> in CommandBuffer adds no significant value over the user managing such a structure entirely by hand: it doesn't save a significant amount of code or add any efficiency.

Thank you for the feedback! I think the main benefit would be command ordering. A mutator in CommandBuffer can be interleaved at an exact position between buffered spawn/insert/remove operations and executed with the same order.

What do you think about adding something like a queue(f: impl FnOnce(&mut World) + Send + Sync + 'static) to CommandBuffer?

@Ralith
Copy link
Owner

Ralith commented Feb 28, 2026

Hmm, ordering wrt. other operations is a fair point. Given how simple the added code for that variation would be, I don't think I could object.

@K-Adam
Copy link
Contributor Author

K-Adam commented Feb 28, 2026

I created a new PR with CommandBuffer::queue

#435

@K-Adam K-Adam closed this Feb 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants