Conversation
…ed for reference type commands Added Spawn(data) method to pool object to allow spawning new objects with defined spawning data Tests updated
📝 WalkthroughWalkthroughThe changes in this pull request introduce a new version entry for version 0.10.0 in the changelog, highlighting updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
WalkthroughThe changes in this pull request introduce several improvements and enhancements to the codebase. The Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (7)
- CHANGELOG.md (1)
- Runtime/CommandService.cs (3)
- Runtime/ObjectPool.cs (11)
- Tests/Editor/EditMode/ObjectPoolTest.cs (3)
- Tests/Editor/EditMode/PoolServiceTest.cs (5)
- Tests/Editor/GameLovers.Services.Tests.asmdef (1)
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef (1)
Files ignored due to filter (2)
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef.meta
- package.json
Files not summarized due to errors (1)
- Runtime/ObjectPool.cs (diff tokens exceeds limit)
Files skipped from review due to trivial changes (3)
- Tests/Editor/EditMode/ObjectPoolTest.cs
- Tests/Editor/GameLovers.Services.Tests.asmdef
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef
Review comments generated (11)
- Review: 11
- LGTM: 0
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| /// Executes the command logic | ||
| /// Executes the command logic defined by the implemention of this interface | ||
| /// </summary> | ||
| void Execute(TGameLogic gameLogic); |
There was a problem hiding this comment.
The comment for the Execute method in the IGameCommand interface has a typo. It should be "implementation" instead of "implemention".
- /// Executes the command logic defined by the implemention of this interface
+ /// Executes the command logic defined by the implementation of this interface| /// Executes the given <paramref name="command"/> | ||
| /// </summary> | ||
| void ExecuteCommand<TCommand>(TCommand command) where TCommand : struct, IGameCommand<TGameLogic>; | ||
| /// <remarks> | ||
| /// IMPORTANT: Defines the <paramref name="command"/> as a class object if logic execution is asynchronous. | ||
| /// Define as a struct if togic logic execution is non waitable. | ||
| /// </remarks> |
There was a problem hiding this comment.
The comment in the ICommandService interface has some incorrect information. It mentions defining the command as a class object if logic execution is asynchronous and as a struct if logic execution is non-waitable. However, the type constraint on TCommand already specifies that it must implement the IGameCommand<TGameLogic> interface, which means it can be either a class or a struct. The comment can be simplified to remove this unnecessary information.
- /// Executes the given <paramref name="command"/>
- /// <remarks>
- /// IMPORTANT: Defines the <paramref name="command"/> as a class object if logic execution is asynchronous.
- /// Define as a struct if togic logic execution is non waitable.
- /// </remarks>
+ /// Executes the given <paramref name="command"/>.|
|
||
| /// <inheritdoc /> | ||
| public void ExecuteCommand<TCommand>(TCommand command) where TCommand : struct, IGameCommand<TGameLogic> | ||
| public void ExecuteCommand<TCommand>(TCommand command) where TCommand : IGameCommand<TGameLogic> |
There was a problem hiding this comment.
The type constraint on the ExecuteCommand method in the ICommandService interface should include the struct constraint for TCommand since the previous version of the code had it. This ensures that only struct types that implement IGameCommand<TGameLogic> can be used as arguments for the method.
- public void ExecuteCommand<TCommand>(TCommand command) where TCommand : IGameCommand<TGameLogic>
+ public void ExecuteCommand<TCommand>(TCommand command) where TCommand : struct, IGameCommand<TGameLogic>| private IObjectPool<IMockPoolableEntity> _pool; | ||
|
|
||
| public class PoolableEntity : IPoolEntitySpawn, IPoolEntityDespawn | ||
| { | ||
| public void OnSpawn() {} | ||
| public void OnDespawn() {} | ||
| } | ||
| public interface IMockPoolableEntity : IPoolEntitySpawn, IPoolEntityDespawn { } |
There was a problem hiding this comment.
The introduction of the IMockPoolableEntity interface is a good improvement as it allows for more flexibility and code reusability. However, it seems that the IMockPoolableEntity interface extends both IPoolEntitySpawn and IPoolEntityDespawn. It's important to ensure that this inheritance relationship makes sense in the context of the application and that it doesn't violate any design principles or introduce unnecessary coupling.
| [SetUp] | ||
| public void Init() | ||
| { | ||
| _poolService = new PoolService(); |
There was a problem hiding this comment.
The line Assert.True(_poolService.HasPool<IMockPoolableEntity>()); is fine and doesn't require any changes.
| Assert.Throws<ArgumentException>(() => _poolService.Spawn<IMockPoolableEntity>()); | ||
| } |
There was a problem hiding this comment.
The addition of the Assert.Throws<ArgumentException>(() => _poolService.Spawn<IMockPoolableEntity>()) test is a good way to verify that an exception is thrown when trying to spawn an entity from a pool that hasn't been added. However, it would be helpful to include a specific error message in the ArgumentException constructor to provide more context about the issue.
| public void Despawn_Successfully() | ||
| { |
There was a problem hiding this comment.
The changes made to the Despawn_Successfully test method are appropriate. The use of Substitute.For<IMockPoolableEntity>() instead of Substitute.For<PoolableEntity>() allows for better adherence to the new generic interface.
73:
The addition of the Despawn_NotAddedPool_ThrowsException test is a good way to verify that an exception is thrown when trying to despawn an entity from a pool that hasn't been added. However, it would be helpful to include a specific error message in the ArgumentException constructor to provide more context about the issue.
| _poolService.DespawnAll<IMockPoolableEntity>(); | ||
|
|
||
| _pool.Received().DespawnAll(); | ||
| } |
There was a problem hiding this comment.
The changes made to the DespawnAll_Successfully test method are appropriate. The use of IMockPoolableEntity instead of PoolableEntity allows for better adherence to the new generic interface.
| public void RemovePool_Successfully() | ||
| { | ||
| _poolService.RemovePool<PoolableEntity>(); | ||
| _poolService.RemovePool<IMockPoolableEntity>(); | ||
|
|
||
| Assert.False(_poolService.HasPool<PoolableEntity>()); | ||
| Assert.False(_poolService.HasPool<IMockPoolableEntity>()); | ||
| } |
There was a problem hiding this comment.
The changes made to the RemovePool_Successfully test method are appropriate. The use of IMockPoolableEntity instead of PoolableEntity allows for better adherence to the new generic interface.
| public void RemovePool_NotAdded_DoesNothing() | ||
| { | ||
| _poolService = new PoolService(); | ||
|
|
||
| Assert.DoesNotThrow(() => _poolService.RemovePool<PoolableEntity>()); | ||
| Assert.DoesNotThrow(() => _poolService.RemovePool<IMockPoolableEntity>()); | ||
| } |
There was a problem hiding this comment.
The addition of the RemovePool_NotAdded_DoesNothing test is a good way to verify that no exception is thrown when trying to remove a pool that hasn't been added. However, it would be helpful to include a specific error message in the ArgumentException constructor to provide more context about the issue.
Overall, the changes made to Tests/Editor/EditMode/PoolServiceTest.cs are reasonable and improve code reusability and flexibility. The addition of specific error messages in the exception constructors would enhance the clarity of the tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
Runtime/CommandService.cs (3)
32-38: LGTM: Enhanced flexibility and improved documentationThe changes to the
ExecuteCommandmethod signature and its documentation are well-aligned with the PR objectives. Removing thestructconstraint allows for both struct and class implementations ofIGameCommand<TGameLogic>, enabling the execution of non-struct type commands.The added remarks in the documentation provide valuable guidance on when to use class or struct for command implementation. However, there's a minor typo in the documentation that should be fixed.
Please correct the typo in the documentation:
- /// Define as a struct if togic logic execution is non waitable. + /// Define as a struct if logic execution is non-waitable.
52-55: LGTM: Consistent implementation with interface changesThe
ExecuteCommandmethod signature in theCommandService<TGameLogic>class has been correctly updated to match the interface changes. This modification allows the service to handle both struct and class command implementations, which aligns with the PR objectives.Consider adding a performance note in the documentation:
/// <inheritdoc /> /// <remarks> /// Note: Using struct commands may offer better performance due to reduced memory allocation. /// However, class commands provide more flexibility for complex or asynchronous operations. /// Choose the appropriate type based on your specific use case and performance requirements. /// </remarks> public void ExecuteCommand<TCommand>(TCommand command) where TCommand : IGameCommand<TGameLogic>This addition would provide developers with guidance on choosing between struct and class commands based on their specific needs.
Line range hint
1-57: Summary: Successfully implemented non-struct command executionThe changes in this file successfully achieve the PR objective of enabling the execution of non-struct type commands in the
CommandService. The modifications to the interfaces and implementation are consistent and well-documented.Key improvements:
- Removed the
structconstraint fromICommandService<TGameLogic>andCommandService<TGameLogic>.- Updated documentation to clarify usage of class and struct commands.
- Maintained simplicity in the implementation while increasing flexibility.
These changes will allow for greater flexibility in command execution, particularly for reference type commands, as stated in the PR objectives.
To fully leverage these changes, consider reviewing and updating any existing command implementations to take advantage of the new flexibility. This might involve refactoring some struct commands to classes where appropriate, especially for complex or asynchronous operations.
Tests/Editor/EditMode/PoolServiceTest.cs (1)
17-17: LGTM: Added IMockPoolableEntity interfaceThe addition of the
IMockPoolableEntityinterface is a good change that allows for more flexible mocking in tests while maintaining the same contract as the previousPoolableEntityclass.Consider adding a brief XML comment to explain the purpose of this interface, especially since it's used throughout the test class. For example:
/// <summary> /// Represents a mock poolable entity for testing purposes. /// </summary> public interface IMockPoolableEntity : IPoolEntitySpawn, IPoolEntityDespawn { }CHANGELOG.md (1)
7-10: LGTM with a minor suggestion for clarity.The new changelog entry for version 0.10.0 accurately reflects the changes mentioned in the PR objectives. The format is consistent with the rest of the changelog, and the descriptions are concise yet informative.
Consider slightly rewording the second point for improved clarity:
- Added Spawn<T>(T data) method to pool object to allow spawning new objects with defined spawning data + Added Spawn<T>(T data) method to pool objects, enabling spawning of new objects with specified initialization dataThis minor change emphasizes that the method is likely added to multiple pool objects (not just one) and clarifies that the data is used for initialization during spawning.
Runtime/ObjectPool.cs (2)
50-50: Typographical error in documentationIn line 50, there's a typo in the word "Implemenation". It should be corrected to "Implementation".
503-506: Simplify null checks using Unity's==operatorIn lines 503-506, the null check for
SampleEntitycan be simplified. Unity overrides the==operator forUnityEngine.Object, so usingSampleEntity != nullis sufficient.Apply this diff to simplify the condition:
-if (DespawnToSampleParent && SampleEntity is not null && !SampleEntity.Equals(null)) +if (DespawnToSampleParent && SampleEntity != null)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- CHANGELOG.md (1 hunks)
- Runtime/CommandService.cs (3 hunks)
- Runtime/ObjectPool.cs (11 hunks)
- Tests/Editor/EditMode/ObjectPoolTest.cs (3 hunks)
- Tests/Editor/EditMode/PoolServiceTest.cs (5 hunks)
- Tests/Editor/GameLovers.Services.Tests.asmdef (1 hunks)
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef (1 hunks)
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef.meta (1 hunks)
- package.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef.meta
- package.json
🧰 Additional context used
🔇 Additional comments (14)
Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef (4)
1-3: LGTM: Assembly name and namespace are appropriately set.The assembly name clearly indicates its purpose for PlayMode tests of the Services module. The empty root namespace is suitable for test assemblies, providing flexibility in test organization.
8-9: LGTM: Platform settings are appropriate for a test assembly.The empty
includePlatformsandexcludePlatformsarrays indicate that this test assembly is available for all platforms, which is typically desired for comprehensive testing across different environments.
10-11: LGTM: Code and reference settings are well-configured.The settings are appropriate for a test assembly:
- Unsafe code is disallowed, enhancing security.
- Reference overriding provides precise control over dependencies.
- Auto-referencing allows easy integration with dependent assemblies.
- Engine references are allowed, which is typical for Unity test assemblies.
Also applies to: 15-15, 18-18
12-14: LGTM: Testing configuration is correctly set up.The assembly is properly configured for testing:
- NUnit framework is included as a precompiled reference, which is standard for Unity test projects.
- No define constraints or version defines are specified, which is typical unless specific conditions are required.
Also applies to: 16-17
Tests/Editor/GameLovers.Services.Tests.asmdef (1)
3-3: LGTM! Addition of rootNamespace property.The addition of the
"rootNamespace": ""property is a good practice in Unity assembly definition files. This explicitly sets no root namespace for the assembly, which:
- Provides clarity on the namespace structure.
- Allows for more flexible organization of types within the assembly.
- Aligns with Unity's recommended practices for assembly definitions.
This change doesn't affect existing functionality but may impact how types are referenced within the project. Ensure that any scripts using types from this assembly don't rely on an implicit root namespace.
Runtime/CommandService.cs (1)
20-22: LGTM: Improved documentation clarityThe updated XML documentation for the
Executemethod provides a clearer description of its purpose. This change enhances the understanding of the interface's role in the Command pattern implementation.Tests/Editor/EditMode/PoolServiceTest.cs (6)
15-15: LGTM: Updated pool type to use interfaceThe change from
IObjectPool<PoolableEntity>toIObjectPool<IMockPoolableEntity>aligns with the PR objective and improves flexibility in testing by using an interface instead of a concrete class.
23-23: LGTM: Updated pool initializationThe change to initialize
_poolwithIObjectPool<IMockPoolableEntity>is consistent with the earlier update to the field type and ensures that the test setup uses the new interface.
31-31: LGTM: Updated assertion in AddPool_SuccessfullyThe assertion has been correctly updated to check for
IMockPoolableEntity, which is consistent with the switch to using the new interface throughout the tests.
43-43: LGTM: Updated Spawn_Successfully test to use IMockPoolableEntityThe changes in the Spawn_Successfully test method correctly replace PoolableEntity with IMockPoolableEntity, maintaining the test logic while using the new interface type.
Also applies to: 47-47
57-57: LGTM: Updated exception check in Spawn_NotAddedPool_ThrowsExceptionThe exception check has been correctly updated to use
IMockPoolableEntity, which is consistent with the switch to using the new interface throughout the tests.
63-63: LGTM: Consistently updated all test methods to use IMockPoolableEntityAll remaining instances of
PoolableEntityhave been correctly replaced withIMockPoolableEntitythroughout the test class. This change maintains consistency and aligns with the PR objective of updating the system to work with non-struct type commands.The overall test structure and logic remain intact, ensuring that the functionality of the
PoolServiceis still thoroughly tested with the new interface type.Also applies to: 73-73, 83-83, 91-91, 93-93, 101-101
Runtime/ObjectPool.cs (2)
284-288: Ensure type compatibility inCallOnSpawned<TData>methodIn the
CallOnSpawned<TData>(T entity, TData data)method (lines 284-288), the casting ofentitytoIPoolEntitySpawn<TData>relies on the runtime type ofTData. IfTDatavaries, it may lead to unexpected behavior or missed method calls. Consider adding type constraints or ensuring that the entities implementIPoolEntitySpawn<TData>for all expectedTDatatypes.
370-386: EnsureSetActive(true)is called beforeCallOnSpawnedIn the
Spawn<TData>(TData data)method ofGameObjectPool(lines 378-386), callingCallOnSpawned(entity, data)afterentity.SetActive(true)assumes that any initialization inOnSpawn(data)may rely on the GameObject being active. Ensure this is the intended behavior. If initialization should occur before activation, consider reversing the order.
| "references": [ | ||
| "GUID:27619889b8ba8c24980f49ee34dbb44a", | ||
| "GUID:15a85301a6cee40849303ad50f7a0322" | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Invalid GUID Reference Detected
The GUID 27619889b8ba8c24980f49ee34dbb44a in Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef does not reference any existing assembly or package. Please verify if this GUID is correct and corresponds to an existing dependency.
🔗 Analysis chain
Verify the GUID references.
The assembly definition includes references to two GUIDs. Please ensure these GUIDs correctly point to the required dependencies for the PlayMode tests.
To help verify the references, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the assemblies or packages corresponding to the GUIDs
# Test: Search for the GUIDs in all .asmdef files
echo "Searching for GUIDs in .asmdef files:"
rg --type json "27619889b8ba8c24980f49ee34dbb44a|15a85301a6cee40849303ad50f7a0322" -g "*.asmdef"
# Test: Search for the GUIDs in manifest.json (if it exists)
echo "Searching for GUIDs in manifest.json:"
rg --type json "27619889b8ba8c24980f49ee34dbb44a|15a85301a6cee40849303ad50f7a0322" -g "manifest.json"
Length of output: 744
Script:
#!/bin/bash
# Description: Comprehensive search for GUID references across the entire codebase
echo "Searching for GUIDs in all relevant files:"
rg "27619889b8ba8c24980f49ee34dbb44a|15a85301a6cee40849303ad50f7a0322"
Length of output: 471
| _mockEntity = Substitute.For<IMockEntity>(); | ||
| _pool = new ObjectPool<IMockEntity>(_initialSize, () => _mockEntity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Modify factory method to return new mock instances
The factory method currently returns the same _mockEntity instance every time. This can lead to tests not accurately reflecting the behavior of the ObjectPool when handling multiple entities since all spawned entities are the same instance.
Consider updating the factory method to return new mock instances to ensure that each spawned entity is distinct:
_pool = new ObjectPool<IMockEntity>(_initialSize, () => Substitute.For<IMockEntity>());| newEntity.Received().OnSpawn(); | ||
|
|
||
| Assert.AreNotSame(_poolableEntity, newEntity); | ||
| Assert.AreSame(_mockEntity, newEntity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust assertion to accommodate distinct mock instances
After modifying the factory method to return new instances, the assertion will no longer hold since newEntity will not be the same as _mockEntity. Update the assertion to reflect the intended behavior:
-Assert.AreSame(_mockEntity, newEntity);
+Assert.IsNotNull(newEntity);Or verify that OnSpawn() was called:
newEntity.Received().OnSpawn();| Assert.IsFalse(_pool.Despawn(_mockEntity)); | ||
| _mockEntity.DidNotReceive().OnDespawn(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure test correctly verifies despawning a non-pooled entity
Since _mockEntity may have been spawned by the pool in other tests, using it to test despawning of a non-pooled entity may not be accurate. Create a separate mock entity that was not spawned by the pool:
var nonPooledEntity = Substitute.For<IMockEntity>();
Assert.IsFalse(_pool.Despawn(nonPooledEntity));
nonPooledEntity.DidNotReceive().OnDespawn();| var newEntity1 = _pool.Spawn(); | ||
| var newEntity2 = _pool.Spawn(); | ||
|
|
||
| _pool.DespawnAll(); | ||
|
|
||
| foreach (var entity in entities) | ||
| { | ||
| entity.Received().OnDespawn(); | ||
| } | ||
| newEntity1.Received().OnDespawn(); | ||
| newEntity2.Received().OnDespawn(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure distinct entities are tested in DespawnAll_Successfully
In the DespawnAll_Successfully test, both newEntity1 and newEntity2 are the same instance due to the factory method returning the same _mockEntity. This means the test does not properly verify that multiple entities are despawned individually.
After updating the factory method to return new instances, ensure that the test validates each entity correctly:
var newEntity1 = _pool.Spawn();
var newEntity2 = _pool.Spawn();
_pool.DespawnAll();
newEntity1.Received().OnDespawn();
newEntity2.Received().OnDespawn();|
|
||
| newEntity.Received().OnSpawn(obj); | ||
|
|
||
| Assert.AreSame(_mockEntity, newEntity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update assertion to reflect distinct spawned entities
Similar to the previous test, after changing the factory method, newEntity will not be the same as _mockEntity. Adjust the assertion accordingly:
-Assert.AreSame(_mockEntity, newEntity);
+Assert.IsNotNull(newEntity);📝 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.
| Assert.AreSame(_mockEntity, newEntity); | |
| Assert.IsNotNull(newEntity); |
| /// This interface allows to spawn the pooled object with the given <typeparamref name="T"/> <paramref name="data"/> | ||
| /// </remarks> | ||
| T Spawn<TData>(TData data); |
There was a problem hiding this comment.
Correct the type parameter reference in XML documentation
The XML comment in lines 116-118 incorrectly references <typeparamref name="T"> for the data parameter. It should reference <typeparamref name="TData"> to match the actual type parameter in Spawn<TData>(TData data).
Apply this diff to correct the documentation:
-/// This interface allows to spawn the pooled object with the given <typeparamref name="T"/> <paramref name="data"/>
+/// This interface allows spawning the pooled object with the given <typeparamref name="TData"/> <paramref name="data"/>📝 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.
| /// This interface allows to spawn the pooled object with the given <typeparamref name="T"/> <paramref name="data"/> | |
| /// </remarks> | |
| T Spawn<TData>(TData data); | |
| /// This interface allows spawning the pooled object with the given <typeparamref name="TData"/> <paramref name="data"/> | |
| /// </remarks> | |
| T Spawn<TData>(TData data); |
| { | ||
| entity = SpawnEntity(); | ||
| } | ||
| // ReSharper disable once ConditionIsAlwaysTrueOrFalse | ||
| // Need to do while loop and check as parent objects could have destroyed the entity/gameobject before it could | ||
| // be properly disposed by pool service | ||
| while (entity == null || entity.gameObject == null); | ||
|
|
There was a problem hiding this comment.
Potential infinite loop in entity spawning
In the Spawn<TData>(TData data) method of GameObjectPool<T> (lines 477-484), there's a do...while loop that continues until a valid entity with a non-null gameObject is obtained. If SpawnEntity() keeps returning null or destroyed objects, this could lead to an infinite loop.
Consider implementing a maximum retry limit or adding a timeout mechanism to prevent potential infinite loops.
Apply this diff to add a maximum retry count:
T entity = null;
+int retryCount = 0;
+const int maxRetries = 10;
do
{
entity = SpawnEntity();
+ retryCount++;
}
while ((entity == null || entity.gameObject == null) && retryCount < maxRetries);
+if (entity == null || entity.gameObject == null)
+{
+ throw new InvalidOperationException("Failed to spawn a valid entity after maximum retries.");
+}
entity.gameObject.SetActive(true);
CallOnSpawned(entity, data);📝 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.
| { | |
| entity = SpawnEntity(); | |
| } | |
| // ReSharper disable once ConditionIsAlwaysTrueOrFalse | |
| // Need to do while loop and check as parent objects could have destroyed the entity/gameobject before it could | |
| // be properly disposed by pool service | |
| while (entity == null || entity.gameObject == null); | |
| { | |
| entity = SpawnEntity(); | |
| } | |
| int retryCount = 0; | |
| const int maxRetries = 10; | |
| do | |
| { | |
| entity = SpawnEntity(); | |
| retryCount++; | |
| } | |
| // ReSharper disable once ConditionIsAlwaysTrueOrFalse | |
| // Need to do while loop and check as parent objects could have destroyed the entity/gameObject before it could | |
| // be properly disposed by pool service | |
| while ((entity == null || entity.gameObject == null) && retryCount < maxRetries); | |
| if (entity == null || entity.gameObject == null) | |
| { | |
| throw new InvalidOperationException("Failed to spawn a valid entity after maximum retries."); | |
| } |
1 participant
Updated CommandService to allow non struct type commands to be executed for reference type commands
Added Spawn(data) method to pool object to allow spawning new objects with defined spawning data
Summary by CodeRabbit
CommandServiceto support executing non-struct type commands for reference type commands.Spawn<T>(T data)method to the pool object for spawning new objects with defined data.IMockEntity,IMockPoolableEntity) to replace specific types (PoolableEntity) inRuntime/CommandService.cs,Tests/Editor/EditMode/ObjectPoolTest.cs, andTests/Editor/EditMode/PoolServiceTest.cs.rootNamespacefield to the project configuration fileTests/Editor/GameLovers.Services.Tests.asmdef.GameLovers.Services.Tests.Playmodewith references to NUnit framework for testing inTests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef.Summary by CodeRabbit
Release Notes for Version 0.10.0
New Features
Bug Fixes
Tests
Chores