-
Notifications
You must be signed in to change notification settings - Fork 431
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.
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
Prototype reload + other enhancements #5371
base: master
Are you sure you want to change the base?
Prototype reload + other enhancements #5371
Conversation
# Conflicts: # RELEASE-NOTES.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of development/debugging, I'm honestly not sure I like entity prototype changes being automatically applied to all existing entities. Is that actually used much in release? I'm also somewhat biased because replays just don't support it, and I don't think there's any easy way to add support
public bool Equals(ComponentRegistry? other) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This equality override, and thus also the EntityPrototype
one, can actually pretty slow, because they go via MappingDataNode.Except()
. This could suddenly make anything that uses a HashSet<EntityPrototype>
or dictionary much slower.
Assuming these were just added specifically to do the equivalence check for prototype reloading, I'd rather that just be a separate method that has to be intentionally called. Removing the hashcode overrides would also remove all the Non-readonly field referenced in 'GetHashCode()'
warnings/suggestions.
// Modified | ||
if (!newComp.Mapping.Equals(oldComp.Value.Mapping)) | ||
{ | ||
modified = true; | ||
EntityManager.AddComponent(entity, newComp, overwrite: true, metaData); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that the data on an entity's component matches the data on the prototype's component, which isn't necessarily true, especially when reloading prototypes in a post map-init map.
What this would have to do instead is closer to map loading. I.e., using methods added in #5571, I guess it'd look something like this (though this is kinda pseudo code):
var compNode = _serManager.Write(EntitysCurrentComponentInstance)
var delta = compNode.Except(oldComp.Value.Mapping)
var newNode = _serManager.CombineMappings(delta, newComp.Mapping);
var comp = (IComponent) _serManager.Read(newComp.Component.GetType(), newNode, _context) EntityManager.AddComponent(entity, comp, overwrite: true);
Or alternatively, instead of creating a new instance, use serialization manager to load directly onto the entity's existing comp instance, though you'd probably still want to raise the comp remove & add events to ensure that its fully refreshed and changes get networked.
See the release notes. Main one is prototype modifications are also reflected, not just comp changes, as I wanted to prototype different sprites in game and there was no easy way to do it.
Something I'm not sure on is this will also affect loadprototype / VVd stuff. Should we just have an overload that avoids updating modified components (e.g. setting it to true on live and making it so admins have to opt out?)