-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Sync leviathans gameplay #2106
base: master
Are you sure you want to change the base?
Sync leviathans gameplay #2106
Conversation
Would require some tests :) |
1fcd2d9
to
3762c2c
Compare
There's still the following to be looked into (but most of the code is ready for review):
|
NitroxPatcher/Patches/Dynamic/Creature_ChooseBestAction_Patch.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Has a remote player inside | ||
return targetObject.GetComponentInChildren<RemotePlayerIdentifier>(true); |
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.
Is there a more performant way to check if a remote player is inside a cyclops? Like checking subroot id and if it's a cyclops? Traversing the complete cyclops seems kinda heavy depending on the algorithm. If it deep-first-search maybe we should implement a breadth-first-search, but that should be benchmarked beforehand.
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.
Two cases here:
- Remote player is in Cyclops' root
- Remote player is FAR in the hierarchy, parented to the driver seat
Current issue: remote player is never unparented from the driver seat even after leaving it (???) so this would need a fix in another PR I guess
Possible solutions (that would come in the same PR mentionned above):
- have a MB (either MultiplayerCyclops or a new one) store in a list all players (local and remote) which are currently in the cyclops, and use this list instead of the components search
- One loop in the first children layer, and one in the driver seat layer
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.
If it's only the two cases could we either check only all direct children of the cyclops and from the driver seat down?
float aggressiveToNoise = __instance.aggressiveToNoise.Value; | ||
|
||
Resolve<IPacketSender>().Send(new AttackCyclopsTargetChanged(creatureId, targetId, aggressiveToNoise)); | ||
ErrorMessage.AddMessage($"[SEND] {__instance.gameObject.name} attacks {__instance.currentTarget.name}"); |
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.
Pinning debug message so it's not forgotten.
NitroxPatcher/Patches/Dynamic/AttackCyclops_OnCollisionEnter_Patch.cs
Outdated
Show resolved
Hide resolved
NitroxPatcher/Patches/Dynamic/SeamothTorpedoWhirlpool_Register_Patch.cs
Outdated
Show resolved
Hide resolved
The latest Sea Dragons sync code wasn't tested because it's really hard without #2113 |
|
||
namespace NitroxServer.Communication.Packets.Processors; | ||
|
||
public class SeaDragonAttackTargetProcessor : TransmitIfCanSeePacketProcessor<SeaDragonAttackTarget> |
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.
Can be simplified using primary constructors
public class SeaDragonAttackTargetProcessor(
PlayerManager playerManager,
EntityRegistry entityRegistry
) : TransmitIfCanSeePacketProcessor<SeaDragonSwatAttack>(playerManager, entityRegistry)
I don't put comments on the others processors but same idea (SeaDragonSwatAttackProcessor, SeaDragonGrabExosuitProcessor, RangedAttackLastTargetUpdateProcessor, ...)
using NitroxServer.GameLogic; | ||
using NitroxServer.GameLogic.Entities; | ||
|
||
namespace NitroxServer.Communication.Packets.Processors; |
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.
Maybe we could regroup the different processors related to creature sync under a same folder ?
Thoughts on it ?
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.
We have many files that should better be moved inside theme named folders. I would say we move this project wide refactor after V1.8
/// <summary> | ||
/// Enables exosuits's position sync when they're released from sea dragons | ||
/// </summary> | ||
public sealed partial class SeaDrargon_ReleaseExosuit_Patch : NitroxPatch, IDynamicPatch |
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.
small typo Dragon
|
||
public static void Prefix(RangedAttackLastTarget __instance) | ||
{ | ||
if (!Resolve<AI>().IsCreatureWhitelisted(__instance.creature) || |
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 if
is really hard to read.It should be splitted imo. The game won't go slower for few more IL lines generated
|
||
public static void Prefix(RangedAttackLastTarget __instance) | ||
{ | ||
if (!Resolve<AI>().IsCreatureWhitelisted(__instance.creature) || |
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.
same here
c0598d1
to
862ae97
Compare
…th inventory not storing torpedos
… creature type whitelist
862ae97
to
bad5975
Compare
// TODO: Adapt this code when #1780 is merged | ||
Utils.PlayEnvSound(seaDragonMeleeAttack.attackSound, collider.transform.position, 20f); |
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.
#1780 is now merged :D
lastTarget.SetTargetInternal(targetObject); | ||
lastTarget.targetLocked = locked; | ||
|
||
if (aggressiveWhenSeeTarget.sightedSound != null && !aggressiveWhenSeeTarget.sightedSound.GetIsPlaying()) |
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.
Comparing MB against null
torpedo.tr.rotation = rotation; | ||
torpedo.OnHit(default); | ||
torpedo.Deactivate(); | ||
} |
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.
Is not heaving the component an expected case? Or should it be logged?
} | ||
|
||
// It should be set to inactive automatically in Bullet.Awake | ||
GameObject playerSphereClone = GameObject.Instantiate(stasisSpherePrefab); |
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.
Would it maybe make sense to name the game object accordingly with the playerid in it?
using NitroxServer.GameLogic; | ||
using NitroxServer.GameLogic.Entities; | ||
|
||
namespace NitroxServer.Communication.Packets.Processors; |
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.
We have many files that should better be moved inside theme named folders. I would say we move this project wide refactor after V1.8
namespace NitroxPatcher.Patches.Dynamic; | ||
|
||
/// <summary> | ||
/// Prevents non simulating players from running locally <see cref="RangedAttackLastTarget.StartCasting"/>. |
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.
/// Prevents non simulating players from running locally <see cref="RangedAttackLastTarget.StartCasting"/>. | |
/// Prevents non simulating players from running locally <see cref="RangedAttackLastTarget.StartCharging"/>. |
remotelyControlled.enabled = false; | ||
} | ||
|
||
if (__instance.TryGetNitroxId(out NitroxId seaDragonId) && exosuit.TryGetNitroxId(out NitroxId targetId)) |
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.
Should we check if the player has a lock on the SeaDragon?
// In this method, players are priority targets. This will also account for the local player case | ||
IEcoTarget ecoTarget = EcoRegionManager.main.FindNearestTarget(RemotePlayer.PLAYER_ECO_TARGET_TYPE, __instance.transform.position, __instance.isTargetValidFilter, __instance.maxSearchRings); | ||
if (ecoTarget != null && __instance.IsTargetValid(ecoTarget.GetGameObject())) | ||
{ | ||
__result = ecoTarget.GetGameObject(); | ||
return false; | ||
} | ||
|
||
// To redirect the call to base.GetAggressionTarget(), we ensure the if is skipped in the original method | ||
float timeLastPlayerAttack = __instance.timeLastPlayerAttack; | ||
__instance.timeLastPlayerAttack = Time.time; | ||
|
||
__result = __instance.GetAggressionTarget(); | ||
|
||
__instance.timeLastPlayerAttack = timeLastPlayerAttack; | ||
return false; |
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.
Will this now also target the local player? I don't know if the local player is included in RemotePlayer.PLAYER_ECO_TARGET_TYPE
so maybe this comment is pointless.
* BroadcastSeaDragonAttackRemotePlayer(this, target); <---- INSERTED LINE | ||
* if (component4 != null) |
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 seems a little to early, no? Broadcasting before all checks are done if the exosuit can even be grabed
return new CodeMatcher(instructions).End() | ||
.InsertAndAdvance([ | ||
new CodeInstruction(OpCodes.Ldarg_0), | ||
new CodeInstruction(OpCodes.Ldarg_1), | ||
new CodeInstruction(OpCodes.Ldarg_2), | ||
new CodeInstruction(OpCodes.Call, Reflect.Method(() => BroadcastSwatAttack(default, default, default))) | ||
]) | ||
.InstructionEnumeration(); | ||
} |
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.
Are you sure this works? won't this add the instructions after the ret?
Creature sync:
Creature behaviours:
Weapon sync:
Other related issues:
TODO when everything is done:
- [ ] Ensure crash fishes can target other playersPOSTPONED to another PR (will be short but required both this one and #2101 )