add a method for copying components #5654
Conversation
|
Looks good to me but would want another engine maintainer to take a look. |
| if (!MetaQuery.Resolve(target, ref metadataTarget, false)) | ||
| throw new ArgumentException($"Entity {target} is not valid.", nameof(target)); | ||
|
|
||
| if (!HasComponent<T>(source)) |
There was a problem hiding this comment.
utilizing something like CopyComps(source, ent, null, typeof(VocalComponent), typeof(SpeechComponent)); causes a Unknown component: IComponent error
| /// <param name="component">The copied component if successful</param> | ||
| /// <param name="meta">Optional metadata of the target entity</param> | ||
| /// <returns>Whether the component was successfully copied</returns> | ||
| bool CopyComponent(EntityUid source, EntityUid target, IComponent type, [NotNullWhen(true)] out IComponent? component, MetaDataComponent? meta = null); |
There was a problem hiding this comment.
shouldn't it all be TryCopyComponent, as it follow overall TryXXX Microsoft lovely pattern
There was a problem hiding this comment.
yes it should, i will fix it one day i swear
| @@ -341,6 +341,38 @@ public partial interface IEntityManager | |||
| /// <returns>If the component existed in the entity.</returns> | |||
| bool TryGetComponent([NotNullWhen(true)] EntityUid? uid, ushort netId, [NotNullWhen(true)] out IComponent? component, MetaDataComponent? meta = null); | |||
|
|
|||
| /// <summary> | |||
| /// Copy a single component from source to target entity and return a reference to the copied component. | |||
There was a problem hiding this comment.
'Tries to copy', and 'if source entity have it' should be here, probably
There was a problem hiding this comment.
Method description also leaves to interpretation the question of 'what will it do if target entity already have that component'. The closest thing we can imagine is - it will return false and null, but it is also possible it will
1 - return true and reference to existing one, without overwritng (it exists already, there it is, take it)
2 - return false and reference (no i didnt copied it, take the existing one you jerk)
3 - return true and null (its already there! but not copied. wierd one, yeah).
this can be avoided by making more overloads and mimics for TryComp and Resolve methods but i wouldn't advice that until someone else asks. Clear description will suffice i think :3 (and no, u better have at least some more description in summary, as people will read summary first, and could read description on property only by doing extra work)
| /// <summary> | ||
| /// Copy a single component from source to target entity and return a reference to the copied component. | ||
| /// </summary> | ||
| /// <param name="source">The source entity to copy the component from</param> |
There was a problem hiding this comment.
Other xml-docs in file have dot at the end so this prolly all should too :3
And i'm not sure 'The' is nessesary here?... sorry if i'm wrong <_<
There was a problem hiding this comment.
Well, thats fair for all of the comments here
| @@ -341,6 +341,38 @@ public partial interface IEntityManager | |||
| /// <returns>If the component existed in the entity.</returns> | |||
| bool TryGetComponent([NotNullWhen(true)] EntityUid? uid, ushort netId, [NotNullWhen(true)] out IComponent? component, MetaDataComponent? meta = null); | |||
|
|
|||
| /// <summary> | |||
| /// Copy a single component from source to target entity and return a reference to the copied component. | |||
There was a problem hiding this comment.
Method description also leaves to interpretation the question of 'what will it do if target entity already have that component'. The closest thing we can imagine is - it will return false and null, but it is also possible it will
1 - return true and reference to existing one, without overwritng (it exists already, there it is, take it)
2 - return false and reference (no i didnt copied it, take the existing one you jerk)
3 - return true and null (its already there! but not copied. wierd one, yeah).
this can be avoided by making more overloads and mimics for TryComp and Resolve methods but i wouldn't advice that until someone else asks. Clear description will suffice i think :3 (and no, u better have at least some more description in summary, as people will read summary first, and could read description on property only by doing extra work)
| { | ||
| // Use the concrete type of the sourceComponent to call the generic version | ||
| var type = sourceComponent.GetType(); | ||
| var method = GetType().GetMethod(nameof(CopyComponent), 1, [typeof(EntityUid), typeof(EntityUid), type.MakeByRefType(), typeof(MetaDataComponent) |
There was a problem hiding this comment.
better use GetType result to duplicate generic version of code. Reflection of this type could be used if method that was generated dynamically would be cached for each requested type, but sadly it still will make code poorly readable. maybe there is an even better idea how to do it, but using dynamic method creation here is a bad fit :(
There was a problem hiding this comment.
The overhead of this will blow using generics out of the water, I'll try and re-write something if I get a chance.
|
|
||
| foreach (var type in types) | ||
| { | ||
| if (!CopyComponent(source, target, type, out _, meta)) |
There was a problem hiding this comment.
each call will lead to dirty internally, i wonder if such bulk operations could be performed in a more optimal manner if dirty-related code would be called once?..
There was a problem hiding this comment.
If metadatacomp is passed through it is just a field check for lastmodified which is not super bad for now.
title, the result of me being sleep deprived
adds EntityManager methods to either copy a component from one entity to another or copy multiple components from params
the code itself is pretty simple, the EntitySystem proxy methods are
CopyCompandCopyCompsthanks to Sloth, FairlySloggersPanda, Kickguy and everyone else who helped make this code