Generic animations - #1067
Conversation
github-actions
Bot
added
Status: Needs Review
Changes: C#
Changes: Localization
Changes: Sprite
Changes: YML
and removed
Status: Needs Review
labels
|
RSI Diff Bot; head commit 4bb8bc8 merging into 2c511bb Resources/Textures/_White/Mobs/Construct/artificer.rsi
Resources/Textures/_White/Mobs/Construct/juggernaut.rsi
Resources/Textures/_White/Mobs/Construct/wraith.rsi
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughЗаменены устаревшие клиентско/серверные системы анимаций (animated emotes, FlipOnHit) на новую сериализуемую инфраструктуру WhiteAnimationPlayerSystem с прототипами анимаций, новыми компонентами-триггерами и интеграцией в чат/прототипы. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Content.Server/Chat/Systems/ChatSystem.Emote.cs (1)
224-232:⚠️ Potential issue | 🟡 MinorПроверьте удаление сущности перед проигрыванием анимации.
Если обработчик события удалит сущность после
RaiseLocalEvent(Line 227), вызов_whiteAnimationPlayer.Playможет работать по удалённому uid. Добавьте проверку на удаление перед проигрыванием.🛡️ Предложение фикса
RaiseLocalEvent(uid, ref ev, true); // goob edit + if (Deleted(uid)) + return; // WD EDIT START if (proto.Animation.HasValue) _whiteAnimationPlayer.Play(uid, proto.Animation.Value);
🤖 Fix all issues with AI agents
In `@Content.Client/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs`:
- Around line 43-76: The CachePrototypes method currently aborts all caching
when an unknown track type yields a null AnimationTrack because of `if
(animationTrack == null) return;`; change this to skip that single track instead
(e.g. `if (animationTrack == null) { /* log warning */ continue; }`) so the loop
over animationPrototype.AnimationTracksData continues and the rest of the
prototype still gets cached; update the code around the null check in
CachePrototypes (referencing animationTrack, animationTrackData,
GetComponentProperty/GetControlProperty/GetPlaySound/GetSpriteFlick and
AnimationPrototype.Animation) to log a warning about the unsupported track type
and continue rather than returning, leaving _animations population intact.
- Around line 78-90: GetComponentProperty currently returns an empty
AnimationTrackComponentProperty when _component.TryGetRegistration fails, which
produces tracks without ComponentType; change GetComponentProperty so that when
_component.TryGetRegistration(animationTrackData.ComponentType, out var
registration, true) returns false it returns null (or an optional) instead of an
empty AnimationTrackComponentProperty, and update callers to skip tracks where
GetComponentProperty returns null; ensure references to
AnimationTrackComponentProperty.ComponentType and SetProperty are only used when
registration is present.
In `@Content.Server/_White/Animations/Commands/PlayAnimationCommand.cs`:
- Around line 16-44: The command treats arguments in the wrong order: when two
args are provided the code parses args[0] as the entity but also uses args[0] as
the animation id; fix Execute in PlayAnimationCommand so that when args.Length
== 2 you parse the second argument as the NetEntity (use
NetEntity.TryParse(args[1], out var netEntity) and set entity =
_entityManager.GetEntity(netEntity)) and keep the animation id as args[0] when
calling _entityManager.System<WhiteAnimationPlayerSystem>().Play(entity.Value,
args[0]); ensure the single-arg fallback still uses the attached entity and
animation id from args[0].
In `@Content.Shared/_White/Animations/AnimationTrackData.cs`:
- Around line 33-39: The DataField on the Property member is missing required:
true which allows null/missing YAML prototypes; update the DataField attributes
for the Property field in AnimationTrackControlPropertyData (class
AnimationTrackControlPropertyData -> Property) and also in
AnimationTrackComponentPropertyData (class AnimationTrackComponentPropertyData
-> Property) to include required: true so the parser will throw if the field is
absent.
- Around line 21-31: Add parser-level enforcement by marking the fields in
AnimationTrackComponentPropertyData as required: set the DataField attributes
for ComponentType and Property to include required: true, and initialize both
string fields with = default! (e.g., public string ComponentType = default!;
public string Property = default!;) to match existing style and avoid
nullable/empty values during YAML parsing; leave the class inheriting
AnimationTrackPropertyData unchanged.
In `@Content.Shared/_White/Animations/KeyFrameData.cs`:
- Around line 30-35: В классе KeyFrameSpriteFlickData добавьте атрибуту
[DataField] параметр required: true и инициализируйте поле State значением по
умолчанию (например пустой строкой), чтобы State не могло быть null; измените
объявление поля State внутри KeyFrameSpriteFlickData (параметры атрибута
DataField и значение по умолчанию) — это предотвратит NRE при использовании поля
в WhiteAnimationPlayerSystem, которое ожидает ненулевое значение.
- Around line 14-20: Добавьте в описание поля Value атрибут DataField с required
= true, чтобы гарантировать, что KeyFramePropertyData.Value всегда
инициализирована и исключить NRE при обращении к keyFrameProperty.Value.Value в
методе WhiteAnimationPlayerSystem.SetProperty; отредактируйте класс
KeyFramePropertyData (поле Value) и пересоберите, чтобы прототипы без этого поля
не проходили валидацию.
- Around line 22-28: KeyFrameSoundData.Sound can be null and is used without
null-check in WhiteAnimationPlayerSystem.ResolveSound; mark it required to avoid
NRE by adding the data attribute and non-null initializer: add
[DataField(required: true)] to the Sound field on class KeyFrameSoundData and
make the declaration non-nullable (e.g. public SoundSpecifier Sound = default!;)
so deserialization fails on missing data and callers can assume Sound is present
when calling ResolveSound.
In `@Content.Shared/_White/Animations/Systems/AnimateOnStartupSystem.cs`:
- Around line 5-17: The AnimateOnStartupSystem in Content.Shared currently
subscribes to ComponentStartup and calls _whiteAnimationPlayer.Play from
OnStartup, causing it to run on both client and server; either move the
AnimateOnStartupSystem class into Content.Server or guard the subscription with
if (IsServer) in Initialize so the
SubscribeLocalEvent<AnimateOnStartupComponent, ComponentStartup>(OnStartup) is
only registered on the server; ensure you still reference the same
AnimateOnStartupComponent, OnStartup handler, and _whiteAnimationPlayer.Play
usage so behavior remains identical but only executes server-side.
In `@Content.Shared/_White/Helpers/DynamicValueSerializer.cs`:
- Around line 40-41: The code uses the null-forgiving operator on the result of
serializationManager.Read, which can produce a NullReferenceException; in
DynamicValueSerializer (the code that calls node.Get<ValueDataNode>("type"),
GetType(serializationManager, type) and serializationManager.Read(...,
node.Get("value"), context)), remove the trailing '!' and explicitly handle a
null return from serializationManager.Read — either throw a clear exception with
context (including the 'type' and node info) or return/assign a safe default
value so callers won't get an unexpected NRE; ensure the null-handling is
applied right after the call that returns 'value'.
In `@Resources/Prototypes/_White/Animations/emote.yml`:
- Around line 83-107: В анимации с id Jump последний KeyFramePropertyData (в
блоке animationTracksData → componentType: Sprite → property: Offset) имеет
keyframe: 0.125, из‑за чего анимация длиной length: 0.25 содержит пустую вторую
половину; переместите/измените значение keyframe у последнего keyframe на 0.25
чтобы финальный кадр совпадал с длиной анимации.
- Around line 1-25: В анимации с id Flip ключи в animationTracksData → keyFrames
для компонента Sprite (property Rotation) имеют два последних keyframe с
значением 0.25 при общей length: 0.5, из‑за чего вторая половина пустая;
исправьте это, переместив финальный KeyFramePropertyData (значение 360) на
keyframe: 0.5 (либо альтернативно уменьшите length до 0.25) чтобы ключи
покрывали весь диапазон времени.
- Around line 27-81: В анимации type: animation с id: Spin (key: spin) все
keyFrames после первого имеют одинаковый keyframe 0.075, из‑за чего таймкоды не
возрастают и вращение «схлопывается»; исправьте значения keyframe в блоке
animationTracksData → keyFrames так, чтобы они были возрастающими и в сумме
давали length 0.6 (например: 0, 0.075, 0.15, 0.225, 0.3, 0.375, 0.45, 0.525,
0.6), оставив остальные поля (value/type/value) без изменений.
🧹 Nitpick comments (9)
Content.Shared/_White/BloodCult/Construct/ConstructComponent.cs (1)
13-18: Зафиксируйте численные значения enum для сетевой стабильности.Так вы снизите риск случайного изменения значений при будущих правках.
♻️ Возможная правка
public enum ConstructLayer : byte { - Base, - Unshaded + Base = 0, + Unshaded = 1 }Content.Shared/_White/Helpers/DynamicValueSerializer.cs (2)
70-71: Улучшить сообщение об ошибке.Сообщение
"NO TYPE "неинформативно. Рекомендуется использовать более описательное сообщение для упрощения отладки.♻️ Предлагаемое исправление
- throw new InvalidMappingException("NO TYPE " + typeValue.Value); + throw new InvalidMappingException($"Unable to resolve type '{typeValue.Value}'. Ensure the type name is fully qualified or registered.");
52-62: Валидация не проверяет разрешимость типа.Метод
Validateпроверяет только наличие полей "type" и "value", но не валидирует, что строка типа может быть успешно разрешена. Это может привести к ошибкам на этапеRead, а не на этапе валидации.Content.Shared/_White/Animations/Systems/SharedWhiteAnimationPlayerSystem.cs (1)
12-17: ИспользуйтеProtoId<AnimationPrototype>для типобезопасности и консистентности.Метод
PlayпринимаетProtoId<AnimationPrototype>, ноPlayAnimationMessage.AnimationIdобъявлен какstring. Это вызывает неявные преобразования в обе стороны (ProtoId → string на сервере, string → ProtoId на клиенте), что снижает типобезопасность. В кодовой базе уже используютсяProtoIdв сетевых сообщениях (например,RuneDrawerSelectedMessage), поэтому рекомендуется использоватьProtoId<AnimationPrototype>напрямую.♻️ Предлагаемое исправление
[Serializable, NetSerializable] -public sealed class PlayAnimationMessage(NetEntity animatedEntity, string animationId) : EntityEventArgs +public sealed class PlayAnimationMessage(NetEntity animatedEntity, ProtoId<AnimationPrototype> animationId) : EntityEventArgs { public NetEntity AnimatedEntity = animatedEntity; - public string AnimationId = animationId; + public ProtoId<AnimationPrototype> AnimationId = animationId; }Content.Server/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs (1)
7-10: LGTM!Серверная реализация корректно переопределяет абстрактный метод
Playи отправляет сетевое сообщение клиенту.Опционально: можно добавить проверку существования сущности перед отправкой сетевого события, чтобы избежать лишнего сетевого трафика для удалённых сущностей.
♻️ Опциональная проверка валидности сущности
public sealed class WhiteAnimationPlayerSystem : SharedWhiteAnimationPlayerSystem { - public override void Play(EntityUid uid, ProtoId<AnimationPrototype> animationId) => - RaiseNetworkEvent(new PlayAnimationMessage(GetNetEntity(uid), animationId)); + public override void Play(EntityUid uid, ProtoId<AnimationPrototype> animationId) + { + if (!Exists(uid)) + return; + + RaiseNetworkEvent(new PlayAnimationMessage(GetNetEntity(uid), animationId)); + } }Content.Shared/_White/Animations/KeyFrameData.cs (1)
6-12: Проверьте, что базовый KeyFrameData помечен DataDefinition.
Если Robust.Serialization требует[DataDefinition]на базовом типе, полеKeyframeможет не сериализоваться и останется 0. Сверьте с документацией и при необходимости добавьте атрибут.💡 Возможная правка
-[Serializable] +[Serializable, DataDefinition] public abstract class KeyFrameDataContent.Shared/_White/Animations/AnimationTrackData.cs (2)
5-11: Проверьте необходимость[DataDefinition]на базовом AnimationTrackData.
Если базовый тип без[DataDefinition]не участвует в сериализации,KeyFramesможет не загрузиться. Сверьте с документацией Robust.Serialization.💡 Возможная правка
-[Serializable] +[Serializable, DataDefinition] public abstract class AnimationTrackData
45-50: Для SpriteFlick лучше требовать LayerKey явно.
Иначе можно получить дефолтный enum‑значок, который не соответствует реальному слою.💡 Возможная правка
- [DataField] - [AlwaysPushInheritance] - public Enum LayerKey; + [DataField(required: true)] + [AlwaysPushInheritance] + public Enum LayerKey;Content.Shared/_White/Animations/Prototypes/AnimationPrototype.cs (1)
28-29: СделайтеAnimationбезопасным кnullи явно инициализируйте.Поле не сериализуется и сейчас форсируется
null!; если его читают до заполнения, возможен NRE. Предлагаю либо инициализировать его в пост‑обработке прототипа, либо сделать nullable и добавить проверки. По возможности лучше использовать конкретный тип вместоobject.♻️ Возможное смягчение null‑риска
- [ViewVariables] - public object Animation = null!; + [ViewVariables] + public object? Animation;
RedFoxIV
left a comment
RedFoxIV
left a comment
There was a problem hiding this comment.
В целом круто, но
- Предикт анимаций
На данный момент в шареде доступен только одинPlay()метод, который играет анимацию на клиенте и отправляет с сервера сообщение об анимации. Да, оно скипается проверкой на уже играющую анимацию. Нет, мне похуй, всё равно хуйня получается, надо чинить, особенно если добавится возможность принудительно прерывать текущую анимацию и заменять её новой - тогда этот баг себя покажет.
А ещё просто лишний трафик гоняется.
Проблема в том, что в 95% случаев, если анимация запускается из шареда - она по умолчанию будет запускаться у всех клиентов, в чей ПВС попадает анимируемая сущность, а у всех остальных - этой энтити просто не будет в игре или она будет засуспенжена. Повторный синхрон через серверное сообщение тут не требуется.
- чтобы при запуске длинной анимации она у нового игруна начиналась не сначала - тебе нужно будет дополнительно отслеживать время начала проигрывания каждой анимации и уведомлять об этом клиент, чтобы он сам мог посчитать оффсет. Например, новый компонент, который держит в себе словарь ключ - timestamp. На клиентском стартапе компонента все анимации, покрываемые этим словарём, будут сдвигаться в соответствии с тем, как давно началась анимация.
т.е. если у предмета начинает играть 10-секундная анимация, и через 5 секунд этот предмет попадает в ПВС какого-то игруна (допустим, телепортировался с лавы на станцию) - у игруна вызовется стартап этого компонента, компонент увидит, что анимация по ключу "ass_blast_usa" была запущена в 13 минут 37 секунд, а сейчас 13 минут 42 секунды - компонент сдвигает анимацию вперёд на 5 секунд и засыпает до следующего стартапа.
К сожалению, у меня недостаточно знаний о том, как работают РТшные анимации с ПВСом, а конкретно - если во время анимации энтити выпадает из ПВСа, а потом возвращается -
- анимация ломается?
- приостанавливается и по возвращению начинает играть с той же точки
- приостанавливается и по возвращению начинает играть с той точки, как если бы она не выпадала?
из того, что я помню о коде плейбека анимаций - если не первый вариант, то, скорее всего, второй.
-
DynamicValue
Это какой-то пиздец и абсолютная киллер фича (в плохом смысле) для любого, кто будет пилить анимации через прототипы. Необходимо что-то с этим сделать. -
фокус системы
WhiteAnimationPlayerSystem
Либо эта система играет роль более крутой системы анимации, либо эта система отвечает исключительно за проигрывание анимаций из прототипов.
В первом случае нужен более-менее полный паритет по фичам с обычной системой анимации, а во втором - более соответствующее название.
Я давно не трогал анимации, поэтому не смогу ответить на вопрос, какие ещё фичи могут понадобится. Предположи сам, что может делать код из Content'а с анимациями (помимо их запуска) и попробуй добавить соответствующие методы в свою систему, чтобы не нужно было добавлять в зависимости "проигрыватель анимаций" и "проигрыватель анимаций 2"
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Content.Client/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs`:
- Around line 33-43: The Initialize method in WhiteAnimationPlayerSystem
subscribes to _prototype.PrototypesReloaded with OnPrototypeReload but never
unsubscribes; add an override Shutdown (or override Shutdown if already present)
in WhiteAnimationPlayerSystem that removes the handler by calling
_prototype.PrototypesReloaded -= OnPrototypeReload and performs any necessary
cleanup (matching Initialize's subscribe/CachePrototypes usage) so the system
doesn't leak when stopped or recreated.
- Around line 59-93: В методе CachePrototypes переменная realLength сейчас
складывает все keyframe'ы всех треков; нужно вместо суммирования брать максимум
длины по трекам: для каждого animationTrackData вычислить длину трека (например
максимальное значение keyFrame.Keyframe converted в TimeSpan) и присвоить
realLength = TimeSpan.Max(realLength, trackLength); сохраните оставшуюся логику
(использование TimeSpan.FromSeconds при вычислении и присвоение animation.Length
= animationPrototype.Length ?? realLength), обновите места с
animationTrackData.KeyFrames и переменной
realLength/animationPrototype.Animation соответственно.
In
`@Content.Shared/_White/Animations/Systems/SharedWhiteAnimationPlayerSystem.cs`:
- Around line 77-81: The XML doc for StopPredicted incorrectly says "The local
client will play animation" (copied from PlayPredicted); update the
summary/comments for the abstract method StopPredicted (and any related XML
tags) to state that the local client will stop the animation for the recipient
while the server stops it for other players, mirroring the intended behavior and
wording from PlayPredicted but replacing "play" with "stop" and adjusting
tense/clarity accordingly.
- Around line 48-53: The XML doc comment for the abstract method Stop(EntityUid
uid, string animationKey) contains a typo ("stoped"); update the documentation
text to use the correct past participle "stopped" (e.g., change "the animation
that will be stoped." to "the animation that will be stopped.") so the
summary/param XML is spelled correctly for the Stop method.
🧹 Nitpick comments (5)
Content.Shared/_White/Animations/Systems/AnimateOnHitSystem.cs (1)
30-34: Необязательное наблюдение: проверкаIsDownдля пользователя приApplyToUser.Когда
ApplyToUser == true,PlayAnimationвызывается сtarget = args.User, и_standingState.IsDown(target)будет проверять состояние самого атакующего. На практике лежащий игрок, как правило, не может наносить удары ближнего боя, поэтому эта проверка избыточна (но безвредна). Если это осознанный выбор для перестраховки — всё в порядке.Content.Client/_White/Animations/Systems/AnimateOnStartupSystem.cs (1)
18-21: Рассмотрите пробросForceв методPlay.
ent.Comp.Forceиспользуется только для решения «проигрывать ли повторно» (строка 18), но не передаётся в_whiteAnimationPlayer.Play()как параметрforce. СигнатураPlayпринимаетbool force = false, который управляет принудительной заменой уже воспроизводящейся анимации. ЕслиForce = trueозначает «обязательно воспроизвести», логично передавать это и в сам плеер:♻️ Предлагаемое изменение
- _whiteAnimationPlayer.Play(ent, ent.Comp.Animation); + _whiteAnimationPlayer.Play(ent, ent.Comp.Animation, ent.Comp.Force);Content.Server/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs (1)
12-16: Нет проверки валидностиuidперед отправкой сетевого сообщения.Методы
PlayиStopвызываютGetNetEntity(uid)и отправляют сетевое событие без проверки, чтоuid— валидная сущность. Если передан невалидныйEntityUid, сервер всё равно отправит сообщение всем клиентам в PVS, хотя клиент его потом проигнорирует. Рекомендуется добавить ранний выход для минимизации лишнего сетевого трафика.♻️ Пример для основного перегруза Play
public override void Play(EntityUid uid, ProtoId<AnimationPrototype> animationId, bool force = false) { + if (!uid.Valid) + return; + var filter = Filter.Pvs(uid, entityManager: EntityManager); RaiseNetworkEvent(new PlayAnimationMessage(GetNetEntity(uid), animationId, force), filter); }Аналогичную проверку стоит добавить и в остальные перегрузы
Play/Stop.Content.Server/_White/Animations/Commands/PlayAnimationCommand.cs (1)
25-44: Нет проверки существования сущности после разрешенияNetEntity.
_entityManager.GetEntity(netEntity)может вернутьEntityUid, который уже удалён или невалиден. Стоит добавить проверку существования перед вызовомPlay, чтобы администратор получил понятную ошибку.♻️ Предлагаемое исправление
entity = _entityManager.GetEntity(netEntity); + + if (!_entityManager.EntityExists(entity)) + { + shell.WriteError(Loc.GetString("shell-invalid-entity-id")); + return; + } }Content.Client/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs (1)
202-216:PlayClientиPlayPredictedна клиенте идентичны.Оба метода проверяют
IsFirstTimePredictedи затем вызываютPlay(uid, animationId, recipient, force). Разницы в поведении между ними на клиенте нет. Если это осознанное решение (разница только на сервере), стоит оставить комментарий для ясности. Если предполагается различное поведение — нужно доработать.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Content.Client/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs`:
- Around line 170-184: In WhiteAnimationPlayerSystem.Play, avoid the direct cast
(Animation) animation.Animation which can throw if animation.Animation is null;
instead retrieve animation.Animation into a local (e.g. var animObj =
animation.Animation), perform a null check (or use as Animation and check
result) and return (optionally log a warning) if it's null before calling
_animationPlayer.Play; ensure you reference Play in WhiteAnimationPlayerSystem
and the animation variable/animation.Animation when adding the guard.
🧹 Nitpick comments (4)
Content.Shared/_White/Animations/Systems/SharedWhiteAnimationPlayerSystem.cs (2)
86-99: Поля сообщений — публичные мутабельные поля вместо свойств.
PlayAnimationMessageиStopAnimationMessageиспользуют публичные поля (public NetEntity AnimatedEntity = ...). Для сериализуемых сетевых сообщений обычно предпочтительнее использовать свойства с{ get; }или хотя бы{ get; set; }, чтобы предотвратить случайную мутацию после создания.♻️ Предлагаемое исправление
public sealed class PlayAnimationMessage(NetEntity animatedEntity, string animationId, bool force) : EntityEventArgs { - public NetEntity AnimatedEntity = animatedEntity; - public string AnimationId = animationId; - public bool Force = force; + public NetEntity AnimatedEntity { get; } = animatedEntity; + public string AnimationId { get; } = animationId; + public bool Force { get; } = force; } public sealed class StopAnimationMessage(NetEntity animatedEntity, string animationKey) : EntityEventArgs { - public NetEntity AnimatedEntity = animatedEntity; - public string AnimationKey = animationKey; + public NetEntity AnimatedEntity { get; } = animatedEntity; + public string AnimationKey { get; } = animationKey; }
20-28: Мелкие грамматические ошибки в XML-документации.В строках 21, 26, 56 и 61 написано "that play animation" / "that stop animation" — должно быть "that plays animation" / "that stops animation" (третье лицо единственного числа).
Content.Client/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs (2)
65-101: Мутация объекта прототипа — архитектурный антипаттерн.Строка 97 (
animationPrototype.Animation = animation) записывает вычисленное значение прямо в объект прототипа. Прототипы обычно считаются immutable и принадлежатIPrototypeManager. Мутация прототипа:
- Нарушает принцип неизменяемости прототипов и может вызвать неожиданное поведение при hot-reload (старое значение
Animationживёт до следующегоCachePrototypes).- Создаёт неявную связь — другие системы, читающие
AnimationPrototype.Animation, зависят от того, что клиентская система уже инициализировалась и закэшировала данные.Рекомендуется хранить вычисленные
Animationв отдельном словаре внутри системы (например,FrozenDictionary<string, Animation>), а не на самом прототипе.♻️ Эскиз предлагаемого рефакторинга
- private FrozenDictionary<string, AnimationPrototype> _animations = default!; + private FrozenDictionary<string, (AnimationPrototype Proto, Animation Anim)> _animations = default!; // В CachePrototypes: - animationPrototype.Animation = animation; + // не мутируем прототип ... - _animations = animationPrototypes.ToFrozenDictionary(x => x.ID); + _animations = animationPrototypes.ToFrozenDictionary( + x => x.ID, + x => (x, /* соответствующая animation */)); // В Play: - _animationPlayer.Play(uid, (Animation) animation.Animation, animation.Key); + _animationPlayer.Play(uid, cachedAnim, animation.Key);
106-134:GetComponentPropertyаллоцирует компонент для определения типа свойства.Строка 117:
_component.GetComponent(registration)создаёт экземпляр компонента только для того, чтобы получить тип свойства через рефлексию. Это происходит при кэшировании, так что не критично для рантайма, но стоит отметить — при большом количестве прототипов это может замедлить инициализацию.
RedFoxIV
left a comment
RedFoxIV
left a comment
There was a problem hiding this comment.
Не вижу смысла в обилии методов Play и почему нельзя сделать получение объекта типа Animation через метод в системе, а не какой-то непонятный костыль в прототипе.
| private void OnStartup(Entity<AnimateOnStartupComponent> ent, ref ComponentStartup args) | ||
| { | ||
| if (ent.Comp is { Played: true, Force: false, }) | ||
| return; | ||
|
|
||
| _whiteAnimationPlayer.Play(ent, ent.Comp.Animation); | ||
| ent.Comp.Played = true; | ||
| } |
There was a problem hiding this comment.
анимация, которая должна играть непосредственно после создания энтити, будет играть сразу после первого попадания в ПВС клиента
устранение этого недочёта потребует либо проигрывание анимации по отдельному сообщению с сервера, либо сверяться с временем создания энтити (что то, что это - хуита)
в целом на это можно пока что забить, но лучше иметь это в виду
| /// <summary> | ||
| /// Play animation on entity for every player in pvs range. | ||
| /// </summary> | ||
| /// <param name="uid">The UID of the entity.</param> | ||
| /// <param name="animationId">The ID of the animation that will be played.</param> | ||
| /// <param name="force">Determines whether this animation should play if an animation with the same key is already playing.</param> | ||
| public abstract void Play(EntityUid uid, ProtoId<AnimationPrototype> animationId, bool force = false); |
There was a problem hiding this comment.
большая часть кода с шареда захочет вызывать PlayClient, потому что она будет исполняться у всех на клиенте, а Play/PlayPredicted будут вызывать нежелательный повтор анимации в её начале
код, вызываемый на шареде, априори вызывается на всех клиентах, на которых он должен вызываться - дополнительный синхрон через сервер не нужен. Либо анимация запускается сразу на клиенте, либо сообщение о ней приходит с сервера. Всё.
Затрудняюсь в целом подобрать применение серверным методам по проигрыванию анимаций.
Нужно два метода:
- метод, который на клиенте играет анимацию на указанной энтити и ничего не делает на сервере
- метод, который на клиенте ничего не делает и на сервере отправляет всем клиентам, в чей ПВС попадает указанная энтити, сообщение об анимации.
There was a problem hiding this comment.
Не весь код в шараде априори вызывается на вcех клиентах на которых надо. Условно при ударе по объекту ивент MeleeHitEvent будет вызван только на клиенте того кто ударил. Все остальные могут получить его только через сервер.
There was a problem hiding this comment.
Как может пригодится 2-ой метод? Если тебе нужно проиграть что то только на сервере, то скорее всего и код у тебя находится на серверной части и тебе ничего не мешает использовать Play. В противном случае какая может быть причина не давать клиентам предугадывать проигрывание анимации?
| keyFrames: | ||
| - !type:KeyFramePropertyData | ||
| value: 0 | ||
| keyframe: 0 | ||
| - !type:KeyFramePropertyData | ||
| value: 90 | ||
| keyframe: 0.075 | ||
| - !type:KeyFramePropertyData | ||
| value: 180 | ||
| keyframe: 0.075 | ||
| - !type:KeyFramePropertyData | ||
| value: 270 | ||
| keyframe: 0.075 | ||
| - !type:KeyFramePropertyData | ||
| value: 0 | ||
| keyframe: 0.075 | ||
| - !type:KeyFramePropertyData | ||
| value: 90 | ||
| keyframe: 0.075 | ||
| - !type:KeyFramePropertyData | ||
| value: 180 | ||
| keyframe: 0.075 | ||
| - !type:KeyFramePropertyData | ||
| value: 270 | ||
| keyframe: 0.075 | ||
| - !type:KeyFramePropertyData | ||
| value: 0 | ||
| keyframe: 0.075 |
There was a problem hiding this comment.
все кейфреймы на 0.075 секунд
Если это относительно предыдущего кейфрейма - мб переименовать?
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@Content.Client/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs`:
- Around line 202-208: The null-check for _player.LocalSession is missing in the
Play(EntityUid uid, ProtoId<AnimationPrototype> animationId, Filter filter, bool
force = false) and Stop(EntityUid uid, ProtoId<AnimationPrototype> animationId,
Filter filter) methods: before calling
filter.Recipients.Contains(_player.LocalSession) add a guard to return early (or
skip the Contains call) when _player.LocalSession is null to avoid a possible
NRE; update both Play and Stop to check _player.LocalSession == null and only
call filter.Recipients.Contains(...) when it is non-null.
- Around line 210-211: PlayClient(EntityUid uid, ProtoId<AnimationPrototype>
animationId, bool force = false) is missing the prediction guard and can replay
animations during replays; add the same check used in the PlayClient overload
with recipient and in PlayPredicted: call _gameTiming.IsFirstTimePredicted and
return early (do not call Play) when it is false so the zero-recipient
PlayClient respects prediction and avoids duplicate plays.
- Around line 229-230: The Stop override should check HasRunningAnimation(uid,
animationkey) before calling _animationPlayer.Stop to match the existing pattern
used in Play (see Play at line ~175); modify the method so it only calls
_animationPlayer.Stop(uid, animationkey) when HasRunningAnimation(uid,
animationkey) returns true, preserving the current method signature and behavior
otherwise.
In `@Content.Server/_White/Animations/Commands/PlayAnimationCommand.cs`:
- Around line 18-26: The error message is incorrect when
shell.Player?.AttachedEntity is null: change the logic in the
PlayAnimationCommand handling (the args.Length == 1 branch that checks
shell.Player?.AttachedEntity) to log a clear "no attached entity / command must
be run by a player with an attached entity" message instead of using the
shell-wrong-arguments-number key; update the Loc.GetString key (e.g.,
shell-no-attached-entity) and call
shell.WriteError(Loc.GetString("shell-no-attached-entity")) (optionally still
call shell.WriteLine(Help)) so the error accurately reflects the missing
attached entity rather than a wrong argument count.
- Around line 40-41: The entity resolved via _entityManager.GetEntity(netEntity)
can be EntityUid.Invalid; before calling
_entityManager.System<WhiteAnimationPlayerSystem>().Play(entity, args[0]) check
that the resolved entity is valid (e.g. compare against EntityUid.Invalid or use
_entityManager.Exists(entity)) and bail out (or log) when invalid to avoid
sending a Play for a non-existent entity; update the code around the entity
variable and the Play call accordingly.
🧹 Nitpick comments (2)
Content.Server/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs (1)
48-48: Несогласованное именование параметра:animationkey→animationKey.В абстрактном классе
SharedWhiteAnimationPlayerSystemпараметр названanimationKey(camelCase), а здесь —animationkey. Хотя C# допускает это при переопределении, стоит привести к единому стилю.♻️ Предлагаемое исправление
- public override void Stop(EntityUid uid, string animationkey) + public override void Stop(EntityUid uid, string animationKey) { var filter = Filter.Pvs(uid, entityManager: EntityManager); - RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationkey), filter); + RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationKey), filter); } - public override void Stop(EntityUid uid, string animationkey, EntityUid recipient) + public override void Stop(EntityUid uid, string animationKey, EntityUid recipient) { if (!TryComp<ActorComponent>(recipient, out var actor)) return; - RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationkey), actor.PlayerSession); + RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationKey), actor.PlayerSession); } - public override void Stop(EntityUid uid, string animationkey, ICommonSession recipient) => - RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationkey), recipient); + public override void Stop(EntityUid uid, string animationKey, ICommonSession recipient) => + RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationKey), recipient); - public override void Stop(EntityUid uid, string animationkey, Filter filter) => - RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationkey), filter); + public override void Stop(EntityUid uid, string animationKey, Filter filter) => + RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationKey), filter); - public override void StopClient(EntityUid uid, string animationkey, EntityUid recipient) + public override void StopClient(EntityUid uid, string animationKey, EntityUid recipient) { // This is for the client } - public override void StopPredicted(EntityUid uid, string animationkey, EntityUid recipient) + public override void StopPredicted(EntityUid uid, string animationKey, EntityUid recipient) { var filter = Filter.PvsExcept(recipient, entityManager: EntityManager); - RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationkey), filter); + RaiseNetworkEvent(new StopAnimationMessage(GetNetEntity(uid), animationKey), filter); }Also applies to: 54-54, 62-62, 65-65, 68-68, 73-73
Content.Client/_White/Animations/Systems/WhiteAnimationPlayerSystem.cs (1)
136-148:GetPlaySoundиGetSpriteFlickмогут вернуть трек без ключевых кадров.Если ни один
keyFrameне соответствует ожидаемому типу (KeyFrameSoundData/KeyFrameSpriteFlickData), метод вернёт трек с пустым спискомKeyFrames. В отличие отGetComponentProperty, который возвращаетnullпри проблемах, эти методы всегда возвращают объект, который затем добавляется в анимацию. Стоит рассмотреть возвратnullпри пустыхKeyFramesили хотя бы логирование предупреждения.Also applies to: 150-162
RedFoxIV
left a comment
RedFoxIV
left a comment
There was a problem hiding this comment.
lgtm за исключением object? Animation в прототипе, см. комментарий из предыдущего ревью
8e5d669
into
WWhiteDreamProject:master
3 participants
Описание PR
Необходим для красивых частичек крови в ньюмеде.
ПР добавляет прототип анимаций. Он не требует обязательного проигрывания на клиенте и анимация автоматически кэшируется на клиенте.
По хорошему это следует перенести в движок, а лучше вообще переписать то как работают анимации.
FlipOnHit был заменен на AnimateOnHit.
AnimatedEmotes был удален и заменен на не щиткодную версию губов.
Добавлен AnimateOnStartup. Название говорит само за себя, да?
Изменения
🆑