feat: weapon improvements and feedback#82
Open
lmProgramming wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a shooting event system ( ChangesShooting Events, Sound & Module Lifecycle
Sequence Diagram(s)sequenceDiagram
participant Cannon
participant ShootingEventChannel
rect rgba(70, 130, 180, 0.5)
Note over Cannon: Shoot() — per spawn point
Cannon->>Cannon: AverageOfVectors(projectileSpawnPoints)
Cannon->>Cannon: Instantiate projectile, set velocity + impulse
Cannon->>ShootingEventChannel: RaiseEvent(BulletShootingData)
end
rect rgba(180, 100, 70, 0.5)
Note over ShootingEventChannel: Listeners react
ShootingEventChannel->>ShootingEffectSpawner: HandleCollision(BulletShootingData)
ShootingEffectSpawner->>ShootingEffectSpawner: Tween.ShakeCamera(sqrt(Momentum)/100)
ShootingEventChannel->>ShootingSoundPlayer: HandleShooting(BulletShootingData)
ShootingSoundPlayer->>ISoundManager: Play(BulletShooting, point)
ISoundManager->>ISoundManager: Distance check via GetEffectiveDistances
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/Scripts/Ships/Ship.cs (1)
229-239:⚠️ Potential issue | 🟡 MinorUse
module.Transforminstead ofmodule.transformfor consistency.Line 236 uses lowercase
module.transform, but the established pattern in this file (lines 203, 219) uses capital-Tmodule.Transformto access the property via theIModuleinterface. Consistency matters for maintainability.🔧 Proposed fix
foreach (var module in existingModules) { module.OnShipConnectionLost(); - module.transform.SetParent(null, true); + module.Transform.SetParent(null, true); Destroy(module.gameObject); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Scripts/Ships/Ship.cs` around lines 229 - 239, The DestroyAllModules method uses lowercase module.transform on line 236, but the established pattern in this file uses capital-T module.Transform to access the property via the IModule interface (as seen in lines 203 and 219). Change module.transform.SetParent(null, true) to module.Transform.SetParent(null, true) to maintain consistency throughout the file.Source: Coding guidelines
🧹 Nitpick comments (3)
Assets/Scripts/Ships/Modules/Module.cs (1)
252-253: 💤 Low valueRedundant null check pattern.
Line 252 returns early if
Ship == null, making the null-conditional operator?.on line 253 unnecessary. Use direct member accessShip.OnModuleConnectionLost(this)instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Scripts/Ships/Modules/Module.cs` around lines 252 - 253, The early return on line 252 when Ship is null makes the null-conditional operator on line 253 redundant. Since execution only reaches line 253 when Ship is not null, replace the null-conditional operator `?.` with a direct member access call to OnModuleConnectionLost on the Ship object. Change `Ship?.OnModuleConnectionLost(this)` to `Ship.OnModuleConnectionLost(this)` to remove the unnecessary defensive null check.Assets/Scripts/Events/Gameplay/Shooting/ShootingData.cs (1)
1-21: 💤 Low valueConsider using properties instead of public fields for the data contract.
The class uses public fields (
point,direction,InstigatorShip) which is common in Unity for serialization, but this appears to be a runtime event data transfer object. Using properties with{ get; set; }or{ get; init; }would better align with C# conventions for data contracts and prevent accidental mutation if immutability is desired.Additionally,
InstigatorShipuses PascalCase which is typically reserved for properties. If keeping fields, consider using camelCase or converting to properties.♻️ Proposed refactor using properties
[Serializable] public abstract class ShootingData { - public Vector2 point; - public Vector2 direction; - public IShip InstigatorShip; + public Vector2 Point { get; } + public Vector2 Direction { get; } + public IShip InstigatorShip { get; } public ShootingData(IShip instigatorShip, Vector2 point, Vector2 direction) { InstigatorShip = instigatorShip; - this.point = point; - this.direction = direction; + Point = point; + Direction = direction; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Scripts/Events/Gameplay/Shooting/ShootingData.cs` around lines 1 - 21, Convert the public fields in the ShootingData class (point, direction, and InstigatorShip) from fields to properties using either { get; set; } or { get; init; } accessors. This aligns with C# conventions for data contracts and improves encapsulation. If using auto-properties, you can initialize them directly in the property declaration or continue using the constructor to set the backing values. The PascalCase naming of InstigatorShip is appropriate for a property, so this refactor also resolves the naming convention inconsistency between it and the other fields.Assets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.cs (1)
36-36: 💤 Low valueExtract magic numbers or document the shake strength formula.
The formula
Mathf.Sqrt(bulletData.Momentum) / 100uses unexplained magic numbers. Consider extracting the scaling factor to a named constant or adding a brief comment explaining the sqrt and division rationale.♻️ Proposed refactor with named constant
+private const float ShakeStrengthScale = 100f; + private void HandleCollision(ShootingData data) { if (data is BulletShootingData bulletData) Tween.ShakeCamera( _camera, duration: 0.1f, - strengthFactor: Mathf.Sqrt(bulletData.Momentum) / 100 + strengthFactor: Mathf.Sqrt(bulletData.Momentum) / ShakeStrengthScale ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.cs` at line 36, The strengthFactor calculation using Mathf.Sqrt(bulletData.Momentum) / 100 contains a magic number (100) that lacks explanation. Extract the divisor value 100 to a named constant at the class level (for example, something like SHAKE_STRENGTH_DIVISOR) and use that constant in the formula, or alternatively add a clear comment above or inline explaining why the square root is taken and why division by 100 is applied for the shake strength scaling. This will make the intent and rationale of the formula transparent to future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/Scripts/Gameplay/Collision/CollisionEffectSpawner.cs`:
- Around line 46-48: Remove the two Debug.Log statements that log
data.pixelsDestroyed.Length and strengthFactor in the CollisionEffectSpawner.cs
file. These console logs will spam the output during gameplay and impact
performance. If you need these logs for development purposes, wrap them in
conditional compilation directives (such as `#if` DEBUG) so they only appear in
development builds, not in production.
- Around line 16-19: The Awake() method assigns Camera.main to the _camera field
without validating that it is not null. If no camera is tagged as MainCamera at
startup, _camera will remain null and cause a NullReferenceException when
Tween.ShakeCamera attempts to use it later. Add a null check immediately after
the assignment in Awake() that validates _camera is not null, and if it is null,
throw an exception with a clear error message stating that a camera tagged as
MainCamera is required for CollisionEffectSpawner to function.
In `@Assets/Scripts/Gameplay/Collision/CollisionSoundPlayer.cs`:
- Line 30: The Debug.Log statement logging data.SpeedDifference.Value.magnitude
in the CollisionSoundPlayer.cs file is executed on every collision, causing
console spam and performance degradation. Remove this Debug.Log statement
entirely, or if it is needed for development debugging, wrap it in a conditional
compilation directive such as `#if` DEBUG to ensure it only executes in
development builds and not in production.
In `@Assets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.cs`:
- Line 12: The _effectsSpawner field is declared with the [Inject] attribute but
is never referenced or used anywhere in the ShootingEffectSpawner class. Remove
the unused injected dependency by deleting the entire line containing the
[Inject] private IEffectsSpawner _effectsSpawner declaration to clean up the
code and eliminate unnecessary injection overhead.
- Around line 15-18: The Awake() method in ShootingEffectSpawner does not
validate that Camera.main successfully returned a camera object. Add a null
check after assigning Camera.main to _camera, and if it is null, fail hard by
logging an error and throwing an exception or calling an error method (such as
Debug.LogError followed by an assertion or explicit error handling) to prevent
silent failures when no camera is tagged "MainCamera".
- Line 30: The method HandleCollision in the ShootingEffectSpawner class is
misnamed since it handles shooting events rather than collision events. Rename
the HandleCollision method to a descriptive name like HandleShooting or
HandleShootingEvent that accurately reflects its actual purpose of processing
shooting events. Then update all registration calls and event subscriptions
throughout the class that reference the old HandleCollision method name to use
the new method name instead.
In `@Assets/Scripts/LMPro/MathExt.cs`:
- Around line 217-222: The method AverageOfVectors has two issues: it lacks
validation for null or empty arrays which will cause a DivideByZeroException on
line 221 when dividing by transforms.Length, and the parameter name transforms
is misleading since it represents Vector3 objects not transforms. Add a
null/empty check at the start of the method to guard against invalid input
(consider throwing an ArgumentException with a descriptive message or returning
a default value), and rename the parameter from transforms to vectors throughout
the method to accurately reflect its purpose.
- Around line 210-215: The AverageOfVectors method contains an incorrect
property access in the Aggregate lambda expression. The parameter v is already a
Transform object from the transforms array, so accessing v.transform.position is
wrong because Transform does not have a transform property. Change
v.transform.position to v.position in the Aggregate function to directly access
the position property of the Transform object.
- Around line 210-215: The AverageOfVectors method lacks null or empty
validation for the transforms parameter, which causes a DivideByZeroException
when transforms.Length is 0 on line 214. Add a validation check at the beginning
of the AverageOfVectors method to throw an appropriate exception with a
descriptive error message if transforms is null or empty, ensuring the method
fails hard with an error as per coding guidelines.
In `@Assets/Scripts/Services/Sound/SoundManager.cs`:
- Line 38: The assignment to _camera from UnityEngine.Camera.main lacks a null
check, which can cause a NullReferenceException when accessing
_camera.transform.position during spatial culling if no camera is tagged as
"MainCamera". Add a null check immediately after the assignment to _camera, and
if Camera.main returns null, log an error message and fail hard (such as
throwing an exception or calling a fail method) to prevent silent failures and
make the issue immediately apparent during development.
- Around line 179-187: The GetEffectiveDistances method contains hardcoded magic
numbers (1000f, 10000f, and 10f) that lack clarity and maintainability. Extract
these values to named constants at the class level in SoundManager.cs: one
constant for the default minimum distance value (1000f), one for the default
maximum distance value (10000f), and one for the distance ratio divisor (10f).
Then replace the hardcoded values in the GetEffectiveDistances method with
references to these named constants to improve code readability and make future
adjustments easier.
In `@Assets/Scripts/Ships/Modules/Cannon.cs`:
- Around line 87-109: The bullet velocity calculation in the projectile spawning
loop is applying the ship's velocity twice: once when directly setting
bulletRigidbody.linearVelocity and again when calling AddForce with
ForceMode2D.Impulse. Remove the redundant velocity application by either: (1)
removing the line that sets bulletRigidbody.linearVelocity to just the ship
velocity and instead combine it with the direction calculation, or (2) keeping
the linearVelocity assignment and removing
PixelatedRigidbody.Rigidbody.linearVelocity from the AddForce call so only the
directional impulse is applied. Choose the approach that matches your intended
projectile trajectory behavior.
In `@Assets/Scripts/Ships/Modules/Module.cs`:
- Around line 247-257: The Destroy(this) call in the OnShipConnectionLost()
method is executed too early, causing the subsequent Ship notification and
DetachAllConnections() call to occur on an already-destroyed component. Move the
Destroy(this) call to the end of the OnShipConnectionLost() method so that
Ship?.OnModuleConnectionLost(this), Ship = null, and DetachAllConnections() are
all executed before the component is destroyed. Additionally, remove the
duplicate DetachAllConnections() call from the OnDestroy() method since it will
now be properly called by OnShipConnectionLost() before destruction occurs.
- Line 108: The Collider2D property forwards to PixelatedRigidbody.Collider2D
without null checking PixelatedRigidbody first, which can cause a
NullReferenceException if accessed before Awake() runs or if the component is
not found. Add a null check in the Collider2D property getter to validate that
PixelatedRigidbody is not null before accessing its Collider2D property, either
by logging an error and returning null or by throwing an exception with a clear
error message per your coding guidelines.
In `@Assets/Scripts/Ships/PlayerShip.cs`:
- Around line 19-23: In the PlayerShip class Start method, replace the
null-forgiving operator `!` on `module.Transform` with an explicit null check.
Instead of silently suppressing the nullable warning with
`module.Transform!.tag`, add a guard clause or conditional check that throws or
logs an error if module.Transform is null, ensuring the code fails hard with a
clear error message rather than risking a NullReferenceException at runtime.
---
Outside diff comments:
In `@Assets/Scripts/Ships/Ship.cs`:
- Around line 229-239: The DestroyAllModules method uses lowercase
module.transform on line 236, but the established pattern in this file uses
capital-T module.Transform to access the property via the IModule interface (as
seen in lines 203 and 219). Change module.transform.SetParent(null, true) to
module.Transform.SetParent(null, true) to maintain consistency throughout the
file.
---
Nitpick comments:
In `@Assets/Scripts/Events/Gameplay/Shooting/ShootingData.cs`:
- Around line 1-21: Convert the public fields in the ShootingData class (point,
direction, and InstigatorShip) from fields to properties using either { get;
set; } or { get; init; } accessors. This aligns with C# conventions for data
contracts and improves encapsulation. If using auto-properties, you can
initialize them directly in the property declaration or continue using the
constructor to set the backing values. The PascalCase naming of InstigatorShip
is appropriate for a property, so this refactor also resolves the naming
convention inconsistency between it and the other fields.
In `@Assets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.cs`:
- Line 36: The strengthFactor calculation using Mathf.Sqrt(bulletData.Momentum)
/ 100 contains a magic number (100) that lacks explanation. Extract the divisor
value 100 to a named constant at the class level (for example, something like
SHAKE_STRENGTH_DIVISOR) and use that constant in the formula, or alternatively
add a clear comment above or inline explaining why the square root is taken and
why division by 100 is applied for the shake strength scaling. This will make
the intent and rationale of the formula transparent to future maintainers.
In `@Assets/Scripts/Ships/Modules/Module.cs`:
- Around line 252-253: The early return on line 252 when Ship is null makes the
null-conditional operator on line 253 redundant. Since execution only reaches
line 253 when Ship is not null, replace the null-conditional operator `?.` with
a direct member access call to OnModuleConnectionLost on the Ship object. Change
`Ship?.OnModuleConnectionLost(this)` to `Ship.OnModuleConnectionLost(this)` to
remove the unnecessary defensive null check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a3a6e963-f55c-47c6-902b-55be53385e08
⛔ Files ignored due to path filters (12)
Assets/Channels/CollisionEventChannel.assetis excluded by!**/*.assetAssets/Channels/ShootingEventChannel.assetis excluded by!**/*.assetAssets/Prefabs/Gameplay/Modules/Weapons/player_cannon_large_32x32.prefabis excluded by!**/*.prefabAssets/Prefabs/Gameplay/Modules/Weapons/player_cannon_medium_24x24.prefabis excluded by!**/*.prefabAssets/Prefabs/Gameplay/Modules/Weapons/player_cannon_small_1_16x16.prefabis excluded by!**/*.prefabAssets/Prefabs/Gameplay/Modules/Weapons/player_cannon_small_2_16x16.prefabis excluded by!**/*.prefabAssets/Prefabs/Scene/Game.prefabis excluded by!**/*.prefabAssets/Resources/ProjectContext.prefabis excluded by!**/*.prefabAssets/Scenes/MainGame.unityis excluded by!**/*.unityAssets/Scenes/ShipFactory.unityis excluded by!**/*.unityAssets/Sounds/cannon-fire.mp3is excluded by!**/*.mp3ProjectSettings/AudioManager.assetis excluded by!**/*.asset
📒 Files selected for processing (49)
Assets/Channels/CollisionEventChannel.asset.metaAssets/Channels/ShootingEventChannel.asset.metaAssets/Scripts/Context/GameProjectInstaller.csAssets/Scripts/Context/GameSceneInstaller.csAssets/Scripts/Core/Constants/GameplayConstants.csAssets/Scripts/Core/Gameplay/Sound/SoundIdentifier.csAssets/Scripts/Core/Pixelation/IPixelatedRigidbody.csAssets/Scripts/Core/Ship/IModule.csAssets/Scripts/Core/Ship/IShip.csAssets/Scripts/E2E/E2ETestBase.csAssets/Scripts/Events/Gameplay.metaAssets/Scripts/Events/Gameplay/Collision.metaAssets/Scripts/Events/Gameplay/Collision/CollisionData.csAssets/Scripts/Events/Gameplay/Collision/CollisionData.cs.metaAssets/Scripts/Events/Gameplay/Collision/CollisionEventChannelSO.csAssets/Scripts/Events/Gameplay/Collision/CollisionEventChannelSO.cs.metaAssets/Scripts/Events/Gameplay/Ship.metaAssets/Scripts/Events/Gameplay/Ship/ShipInitializeModulesEventChannel.csAssets/Scripts/Events/Gameplay/Ship/ShipInitializeModulesEventChannel.cs.metaAssets/Scripts/Events/Gameplay/Shooting.metaAssets/Scripts/Events/Gameplay/Shooting/BulletShootingData.csAssets/Scripts/Events/Gameplay/Shooting/BulletShootingData.cs.metaAssets/Scripts/Events/Gameplay/Shooting/ShootingData.csAssets/Scripts/Events/Gameplay/Shooting/ShootingData.cs.metaAssets/Scripts/Events/Gameplay/Shooting/ShootingEventChannel.csAssets/Scripts/Events/Gameplay/Shooting/ShootingEventChannel.cs.metaAssets/Scripts/Gameplay/Collision/CollisionEffectSpawner.csAssets/Scripts/Gameplay/Collision/CollisionSoundPlayer.csAssets/Scripts/Gameplay/Gameplay.asmdefAssets/Scripts/Gameplay/Shooting.metaAssets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.csAssets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.cs.metaAssets/Scripts/Gameplay/Shooting/ShootingSoundPlayer.csAssets/Scripts/Gameplay/Shooting/ShootingSoundPlayer.cs.metaAssets/Scripts/LMPro/MathExt.csAssets/Scripts/Pixelation/PixelCollisionHandler.csAssets/Scripts/Pixelation/PixelatedRigidbody.csAssets/Scripts/Services/ShipService.csAssets/Scripts/Services/Sound/Sound.csAssets/Scripts/Services/Sound/Sound.cs.metaAssets/Scripts/Services/Sound/SoundManager.csAssets/Scripts/Ships/Modules/Cannon.csAssets/Scripts/Ships/Modules/Laser.csAssets/Scripts/Ships/Modules/Module.csAssets/Scripts/Ships/PlayerShip.csAssets/Scripts/Ships/Ship.csAssets/Scripts/Ships/ShipInstaller.csAssets/Scripts/Ships/Tests/TestHelpers/Fixtures/TestContainerFactory.csAssets/Sounds/cannon-fire.mp3.meta
💤 Files with no reviewable changes (1)
- Assets/Scripts/Services/ShipService.cs
Comment on lines
+16
to
+19
| private void Awake() | ||
| { | ||
| _camera = Camera.main; | ||
| } |
There was a problem hiding this comment.
Fail hard if Camera.main is null.
Camera.main is cached without validation. If no camera is tagged as MainCamera at startup, _camera will be null and the Tween.ShakeCamera call at Line 50 will throw a NullReferenceException. As per coding guidelines, components necessary for the script should fail hard with a clear error message.
🛡️ Proposed fix
private void Awake()
{
_camera = Camera.main;
+ if (_camera == null)
+ {
+ Debug.LogError($"[CollisionEffectSpawner] Camera.main is null on '{name}'. " +
+ "Ensure a camera is tagged as MainCamera in the scene.");
+ }
}📝 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.
Suggested change
| private void Awake() | |
| { | |
| _camera = Camera.main; | |
| } | |
| private void Awake() | |
| { | |
| _camera = Camera.main; | |
| if (_camera == null) | |
| { | |
| Debug.LogError($"[CollisionEffectSpawner] Camera.main is null on '{name}'. " + | |
| "Ensure a camera is tagged as MainCamera in the scene."); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Gameplay/Collision/CollisionEffectSpawner.cs` around lines 16
- 19, The Awake() method assigns Camera.main to the _camera field without
validating that it is not null. If no camera is tagged as MainCamera at startup,
_camera will remain null and cause a NullReferenceException when
Tween.ShakeCamera attempts to use it later. Add a null check immediately after
the assignment in Awake() that validates _camera is not null, and if it is null,
throw an exception with a clear error message stating that a camera tagged as
MainCamera is required for CollisionEffectSpawner to function.
Source: Coding guidelines
Comment on lines
+46
to
+48
| Debug.Log(data.pixelsDestroyed.Length); | ||
| var strengthFactor = Mathf.Clamp(Mathf.Log(Mathf.Sqrt(data.pixelsDestroyed.Length), 4), 0.1f, 0.5f); | ||
| Debug.Log(strengthFactor); |
There was a problem hiding this comment.
Remove debug logging before production.
These Debug.Log statements will spam the console during gameplay and degrade performance. Remove them or wrap them in conditional compilation directives if needed for development builds only.
🧹 Proposed fix
if (data.pixelsDestroyed.Length > 0 && PlayerCollision(data))
{
- Debug.Log(data.pixelsDestroyed.Length);
var strengthFactor = Mathf.Clamp(Mathf.Log(Mathf.Sqrt(data.pixelsDestroyed.Length), 4), 0.1f, 0.5f);
- Debug.Log(strengthFactor);
if (strengthFactor > 0.1)
Tween.ShakeCamera(📝 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.
Suggested change
| Debug.Log(data.pixelsDestroyed.Length); | |
| var strengthFactor = Mathf.Clamp(Mathf.Log(Mathf.Sqrt(data.pixelsDestroyed.Length), 4), 0.1f, 0.5f); | |
| Debug.Log(strengthFactor); | |
| var strengthFactor = Mathf.Clamp(Mathf.Log(Mathf.Sqrt(data.pixelsDestroyed.Length), 4), 0.1f, 0.5f); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Gameplay/Collision/CollisionEffectSpawner.cs` around lines 46
- 48, Remove the two Debug.Log statements that log data.pixelsDestroyed.Length
and strengthFactor in the CollisionEffectSpawner.cs file. These console logs
will spam the output during gameplay and impact performance. If you need these
logs for development purposes, wrap them in conditional compilation directives
(such as `#if` DEBUG) so they only appear in development builds, not in
production.
|
|
||
| if (data.SpeedDifference != null) | ||
| { | ||
| Debug.Log(data.SpeedDifference.Value.magnitude); |
There was a problem hiding this comment.
Remove debug logging before production.
This Debug.Log statement will spam the console on every collision and degrades performance. Remove it or wrap it in a conditional compilation directive if needed for development builds only.
🧹 Proposed fix
if (data.SpeedDifference != null)
{
- Debug.Log(data.SpeedDifference.Value.magnitude);
if (data.SpeedDifference.Value.magnitude > minMagnitudeForClunk)
_soundManager.Play(SoundIdentifier.Collision, data.contactPoint);
}📝 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.
Suggested change
| Debug.Log(data.SpeedDifference.Value.magnitude); | |
| if (data.SpeedDifference != null) | |
| { | |
| if (data.SpeedDifference.Value.magnitude > minMagnitudeForClunk) | |
| _soundManager.Play(SoundIdentifier.Collision, data.contactPoint); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Gameplay/Collision/CollisionSoundPlayer.cs` at line 30, The
Debug.Log statement logging data.SpeedDifference.Value.magnitude in the
CollisionSoundPlayer.cs file is executed on every collision, causing console
spam and performance degradation. Remove this Debug.Log statement entirely, or
if it is needed for development debugging, wrap it in a conditional compilation
directive such as `#if` DEBUG to ensure it only executes in development builds and
not in production.
| public class ShootingEffectSpawner : MonoBehaviour | ||
| { | ||
| private Camera _camera; | ||
| [Inject] private IEffectsSpawner _effectsSpawner; |
There was a problem hiding this comment.
Remove unused injected dependency.
The _effectsSpawner field is injected but never used anywhere in the class.
🧹 Proposed fix
- [Inject] private IEffectsSpawner _effectsSpawner;
[Inject] private ShootingEventChannel _shootingEventChannel;📝 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.
Suggested change
| [Inject] private IEffectsSpawner _effectsSpawner; | |
| [Inject] private ShootingEventChannel _shootingEventChannel; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.cs` at line 12, The
_effectsSpawner field is declared with the [Inject] attribute but is never
referenced or used anywhere in the ShootingEffectSpawner class. Remove the
unused injected dependency by deleting the entire line containing the [Inject]
private IEffectsSpawner _effectsSpawner declaration to clean up the code and
eliminate unnecessary injection overhead.
Comment on lines
+15
to
+18
| private void Awake() | ||
| { | ||
| _camera = Camera.main; | ||
| } |
There was a problem hiding this comment.
Fail hard if Camera.main is null.
Camera.main can return null if no camera is tagged "MainCamera". As per coding guidelines, components necessary for the script should fail hard with an error rather than silently continuing.
🛡️ Proposed fix
private void Awake()
{
_camera = Camera.main;
+ if (_camera == null)
+ throw new UnityException("[ShootingEffectSpawner] Main camera is required but not found.");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Gameplay/Shooting/ShootingEffectSpawner.cs` around lines 15 -
18, The Awake() method in ShootingEffectSpawner does not validate that
Camera.main successfully returned a camera object. Add a null check after
assigning Camera.main to _camera, and if it is null, fail hard by logging an
error and throwing an exception or calling an error method (such as
Debug.LogError followed by an assertion or explicit error handling) to prevent
silent failures when no camera is tagged "MainCamera".
Source: Coding guidelines
Comment on lines
+179
to
+187
| private static (float minDistance, float maxDistance) GetEffectiveDistances(Sound sound) | ||
| { | ||
| if (Mathf.Approximately(sound.maxDistance, -1f)) | ||
| return (1000f, 10000f); | ||
|
|
||
| var minDistance = sound.maxDistance / 10f; | ||
| var maxDistance = sound.maxDistance; | ||
| return (minDistance, maxDistance); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract magic numbers to named constants.
The method uses hardcoded values 1000f, 10000f, and division by 10f without explanation. These spatial audio configuration values should be extracted to named constants for clarity and maintainability.
♻️ Proposed refactor to use constants
Add constants at class level:
+private const float DefaultSpatialMinDistance = 1000f;
+private const float DefaultSpatialMaxDistance = 10000f;
+private const float SpatialMinDistanceRatio = 10f;
+
private void Awake()Then update the method:
private static (float minDistance, float maxDistance) GetEffectiveDistances(Sound sound)
{
if (Mathf.Approximately(sound.maxDistance, -1f))
- return (1000f, 10000f);
+ return (DefaultSpatialMinDistance, DefaultSpatialMaxDistance);
- var minDistance = sound.maxDistance / 10f;
+ var minDistance = sound.maxDistance / SpatialMinDistanceRatio;
var maxDistance = sound.maxDistance;
return (minDistance, maxDistance);
}📝 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.
Suggested change
| private static (float minDistance, float maxDistance) GetEffectiveDistances(Sound sound) | |
| { | |
| if (Mathf.Approximately(sound.maxDistance, -1f)) | |
| return (1000f, 10000f); | |
| var minDistance = sound.maxDistance / 10f; | |
| var maxDistance = sound.maxDistance; | |
| return (minDistance, maxDistance); | |
| } | |
| private const float DefaultSpatialMinDistance = 1000f; | |
| private const float DefaultSpatialMaxDistance = 10000f; | |
| private const float SpatialMinDistanceRatio = 10f; | |
| private static (float minDistance, float maxDistance) GetEffectiveDistances(Sound sound) | |
| { | |
| if (Mathf.Approximately(sound.maxDistance, -1f)) | |
| return (DefaultSpatialMinDistance, DefaultSpatialMaxDistance); | |
| var minDistance = sound.maxDistance / SpatialMinDistanceRatio; | |
| var maxDistance = sound.maxDistance; | |
| return (minDistance, maxDistance); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Services/Sound/SoundManager.cs` around lines 179 - 187, The
GetEffectiveDistances method contains hardcoded magic numbers (1000f, 10000f,
and 10f) that lack clarity and maintainability. Extract these values to named
constants at the class level in SoundManager.cs: one constant for the default
minimum distance value (1000f), one for the default maximum distance value
(10000f), and one for the distance ratio divisor (10f). Then replace the
hardcoded values in the GetEffectiveDistances method with references to these
named constants to improve code readability and make future adjustments easier.
Comment on lines
+87
to
+109
| foreach (var projectileSpawnPoint in projectileSpawnPoints) | ||
| { | ||
| var newBullet = | ||
| _projectilesSpawner.Spawn(projectilePrefab, projectileSpawnPoint.position, rotation, | ||
| shooterColliders); | ||
|
|
||
| var bulletRigidbody = newBullet.GetComponent<Rigidbody2D>(); | ||
| bulletRigidbody.linearVelocity = PixelatedRigidbody.Rigidbody.linearVelocity; | ||
| bulletRigidbody.AddForce(PixelatedRigidbody.Rigidbody.linearVelocity + direction * projectileSpeed, | ||
| ForceMode2D.Impulse); | ||
|
|
||
| var bulletCollider = newBullet.GetComponent<Collider2D>(); | ||
| foreach (var otherBulletCollider in bulletColliders) | ||
| Physics2D.IgnoreCollision(bulletCollider, otherBulletCollider); | ||
| bulletColliders.Add(bulletCollider); | ||
|
|
||
| _shootingEventChannel?.Raise(new BulletShootingData( | ||
| Ship, | ||
| projectileSpawnPoint.position, | ||
| direction, | ||
| bulletRigidbody.mass * bulletRigidbody.linearVelocity.magnitude | ||
| )); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for similar AddForce patterns with linearVelocity
rg -nP -C3 'linearVelocity.*AddForce|AddForce.*linearVelocity' --type=csRepository: lmProgramming/space-pixel-unity
Length of output: 775
🏁 Script executed:
cd Assets/Scripts/Ships/Modules && wc -l Cannon.cs && sed -n '70,115p' Cannon.csRepository: lmProgramming/space-pixel-unity
Length of output: 2005
🏁 Script executed:
# Search for other bullet/projectile spawning patterns
rg -n 'AddForce.*Impulse|ForceMode2D' --type=cs -B 2 -A 2Repository: lmProgramming/space-pixel-unity
Length of output: 1053
🏁 Script executed:
# Search for PixelatedRigidbody implementation
fd -e cs | xargs grep -l "class PixelatedRigidbody" | head -5Repository: lmProgramming/space-pixel-unity
Length of output: 193
🏁 Script executed:
# Look for tests related to bullet spawning
fd -e cs -path "*/Tests/*" | xargs grep -l "Bullet\|Cannon" 2>/dev/null | head -5Repository: lmProgramming/space-pixel-unity
Length of output: 244
🏁 Script executed:
# Read PixelatedRigidbody implementation
cat Assets/Scripts/Pixelation/PixelatedRigidbody.csRepository: lmProgramming/space-pixel-unity
Length of output: 17674
🏁 Script executed:
# Search for test files related to Cannon or Bullet
find . -name "*.cs" -path "*/Tests/*" -o -name "*Test*.cs" | grep -i "cannon\|bullet" | head -10Repository: lmProgramming/space-pixel-unity
Length of output: 57
🏁 Script executed:
# Check for any documentation or comments about projectile velocity in the codebase
rg -n 'velocity.*projectile|projectile.*velocity' --type=cs -B 1 -A 1Repository: lmProgramming/space-pixel-unity
Length of output: 57
Fix bullet velocity calculation to avoid applying ship velocity twice.
Lines 94–96 set bulletRigidbody.linearVelocity to the ship's velocity, then immediately apply an impulse that includes the ship's velocity again. Since AddForce with ForceMode2D.Impulse directly modifies velocity, the ship's velocity is applied twice:
- Final velocity = ship velocity + (ship velocity + direction × projectileSpeed)
Either set the velocity once:
bulletRigidbody.linearVelocity = PixelatedRigidbody.Rigidbody.linearVelocity + direction * projectileSpeed;
Or apply only the directional force:
bulletRigidbody.linearVelocity = PixelatedRigidbody.Rigidbody.linearVelocity;
bulletRigidbody.AddForce(direction * projectileSpeed, ForceMode2D.Impulse);
Clarify which approach matches the intended projectile trajectory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Ships/Modules/Cannon.cs` around lines 87 - 109, The bullet
velocity calculation in the projectile spawning loop is applying the ship's
velocity twice: once when directly setting bulletRigidbody.linearVelocity and
again when calling AddForce with ForceMode2D.Impulse. Remove the redundant
velocity application by either: (1) removing the line that sets
bulletRigidbody.linearVelocity to just the ship velocity and instead combine it
with the direction calculation, or (2) keeping the linearVelocity assignment and
removing PixelatedRigidbody.Rigidbody.linearVelocity from the AddForce call so
only the directional impulse is applied. Choose the approach that matches your
intended projectile trajectory behavior.
| } | ||
|
|
||
| public IShip Ship { get; protected set; } | ||
| public Collider2D Collider2D => PixelatedRigidbody.Collider2D; |
There was a problem hiding this comment.
Potential null reference when accessing PixelatedRigidbody.Collider2D.
The property forwards to PixelatedRigidbody.Collider2D without null checking PixelatedRigidbody. Although PixelatedRigidbody is assigned in Awake() (line 66), if Collider2D is accessed before Awake() runs or if GetComponent<PixelatedRigidbody>() returns null, a NullReferenceException will be thrown.
🛡️ Proposed defensive guard
-public Collider2D Collider2D => PixelatedRigidbody.Collider2D;
+public Collider2D Collider2D => PixelatedRigidbody?.Collider2D;Or fail hard per coding guidelines:
-public Collider2D Collider2D => PixelatedRigidbody.Collider2D;
+public Collider2D Collider2D => PixelatedRigidbody != null
+ ? PixelatedRigidbody.Collider2D
+ : throw new InvalidOperationException("PixelatedRigidbody not initialized");📝 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.
Suggested change
| public Collider2D Collider2D => PixelatedRigidbody.Collider2D; | |
| public Collider2D Collider2D => PixelatedRigidbody != null | |
| ? PixelatedRigidbody.Collider2D | |
| : throw new InvalidOperationException("PixelatedRigidbody not initialized"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Ships/Modules/Module.cs` at line 108, The Collider2D property
forwards to PixelatedRigidbody.Collider2D without null checking
PixelatedRigidbody first, which can cause a NullReferenceException if accessed
before Awake() runs or if the component is not found. Add a null check in the
Collider2D property getter to validate that PixelatedRigidbody is not null
before accessing its Collider2D property, either by logging an error and
returning null or by throwing an exception with a clear error message per your
coding guidelines.
Comment on lines
247
to
257
| public void OnShipConnectionLost() | ||
| { | ||
| Ship = null; | ||
| Destroy(this); | ||
|
|
||
| // ship snapshot doesn't assign modules before rewriting so they can be stale. Should be OK considering using shells | ||
| if (Ship == null) return; | ||
| Ship?.OnModuleConnectionLost(this); | ||
| Ship = null; | ||
|
|
||
| DetachAllConnections(); | ||
| } |
There was a problem hiding this comment.
Destruction order causes the module to notify the ship and detach after Destroy(this).
Line 249 calls Destroy(this) immediately, marking the Module component for destruction. The subsequent operations (notifying the ship at line 253 and calling DetachAllConnections() at line 256) execute on a destroyed component, which violates component lifecycle best practices. Additionally, DetachAllConnections() is called both here (line 256) and in OnDestroy() (line 82), creating duplicate cleanup.
The destruction sequence should be: notify ship, detach connections, then destroy the component at the end of the method.
🔧 Proposed fix for destruction order
public void OnShipConnectionLost()
{
- Destroy(this);
-
// ship snapshot doesn't assign modules before rewriting so they can be stale. Should be OK considering using shells
if (Ship == null) return;
- Ship?.OnModuleConnectionLost(this);
+ Ship.OnModuleConnectionLost(this);
Ship = null;
DetachAllConnections();
+
+ Destroy(this);
}Also consider removing the duplicate DetachAllConnections() call from OnDestroy() (line 82) since it will be called by OnShipConnectionLost() (line 85).
📝 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.
Suggested change
| public void OnShipConnectionLost() | |
| { | |
| Ship = null; | |
| Destroy(this); | |
| // ship snapshot doesn't assign modules before rewriting so they can be stale. Should be OK considering using shells | |
| if (Ship == null) return; | |
| Ship?.OnModuleConnectionLost(this); | |
| Ship = null; | |
| DetachAllConnections(); | |
| } | |
| public void OnShipConnectionLost() | |
| { | |
| // ship snapshot doesn't assign modules before rewriting so they can be stale. Should be OK considering using shells | |
| if (Ship == null) return; | |
| Ship.OnModuleConnectionLost(this); | |
| Ship = null; | |
| DetachAllConnections(); | |
| Destroy(this); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Ships/Modules/Module.cs` around lines 247 - 257, The
Destroy(this) call in the OnShipConnectionLost() method is executed too early,
causing the subsequent Ship notification and DetachAllConnections() call to
occur on an already-destroyed component. Move the Destroy(this) call to the end
of the OnShipConnectionLost() method so that Ship?.OnModuleConnectionLost(this),
Ship = null, and DetachAllConnections() are all executed before the component is
destroyed. Additionally, remove the duplicate DetachAllConnections() call from
the OnDestroy() method since it will now be properly called by
OnShipConnectionLost() before destruction occurs.
Comment on lines
+19
to
+23
| protected override void Start() | ||
| { | ||
| base.Start(); | ||
| foreach (var module in AllModules) module.Transform!.tag = "Player"; | ||
| } |
There was a problem hiding this comment.
Null-forgiving operator suppresses legitimate null check on module.Transform.
Line 22 uses module.Transform!.tag, where the ! operator suppresses the nullable warning. Since IModule.Transform is marked [CanBeNull], this can throw a NullReferenceException if a module's Transform is null. As per coding guidelines, fail hard with an explicit error rather than allowing silent failures.
🛡️ Proposed fix with explicit null check
protected override void Start()
{
base.Start();
- foreach (var module in AllModules) module.Transform!.tag = "Player";
+ foreach (var module in AllModules)
+ {
+ if (module.Transform == null)
+ {
+ Debug.LogError($"Module {module.Type} has null Transform", this);
+ continue;
+ }
+ module.Transform.tag = "Player";
+ }
}📝 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.
Suggested change
| protected override void Start() | |
| { | |
| base.Start(); | |
| foreach (var module in AllModules) module.Transform!.tag = "Player"; | |
| } | |
| protected override void Start() | |
| { | |
| base.Start(); | |
| foreach (var module in AllModules) | |
| { | |
| if (module.Transform == null) | |
| { | |
| Debug.LogError($"Module {module.Type} has null Transform", this); | |
| continue; | |
| } | |
| module.Transform.tag = "Player"; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/Scripts/Ships/PlayerShip.cs` around lines 19 - 23, In the PlayerShip
class Start method, replace the null-forgiving operator `!` on
`module.Transform` with an explicit null check. Instead of silently suppressing
the nullable warning with `module.Transform!.tag`, add a guard clause or
conditional check that throws or logs an error if module.Transform is null,
ensuring the code fails hard with a clear error message rather than risking a
NullReferenceException at runtime.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #81
resolvse #73
Summary by CodeRabbit
Release Notes
New Features
Improvements