use external camera #419
use external camera #419
Conversation
Co-authored-by: Winston H. <[email protected]>
|
@winstxnhdw looks solid to me, i have been testing these past 3 hours and got this custom camera to work and preserve hud and the rest, enemies work properly when they go outside, and best of all spider now follows the controller . |
|
@winstxnhdw with the jester i had to add a new Event , OnPlayerCollision , because for some weird reason the jester wouldn't kill a player if outside, so i fixed that. EDIT : i checked the other enemies that are inside and replicated their collision system without a check to make sure they work |
|
There's still too many things I want to change. I don't like how |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- lc-hax/Scripts/Core/HaxCamera.cs (1 hunks)
- lc-hax/Scripts/Modules/PhantomMod.cs (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- lc-hax/Scripts/Core/HaxCamera.cs
- lc-hax/Scripts/Modules/PhantomMod.cs
winstxnhdw
force-pushed
the
main
branch
3 times, most recently
from
f1bfe19
to
1411cd1
Compare
|
@winstxnhdw just to let you know, im done collabing to your repo trying to follow your strict requirements. |
you never really ever did follow any. cheers. |
the issue is that you dont know how to accept diverse code styles, i understand readability , but how you do it is too much |
|
I won't argue with you further on this. You probably won't find another maintainer that would bother to refactor your code. All I hope is that you won't keep this attitude at a real job. Also, https://blog.codacy.com/coding-standards Have a nice week. |
winstxnhdw
force-pushed
the
main
branch
2 times, most recently
from
05080f6
to
1337f55
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
lc-hax/Scripts/Modules/Possession/Controllers/CircuitBeesController.cs (3)
13-13: Consider simplifying theGetCameraOffsetmethodThe
GetCameraOffsetmethod looks good, but it can be simplified. Since theRedLocustBeesparameter is unused, you could remove it and use an expression-bodied member for conciseness.Here's a suggested improvement:
- public Vector3 GetCameraOffset(RedLocustBees _) => this.CameraOffset; + public Vector3 GetCameraOffset() => CameraOffset;This change maintains the functionality while making the code more concise.
24-24: LGTM:OnOutsideStatusChangemethod addedThe
OnOutsideStatusChangemethod is a good addition for handling changes in the enemy's outside status. It appropriately stops the search for a hive when the status changes.For improved readability, consider extracting the boolean parameter to a named constant:
- public void OnOutsideStatusChange(RedLocustBees enemy) => enemy.StopSearch(enemy.searchForHive, true); + public void OnOutsideStatusChange(RedLocustBees enemy) + { + const bool stopImmediately = true; + enemy.StopSearch(enemy.searchForHive, stopImmediately); + }This change makes the intent of the
trueparameter more clear.
Potential Issues Detected: Residual
InteractRangeReferencesThe
InteractRangemethod remains referenced in multiple files:
lc-hax/Scripts/Modules/Possession/PossessionMod.cslc-hax/Scripts/Modules/Possession/Controllers/IEnemyController.cslc-hax/Scripts/Modules/Possession/Controllers/ForestGiantController.cslc-hax/Scripts/Modules/Possession/Controllers/EarthLeviathanController.csPlease review these references to ensure that the removal of
InteractRangeinCircuitBeesController.csdoes not introduce inconsistencies or bugs elsewhere in the codebase.🔗 Analysis chain
Line range hint
1-24: Overall changes look good, but clarification needed onInteractRangemethodThe additions to the
CircuitBeesControllerclass improve its functionality by adding camera offset handling and outside status change management. These changes align well with the PR objectives related to camera functionality and enemy behavior.However, the AI summary mentioned the removal of the
InteractRangemethod, which is not visible in the provided code. To ensure we're not missing any important changes, please run the following script to verify the removal of this method:If the method has indeed been removed, please confirm if this change affects the enemy's interaction capabilities and if any adjustments are needed in other parts of the codebase.
Further Verification Required:
InteractRangeMethod RemovalThe initial verification did not locate the
InteractRangemethod inCircuitBeesController.cs. To ensure complete removal and identify any remaining references across the codebase, please execute the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of the InteractRange method # Test: Search for the InteractRange method in the current and previous versions of the file git show HEAD:lc-hax/Scripts/Modules/Possession/Controllers/CircuitBeesController.cs | grep -n "InteractRange" git show HEAD^:lc-hax/Scripts/Modules/Possession/Controllers/CircuitBeesController.cs | grep -n "InteractRange"Length of output: 224
Script:
#!/bin/bash # Description: Search the entire codebase for any references to the InteractRange method rg 'InteractRange' --type csLength of output: 1060
lc-hax/Scripts/Components/CharacterMovement.cs (3)
Line range hint
69-90: Potential logic issue in sprint togglingIn the
Movemethod, when the sprint key is not pressed (lines 81-86),IsSprintingis toggled usingthis.IsSprinting = !this.IsSprinting;. This will causeIsSprintingto change state every time the sprint key is released, which might not be the intended behavior. Typically, when the sprint key is released, sprinting should stop.Consider updating the logic to explicitly set
IsSprintingtofalsewhen the sprint key is not pressed.Apply this diff to fix the issue:
else { this.IsSprintHeld = false; - this.IsSprinting = !this.IsSprinting; + this.IsSprinting = false; }
98-100: Clarify the naming ofCharacterSprintSpeedIn line 99,
CharacterSprintSpeedis used as a multiplier toCharacterSpeedwhen sprinting:float sprintModifier = this.IsSprinting ? this.CharacterSpeed * this.CharacterSprintSpeed : this.CharacterSpeed;Since it's acting as a multiplier rather than an absolute speed, consider renaming
CharacterSprintSpeedtoCharacterSprintMultiplierfor clarity.
23-24: InitializeKeyboardinAwaketo avoid potential issuesInitializing the
Keyboardproperty directly withKeyboard.currentmight lead to issues if the input system is not yet initialized when the class is constructed. To ensure the keyboard is initialized properly, consider setting it in theAwakemethod.Apply this diff to adjust the initialization:
-Keyboard Keyboard { get; } = Keyboard.current; +Keyboard Keyboard { get; set; } void Awake() { + this.Keyboard = Keyboard.current; this.NoClipKeyboard = this.gameObject.AddComponent<KeyboardMovement>(); this.CharacterController = this.gameObject.AddComponent<CharacterController>(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- lc-hax/Scripts/Components/CharacterMovement.cs (3 hunks)
- lc-hax/Scripts/Loader.cs (1 hunks)
- lc-hax/Scripts/Mixins/IEnemyPrompter.cs (7 hunks)
- lc-hax/Scripts/Modules/Possession/Controllers/CircuitBeesController.cs (2 hunks)
- lc-hax/Scripts/Modules/Possession/PossessionMod.cs (1 hunks)
🔇 Additional comments (3)
lc-hax/Scripts/Modules/Possession/Controllers/CircuitBeesController.cs (1)
11-11: LGTM: Camera offset property addedThe
CameraOffsetproperty is a good addition for defining the camera position relative to the controlled enemy. The read-only nature ensures that the offset cannot be modified externally, maintaining encapsulation.lc-hax/Scripts/Loader.cs (1)
65-65: LGTM! Verify HaxCamera initialization and compatibility.The addition of
HaxCamerato theLoadHaxGameObjectsmethod is consistent with the existing pattern and aligns with the PR objectives. This change should enable the new camera system for phantom mode as intended.To ensure proper integration, please verify:
- HaxCamera doesn't require any specific initialization beyond what
AddHaxGameObjectprovides.- HaxCamera is compatible with other loaded components, especially any existing camera systems.
You can run the following script to check for any potential conflicts or dependencies:
lc-hax/Scripts/Components/CharacterMovement.cs (1)
8-9: Review theSprintDurationvalueThe
SprintDurationis set to0.0f, which means sprinting will activate immediately upon pressing the sprint key without any delay. If this is intentional, then it's acceptable. However, if you intended to have a delay before sprinting starts, consider settingSprintDurationto a positive value.
| void Update() { | ||
| if (this.NoClipKeyboard is { enabled: true }) return; | ||
| if (this.CharacterController is { enabled: false }) return; | ||
|
|
There was a problem hiding this comment.
Prevent potential null reference exceptions
In the Update method, you check if NoClipKeyboard is enabled or if CharacterController is disabled. However, if CharacterController is null, accessing CharacterController.enabled could throw a null reference exception.
Consider adding a null check for CharacterController before accessing its enabled property:
if (this.NoClipKeyboard is { enabled: true }) return;
-if (this.CharacterController is { enabled: false }) return;
+if (this.CharacterController == null || !this.CharacterController.enabled) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Update() { | |
| if (this.NoClipKeyboard is { enabled: true }) return; | |
| if (this.CharacterController is { enabled: false }) return; | |
| void Update() { | |
| if (this.NoClipKeyboard is { enabled: true }) return; | |
| if (this.CharacterController == null || !this.CharacterController.enabled) return; | |
| float speedModifier = this.Keyboard.leftCtrlKey.isPressed ? CharacterMovement.WalkingSpeed : 1.0f; | ||
| float sprintModifier = this.IsSprinting ? this.CharacterSpeed * this.CharacterSprintSpeed : this.CharacterSpeed; | ||
|
|
||
| return sprintModifier * ( | ||
| (this.transform.forward * moveInput.y) + | ||
| (this.transform.right * moveInput.x * speedModifier) |
There was a problem hiding this comment.
Apply speedModifier consistently to movement
Currently, speedModifier is applied only to the horizontal movement (line 103) but not to the forward/backward movement (line 102). This could lead to inconsistent movement speeds when moving diagonally, as holding the left control key will slow horizontal movement but not forward movement.
Consider applying speedModifier to both movement axes to ensure consistent speed adjustments.
Apply this diff to adjust the movement calculation:
return sprintModifier * (
- (this.transform.forward * moveInput.y) +
- (this.transform.right * moveInput.x * speedModifier)
+ (this.transform.forward * moveInput.y * speedModifier) +
+ (this.transform.right * moveInput.x * speedModifier)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| float speedModifier = this.Keyboard.leftCtrlKey.isPressed ? CharacterMovement.WalkingSpeed : 1.0f; | |
| float sprintModifier = this.IsSprinting ? this.CharacterSpeed * this.CharacterSprintSpeed : this.CharacterSpeed; | |
| return sprintModifier * ( | |
| (this.transform.forward * moveInput.y) + | |
| (this.transform.right * moveInput.x * speedModifier) | |
| float speedModifier = this.Keyboard.leftCtrlKey.isPressed ? CharacterMovement.WalkingSpeed : 1.0f; | |
| float sprintModifier = this.IsSprinting ? this.CharacterSpeed * this.CharacterSprintSpeed : this.CharacterSpeed; | |
| return sprintModifier * ( | |
| (this.transform.forward * moveInput.y * speedModifier) + | |
| (this.transform.right * moveInput.x * speedModifier) | |
| ); |
| @@ -124,13 +131,16 @@ void HandleNutcracker(NutcrackerEnemyAI nutcracker, PlayerControllerB targetPlay | |||
|
|
|||
| void HandleMaskedPlayer(MaskedPlayerEnemy maskedPlayer, PlayerControllerB targetPlayer, bool willTeleportEnemy) { | |||
| this.TeleportEnemyToPlayer(maskedPlayer, targetPlayer, willTeleportEnemy, true, true); | |||
| maskedPlayer.SetBehaviourState(BehaviourState.CHASE); | |||
| maskedPlayer.SwitchToBehaviourServerRpc(1); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid magic numbers by using named constants or enums.
In the HandleMaskedPlayer method, the call to maskedPlayer.SwitchToBehaviourServerRpc(1); uses the hard-coded value 1. To enhance code clarity and maintainability, replace the magic number with a named constant or enum value.
Update the code as follows:
- maskedPlayer.SwitchToBehaviourServerRpc(1);
+ maskedPlayer.SwitchToBehaviourServerRpc(MaskedPlayerState.Chase);Ensure that the MaskedPlayerState enum is defined and includes the Chase value.
Committable suggestion was skipped due to low confidence.
|
|
||
| void HandleDocileLocustBees(DocileLocustBeesAI docileLocustBees, PlayerControllerB targetPlayer, bool willTeleportEnemy) { | ||
| this.TeleportEnemyToPlayer(docileLocustBees, targetPlayer, willTeleportEnemy, true); | ||
| docileLocustBees.SetBehaviourState(BehaviourState.IDLE); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align behavior state with enemy-specific enums for clarity.
In the HandleDocileLocustBees method, you're setting the behavior state using BehaviourState.IDLE. To improve clarity and maintain consistency with other enemy handlers, consider using an enemy-specific enum, such as DocileLocustBeesState, if it exists.
Update the code if an enemy-specific enum is available:
- docileLocustBees.SetBehaviourState(BehaviourState.IDLE);
+ docileLocustBees.SetBehaviourState(DocileLocustBeesState.Idle);This ensures that the behavior states are explicitly tied to their respective enemy types.
Committable suggestion was skipped due to low confidence.
| if (willTeleportEnemy) { | ||
| bunkerSpider.meshContainerPosition = targetPlayer.transform.position; | ||
| bunkerSpider.SyncMeshContainerPositionToClients(); | ||
| } | ||
| bunkerSpider.SwitchToBehaviourServerRpc(2); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace magic numbers with meaningful constants or enums for clarity.
In the HandleBunkerSpider method, the call to bunkerSpider.SwitchToBehaviourServerRpc(2); uses a hard-coded value 2. Using magic numbers can make the code less readable and more challenging to maintain. Consider defining an enum or constant to represent the behavior state.
Apply this diff to improve readability:
- bunkerSpider.SwitchToBehaviourServerRpc(2);
+ bunkerSpider.SwitchToBehaviourServerRpc(BunkerSpiderState.Chase);Ensure that the BunkerSpiderState enum is defined and includes the Chase value.
Committable suggestion was skipped due to low confidence.
| this.TeleportEnemyToPlayer(dressGirl, targetPlayer, willTeleportEnemy, true); | ||
| dressGirl.hauntingPlayer = targetPlayer; | ||
| dressGirl.ChangeEnemyOwnerServerRpc(targetPlayer.playerClientId); | ||
| dressGirl.SetBehaviourState(BehaviourState.IDLE); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific enemy behavior states for consistency.
In the HandleDressGirl method, you're setting the behavior state using the general BehaviourState.IDLE. For consistency with other enemy handlers that use enemy-specific enums (e.g., GiantState.CHASE), consider using a DressGirlState enum if available.
If a DressGirlState enum exists, update the code:
- dressGirl.SetBehaviourState(BehaviourState.IDLE);
+ dressGirl.SetBehaviourState(DressGirlState.Idle);This enhances readability and maintains consistency across enemy behavior management.
Committable suggestion was skipped due to low confidence.
| void UsePrimarySkill() { | ||
| if (this.Possession.Enemy is not EnemyAI enemy || this.Controller is null) return; | ||
| this.Controller.UsePrimarySkill(enemy); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated null checks into a helper method
The method UsePrimarySkill and other similar methods repeat null checks for Possession.Enemy and Controller. To reduce code duplication and enhance maintainability, consider creating a helper method for these checks.
Apply this diff to UsePrimarySkill:
- void UsePrimarySkill() {
- if (this.Possession.Enemy is not EnemyAI enemy || this.Controller is null) return;
- this.Controller.UsePrimarySkill(enemy);
- }
+ void UsePrimarySkill() {
+ if (this.TryGetEnemyAndController(out var enemy, out var controller))
+ controller.UsePrimarySkill(enemy);
+ }Add the helper method outside the selected line ranges:
private bool TryGetEnemyAndController(out EnemyAI enemy, out IController controller)
{
enemy = null;
controller = null;
if (this.Possession.Enemy is not EnemyAI e || this.Controller is null)
{
return false;
}
enemy = e;
controller = this.Controller;
return true;
}Update the other methods (UseSecondarySkill, OnSecondarySkillHold, ReleaseSecondarySkill, UseSpecialAbility) similarly to utilize the helper method.
| if (this.Possession.Enemy is not EnemyAI enemy) return; | ||
| if (enemy.agent is not NavMeshAgent agent) return; | ||
|
|
||
| enemy.ChangeEnemyOwnerServerRpc(localPlayer.actualClientId); |
There was a problem hiding this comment.
Optimize ownership change to prevent unnecessary network traffic
Calling ChangeEnemyOwnerServerRpc every frame in the Update method can lead to unnecessary network usage and performance issues. Consider checking if the enemy's owner has changed before invoking this method.
Apply this diff to optimize the code:
- enemy.ChangeEnemyOwnerServerRpc(localPlayer.actualClientId);
+ if (enemy.OwnerClientId != localPlayer.actualClientId) {
+ enemy.ChangeEnemyOwnerServerRpc(localPlayer.actualClientId);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enemy.ChangeEnemyOwnerServerRpc(localPlayer.actualClientId); | |
| if (enemy.OwnerClientId != localPlayer.actualClientId) { | |
| enemy.ChangeEnemyOwnerServerRpc(localPlayer.actualClientId); | |
| } |
| Transform? GetExitPointFromDoor(EntranceTeleport entrance) => | ||
| Helper.FindObjects<EntranceTeleport>().First(teleport => | ||
| teleport.entranceId == entrance.entranceId && teleport.isEntranceToBuilding != entrance.isEntranceToBuilding | ||
| )?.entrancePoint; |
There was a problem hiding this comment.
Prevent potential InvalidOperationException by using FirstOrDefault
Using .First() may throw an InvalidOperationException if no matching teleport is found. Replace .First() with .FirstOrDefault() to safely return null when no exit point exists.
Apply this diff to fix the issue:
- Helper.FindObjects<EntranceTeleport>().First(teleport =>
+ Helper.FindObjects<EntranceTeleport>().FirstOrDefault(teleport =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Transform? GetExitPointFromDoor(EntranceTeleport entrance) => | |
| Helper.FindObjects<EntranceTeleport>().First(teleport => | |
| teleport.entranceId == entrance.entranceId && teleport.isEntranceToBuilding != entrance.isEntranceToBuilding | |
| )?.entrancePoint; | |
| Transform? GetExitPointFromDoor(EntranceTeleport entrance) => | |
| Helper.FindObjects<EntranceTeleport>().FirstOrDefault(teleport => | |
| teleport.entranceId == entrance.entranceId && teleport.isEntranceToBuilding != entrance.isEntranceToBuilding | |
| )?.entrancePoint; |
winstxnhdw
force-pushed
the
main
branch
4 times, most recently
from
8bc96c0
to
d0d120f
Compare
winstxnhdw
force-pushed
the
main
branch
3 times, most recently
from
b14871b
to
c01417a
Compare
Fix Enemies Nodes not updating
Fix spider mesh not following the current position & rotation
when not controlling, the spider will use that last position and claim it as it's nest.
add LcHaxCamera and make all mods use that one on phantom mode.
add isDead()
add OnOutsideStatusChange to reset all enemies's NavMesh's searches to not be broken when AI Nodes update.
@winstxnhdw all done.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores