Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a method for copying components #5654

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

add a method for copying components #5654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bebc895
it just works tm
MilonPL Feb 4, 2025
e5cbd34
spans bad
MilonPL Feb 4, 2025
9124d01
Merge remote-tracking branch 'upstream/master' into copycomp
MilonPL Feb 7, 2025
dfb97d4
review
MilonPL Feb 8, 2025
fdba05d
maybe works?
MilonPL Feb 10, 2025
f885b68
Revert "maybe works?"
MilonPL Feb 10, 2025
090a35b
Revert "review"
MilonPL Feb 10, 2025
ec8f3e8
sloth is like an enigma
MilonPL Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions Robust.Shared/GameObjects/EntityManager.Components.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,58 @@ public bool TryGetComponent([NotNullWhen(true)] EntityUid? uid, ushort netId,
return TryGetComponent(uid.Value, netId, out component, meta);
}

/// <inheritdoc/>
public bool CopyComponent(EntityUid source, EntityUid target, IComponent sourceComponent, [NotNullWhen(true)] out IComponent? component, MetaDataComponent? meta = null)
{
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :(

Copy link
Contributor

Choose a reason for hiding this comment

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

The overhead of this will blow using generics out of the water, I'll try and re-write something if I get a chance.

])!
.MakeGenericMethod(type);

var parameters = new object?[] { source, target, null, meta };
var result = (bool)method.Invoke(this, parameters)!;
component = (IComponent?)parameters[2];
return result;
}

/// <inheritdoc/>
public bool CopyComponent<T>(EntityUid source, EntityUid target, [NotNullWhen(true)] out T? component, MetaDataComponent? meta = null) where T : IComponent
{
component = default;

if (!MetaQuery.Resolve(target, ref meta))
return false;

if (!TryGetComponent<T>(source, out var sourceComp))
return false;

var compReg = ComponentFactory.GetRegistration(typeof(T));
component = (T)ComponentFactory.GetComponent(compReg);

_serManager.CopyTo(sourceComp, ref component, notNullableOverride: true);

AddComponentInternal(target, component, compReg, true, false, meta);
return true;
}

/// <inheritdoc/>
public bool CopyComponents(EntityUid source, EntityUid target, MetaDataComponent? meta = null, params IComponent[] types)
{
if (!MetaQuery.Resolve(target, ref meta))
return false;

var allSuccessful = true;

foreach (var type in types)
{
if (!CopyComponent(source, target, type, out _, meta))
Copy link
Contributor

Choose a reason for hiding this comment

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

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?..

Copy link
Contributor

Choose a reason for hiding this comment

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

If metadatacomp is passed through it is just a field check for lastmodified which is not super bad for now.

allSuccessful = false;
}

return allSuccessful;
}

public EntityQuery<TComp1> GetEntityQuery<TComp1>() where TComp1 : IComponent
{
var comps = _entTraitArray[CompIdx.ArrayIndex<TComp1>()];
Expand Down
25 changes: 25 additions & 0 deletions Robust.Shared/GameObjects/EntitySystem.Proxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,31 @@ protected bool TryGetEntityData(NetEntity nuid, [NotNullWhen(true)] out EntityUi

#endregion

#region Component Copy

/// <inheritdoc cref="IEntityManager.CopyComponent"/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
protected bool CopyComp(EntityUid source, EntityUid target, IComponent type, [NotNullWhen(true)] out IComponent? component, MetaDataComponent? metadataTarget = null)
{
return EntityManager.CopyComponent(source, target, type, out component, metadataTarget);
}

/// <inheritdoc cref="IEntityManager.CopyComponent{T}"/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
protected bool CopyComp<T>(EntityUid source, EntityUid target, [NotNullWhen(true)] out T? component, MetaDataComponent? metadataTarget = null) where T : IComponent
{
return EntityManager.CopyComponent(source, target, out component, metadataTarget);
}

/// <inheritdoc cref="IEntityManager.CopyComponents"/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
protected bool CopyComps(EntityUid source, EntityUid target, MetaDataComponent? metadataTarget = null, params IComponent[] types)
{
return EntityManager.CopyComponents(source, target, metadataTarget, types);
}

#endregion

#region Component Has

/// <summary>
Expand Down
32 changes: 32 additions & 0 deletions Robust.Shared/GameObjects/IEntityManager.Components.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

'Tries to copy', and 'if source entity have it' should be here, probably

Copy link
Contributor

Choose a reason for hiding this comment

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

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>
/// <param name="source">The source entity to copy the component from</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

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 <_<

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, thats fair for all of the comments here

/// <param name="target">The target entity to copy the component to</param>
/// <param name="type">The type of component to copy</param>
/// <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);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it all be TryCopyComponent, as it follow overall TryXXX Microsoft lovely pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it should, i will fix it one day i swear


/// <summary>
/// Copy a typed component from source to target entity and return a reference to the copied component.
/// </summary>
/// <typeparam name="T">The type of component to copy</typeparam>
/// <param name="source">The source entity to copy from</param>
/// <param name="target">The target entity to copy to</param>
/// <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<T>(EntityUid source, EntityUid target, [NotNullWhen(true)] out T? component, MetaDataComponent? meta = null) where T : IComponent;

/// <summary>
/// Copy multiple components from source to target entity.
/// </summary>
/// <param name="source">The source entity to copy from</param>
/// <param name="target">The target entity to copy to</param>
/// <param name="meta">Optional metadata of the target entity</param>
/// <param name="types">Array of component types to copy</param>
/// <returns>Whether all components were successfully copied</returns>
bool CopyComponents(EntityUid source, EntityUid target, MetaDataComponent? meta = null, params IComponent[] types);

/// <summary>
/// Returns a cached struct enumerator with the specified component.
/// </summary>
Expand Down
Loading