Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a centralized ObjectId/IdFactory system and migrates players, items, party logic, paper doll, and many packet serializers/tests to use object IDs via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant IdFactory as IdFactory (singleton)
participant Player
participant Item as ItemObject
participant Packet as PacketHandler
Note right of IdFactory: Allocation flow
Packet->>Player: request get_object_id()
Player->>IdFactory: IdFactory::instance().get_next_id() (on construct)
IdFactory-->>Player: ObjectId(Arc<i32>)
Player->>Packet: get_object_id() -> i32
Packet->>Packet: serialize object id into buffer
Note over Packet,Player: Tests now set Player.object_id explicitly
Item->>IdFactory: get_next_id()
IdFactory-->>Item: ObjectId(Arc<i32>)
Item->>Item: store object_id and expose via item_model
Note over IdFactory: On last ObjectId Arc drop -> release_id()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
l2-core/src/game_objects/player/quest.rs (1)
16-27:Quest::has_stateignores the requested state and always returns true when any state is presentThe current implementation shadows the
stateparameter and compares the value to itself:
- Inner
statebinding hides the parameter.state.eq(state)is alwaystruewhen a value exists.- As a result,
has_state("started")andhas_state("completed")both returntruewhenever aStatevalue is set.Because
is_started/is_completedrely onhas_state, this breaks quest state checks and directly affects logic inQuestList::new(completed quests may be serialized as started and never hit theis_completedbranch).Suggest correcting the implementation to actually compare against the requested state:
- pub fn has_state(&self, state: &str) -> bool { - if let Some(state) = self - .model - .variables - .get(QuestVariables::State.as_key()) - .and_then(JsonValue::as_str) - && state.eq(state) - { - true - } else { - false - } - } + pub fn has_state(&self, expected_state: &str) -> bool { + self.model + .variables + .get(QuestVariables::State.as_key()) + .and_then(JsonValue::as_str) + .map_or(false, |state| state.eq(expected_state)) + }
🧹 Nitpick comments (5)
login/src/enums.rs (1)
3-23: Usetracinginstead ofprintln!for state transition errorsYou now import
tracing::{error, info}but still useprintln!in the invalid‑transition branch. For consistent, structured logging (and to justify the new import), consider switching that message toerror!:impl GS { @@ - (Self::BfConnected, Self::Authed) => *self = Self::Authed, - _ => { - println!("Can't transition from connection state {self} to {desired_state}"); - return Err(GSLoginFailReasons::NotAuthed); - } + (Self::BfConnected, Self::Authed) => *self = Self::Authed, + _ => { + error!("Can't transition from connection state {self} to {desired_state}"); + return Err(GSLoginFailReasons::NotAuthed); + } }Optionally, you could also add an
info!on successful transitions if that’s useful for debugging state flows.l2-core/src/game_objects/item/_item.rs (1)
9-9: Unused import: Arc.The
Arcimport on line 9 doesn't appear to be used in this code segment. Consider removing it unless it's used elsewhere in the file.-use std::sync::Arc;l2-core/src/id_factory.rs (3)
75-88: Approve the deadlock mitigation, but consider further refinement.The implementation correctly avoids holding a lock during modification by copying the ID value and dropping the iterator before calling
remove(). This addresses the deadlock concern mentioned in the comment.However, for even clearer code, consider using
pop()if DashSet provides it (eliminating the two-phase approach):pub fn get_next_id(&self) -> ObjectId { // Try to get a reusable ID atomically if let Some(id_ref) = self.reusable_ids.iter().next() { let id_val = *id_ref; drop(id_ref); // explicit drop for clarity if self.reusable_ids.remove(&id_val) { return ObjectId::new(id_val); } // If remove failed, another thread took it, fall through to allocate new } ObjectId::new(self.next_id.fetch_add(1, Ordering::SeqCst)) }This handles the race where another thread might remove the same ID between iteration and removal.
10-10: Add overflow protection for the ID counter.The
next_idcounter will eventually overflow after ~2 billion allocations (since it starts at 0x1000_0000 and i32::MAX is ~2.1 billion). When this happens,fetch_addwill wrap to negative values, causing undefined behavior in the ID system.Consider adding a check in
get_next_id():pub fn get_next_id(&self) -> ObjectId { // ... reusable IDs logic ... let new_id = self.next_id.fetch_add(1, Ordering::SeqCst); if new_id < 0 { panic!("ObjectId counter overflow - no more IDs available!"); } ObjectId::new(new_id) }Or use
u32/u64instead ofi32if the protocol permits, and document the maximum number of allocations before restart is needed.
102-122: Tests use shared singleton state.The tests use
IdFactory::instance(), which returns the global singleton. This means tests are not isolated and could interfere with each other if run in parallel. While the current tests may work, this is fragile.Consider refactoring IdFactory to support direct instantiation for testing:
impl IdFactory { #[cfg(test)] pub fn new_for_test() -> Self { Self { next_id: AtomicI32::new(FIRST_OID), reusable_ids: DashSet::new(), } } }Then update tests to use isolated instances:
#[test] fn test_allocation() { let factory = IdFactory::new_for_test(); // ... test with isolated instance }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
game/src/packets/from_client/restart.rs(1 hunks)game/src/packets/to_client/char_info.rs(1 hunks)game/src/packets/to_client/char_move_to_location.rs(1 hunks)game/src/packets/to_client/char_selected.rs(3 hunks)game/src/packets/to_client/extended/equipped_items.rs(3 hunks)game/src/packets/to_client/extended/inventory_weight.rs(3 hunks)game/src/packets/to_client/extended/premium_state.rs(3 hunks)game/src/packets/to_client/extended/rotation.rs(3 hunks)game/src/packets/to_client/move_to.rs(1 hunks)game/src/packets/to_client/quest_list.rs(2 hunks)game/src/packets/to_client/relation_changed.rs(1 hunks)game/src/packets/to_client/skill_list.rs(0 hunks)game/src/packets/to_client/user_info.rs(5 hunks)l2-core/src/data/char_template.rs(1 hunks)l2-core/src/game_objects/item/_item.rs(3 hunks)l2-core/src/game_objects/player/_player.rs(8 hunks)l2-core/src/game_objects/player/paper_doll.rs(1 hunks)l2-core/src/game_objects/player/party.rs(1 hunks)l2-core/src/game_objects/player/quest.rs(1 hunks)l2-core/src/id_factory.rs(1 hunks)l2-core/src/lib.rs(1 hunks)login/src/enums.rs(1 hunks)login/src/packet/handleable/player_logout.rs(1 hunks)
💤 Files with no reviewable changes (1)
- game/src/packets/to_client/skill_list.rs
🧰 Additional context used
🧬 Code graph analysis (12)
game/src/packets/to_client/quest_list.rs (1)
l2-core/src/id_factory.rs (1)
new(18-20)
game/src/packets/to_client/relation_changed.rs (1)
entities/src/entities/character.rs (1)
Relation(76-95)
game/src/packets/from_client/restart.rs (1)
game/src/packets/to_client/delete_object.rs (1)
new(13-21)
game/src/packets/to_client/extended/equipped_items.rs (3)
game/src/packets/to_client/extended/inventory_weight.rs (1)
new(14-24)game/src/packets/to_client/extended/premium_state.rs (1)
new(14-21)game/src/packets/to_client/extended/rotation.rs (1)
new(15-24)
game/src/packets/to_client/extended/premium_state.rs (2)
l2-core/src/game_objects/player/_player.rs (1)
new(77-116)l2-core/src/id_factory.rs (1)
new(18-20)
login/src/packet/handleable/player_logout.rs (1)
login/src/controller/player_management.rs (1)
on_players_in_game(94-112)
game/src/packets/to_client/user_info.rs (2)
game/src/packets/to_client/extended/equipped_items.rs (1)
new(18-52)l2-core/src/game_objects/player/_player.rs (1)
new(77-116)
game/src/packets/to_client/char_selected.rs (2)
l2-core/src/id_factory.rs (1)
new(18-20)game/src/packets/to_client/char_selection.rs (1)
new(21-137)
game/src/packets/to_client/extended/inventory_weight.rs (5)
game/src/packets/to_client/extended/equipped_items.rs (1)
new(18-52)game/src/packets/to_client/extended/premium_state.rs (1)
new(14-21)game/src/packets/to_client/extended/rotation.rs (1)
new(15-24)l2-core/src/id_factory.rs (1)
new(18-20)game/src/packets/to_client/char_info.rs (1)
new(47-198)
l2-core/src/game_objects/player/_player.rs (4)
l2-core/src/game_objects/item/_item.rs (1)
from_items(51-65)l2-core/src/game_objects/player/inventory.rs (1)
from_items(14-18)l2-core/src/game_objects/player/paper_doll.rs (1)
restore_visible_inventory(97-119)l2-core/src/id_factory.rs (1)
instance(63-73)
l2-core/src/id_factory.rs (1)
l2-core/src/game_objects/player/_player.rs (3)
new(77-116)from(37-43)from(46-48)
game/src/packets/to_client/extended/rotation.rs (7)
game/src/packets/to_client/char_selected.rs (1)
new(16-56)game/src/packets/to_client/extended/equipped_items.rs (1)
new(18-52)game/src/packets/to_client/extended/inventory_weight.rs (1)
new(14-24)game/src/packets/to_client/extended/premium_state.rs (1)
new(14-21)game/src/packets/to_client/user_info.rs (1)
new(28-141)l2-core/src/id_factory.rs (1)
new(18-20)game/src/packets/to_client/move_to.rs (1)
new(14-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-only
🔇 Additional comments (31)
l2-core/src/data/char_template.rs (1)
142-144: LGTM!The formatting consolidation improves code readability by placing each variable declaration on a single line, following standard Rust conventions.
login/src/packet/handleable/player_logout.rs (3)
6-18: LGTM!The handler implementation correctly delegates to
on_player_logoutand is properly instrumented.
31-42: LGTM!The test correctly verifies the handler's behavior when the game server is not set.
43-58: Nice optimization - avoids unnecessary clone in test.The change from
&[acc.clone()]tostd::slice::from_ref(&acc)eliminates an unnecessary string clone while maintaining the same functionality. This is a good practice for test code efficiency.game/src/packets/to_client/quest_list.rs (1)
51-51: QuestList test wiring toObjectIdlooks correctImporting
ObjectIdand assigning a fixedplayer.object_idkeeps the test deterministic and aligns it with the new object‑ID system without affecting production behavior.Also applies to: 72-72
l2-core/src/lib.rs (1)
25-25: Publicly exportingid_factoryis appropriateMaking
id_factorya public module is necessary for consumers like the game crate to accessObjectId/IdFactory; no issues with this addition.game/src/packets/to_client/char_info.rs (1)
58-58: Usingget_object_id()inCharInfois consistent with the new ID modelSwitching this field to
p.get_object_id()ensures the packet uses the runtime object ID rather than the persistent character ID, matching the rest of the packet refactor.game/src/packets/from_client/restart.rs (1)
37-37: Restart handler now deletes by object IDUsing
player.get_object_id()forDeleteObjectensures the restart broadcast targets the correct in‑world entity instead of a persistent character ID.game/src/packets/to_client/char_move_to_location.rs (1)
21-21: Movement packet correctly keyed byget_object_id()Serializing
p.get_object_id()as the mover’s identifier matches the new object‑ID system and keeps this packet consistent with other movement‑related packets.game/src/packets/to_client/relation_changed.rs (1)
48-48: Relations now use runtime object IDsSetting
obj_idfromplayer.get_object_id()correctly ties relation updates to the in‑world object identifier instead of the character’s persistent ID, in line with the rest of the refactor.game/src/packets/to_client/move_to.rs (1)
17-17:MoveTopacket now keyed byget_object_id()Writing
p.get_object_id()here aligns the movement packet with the new object‑ID system and ensures clients track movement by runtime object ID rather than a persistent character ID.game/src/packets/to_client/char_selected.rs (2)
23-23: LGTM! Object ID sourcing centralized.The change from
player.char_model.idtoplayer.get_object_id()aligns with the PR's introduction of centralized object ID management viaIdFactoryandObjectId.
64-64: Test correctly updated for ObjectId usage.The test imports
ObjectIdand assigns a concrete object_id to the player instance, properly reflecting the new ID system.Also applies to: 89-90
game/src/packets/to_client/extended/premium_state.rs (1)
18-18: LGTM! Consistent with centralized object ID system.The migration from
p.char_model.idtop.get_object_id()is consistent with the broader refactoring across packet handlers.game/src/packets/to_client/user_info.rs (2)
55-55: LGTM! Object ID sourcing centralized in packet construction.The change to
player.get_object_id()aligns with the broader refactoring to use centralized object ID management.
428-428: LGTM! Party leadership comparison now uses object IDs.The update to compare
pt.get_leader_id()withp.get_object_id()is consistent with the changes inl2-core/src/game_objects/player/party.rs, whereget_leader_id()now returns the leader's object ID rather than the character model ID.game/src/packets/to_client/extended/inventory_weight.rs (1)
20-20: LGTM! Consistent with centralized object ID system.The change to
p.get_object_id()follows the same pattern as other packet handlers in this PR.l2-core/src/game_objects/player/party.rs (2)
45-45: LGTM! Leader ID now sourced from object ID.The change to return
self.get_leader().get_object_id()is consistent with the centralized object ID system. This aligns with the usage inuser_info.rsline 428 where party leadership is compared using object IDs.
53-57: All callers ofParty::index_of()have been properly updated.Verification confirms there is exactly one call site at
l2-core/src/game_objects/player/_player.rs:890, and it correctly passesself.get_object_id()as the parameter. The parameter rename fromplayer_idtoplayer_object_idaligns with the implementation and caller expectations.game/src/packets/to_client/extended/equipped_items.rs (1)
24-24: LGTM! Consistent with centralized object ID system.The migration to
p.get_object_id()follows the established pattern across packet handlers.game/src/packets/to_client/extended/rotation.rs (1)
21-21: LGTM! Consistent with centralized object ID system.The change to
player.get_object_id()is consistent with the broader refactoring across packet handlers.l2-core/src/game_objects/item/_item.rs (2)
45-45: New object_id field added to ItemObject.The addition of
object_id: ObjectIdtoItemObjectaligns with the broader refactoring to centralize object ID management across players, items, and other game objects.
54-64: No incompatibilities found—HashMap key change is already consistently implemented throughout the codebase.All identified item HashMap access patterns are already compatible with
object_id-based keys:
get_item()acceptsitem_obj_id: i32parameter (line 449-451 in _player.rs)PaperDoll::restore_visible_inventory()iterates.values()and usesitem.object_id.value()(paper_doll.rs:99-102)ItemListpacket iterates.values()without depending on keys (item_list.rs:25)WarehouseandInventoryboth useItemObject::from_items()consistentlyNo code attempts to access items by
item.item_model.id(database ID), confirming the refactoring was completed throughout the codebase.l2-core/src/game_objects/player/paper_doll.rs (1)
97-119: LGTM! Refactoring aligns with the new ObjectId system.The changes correctly adapt the function to work with the new
HashMap<i32, ItemObject>structure and properly access theobject_idfield through theObjectIdwrapper. The logic remains sound and the nested field accesses are appropriate.l2-core/src/game_objects/player/_player.rs (4)
82-87: LGTM! Proper integration of ObjectId system.The inventory creation and ObjectId assignment are correctly implemented. The factory-based ID generation ensures uniqueness and proper lifecycle management.
152-154: LGTM! Accessor method correctly retrieves the underlying ID value.The
get_object_id()method properly delegates toObjectId::value()to return the primitivei32value, maintaining the existing API contract while using the new ObjectId system internally.
890-890: LGTM! Party relation logic updated correctly.The party index lookup now correctly uses
get_object_id()to retrieve the player's unique identifier, consistent with the ObjectId refactor.
806-808: Additional impl block is fine.Splitting Player methods into multiple impl blocks can aid organization. No concerns here.
l2-core/src/id_factory.rs (3)
6-6: Verify the starting ID constant value.
FIRST_OID = 0x1000_0000(268,435,456 in decimal) seems like an unusual starting point. Ensure this aligns with the game's expected object ID range and doesn't conflict with other ID systems or serialization formats.If you're unsure about the rationale, consider documenting why this specific value was chosen (e.g., to reserve lower IDs for static/system objects).
62-73: LGTM! Proper singleton implementation.The use of
OnceLockensures thread-safe lazy initialization of the singleton instance. The implementation is correct and follows Rust best practices.
16-25: LGTM! Clean ObjectId wrapper design.The
ObjectIdwrapper aroundArc<i32>provides a clean abstraction with appropriate constructor and accessor methods. The use ofArcenables cheap cloning and automatic memory management.
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct Player { | ||
| pub object_id: ObjectId, |
There was a problem hiding this comment.
Consider making the object_id field private.
The object_id field is currently public, which exposes implementation details and could allow external code to replace the ObjectId directly, bypassing the factory's ID management and lifecycle tracking. This undermines the integrity of the ID system.
Consider this change:
- pub object_id: ObjectId,
+ object_id: ObjectId,Then provide a public getter if external read access is needed (though get_object_id() already serves this purpose).
📝 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.
| pub object_id: ObjectId, | |
| object_id: ObjectId, |
🤖 Prompt for AI Agents
In l2-core/src/game_objects/player/_player.rs around line 53, the object_id
field is declared pub which exposes internal ID management; change its
visibility from pub to private (remove pub) so external code cannot replace it,
rely on the existing get_object_id() for read access (or add a simple public
getter if missing), then update any external call sites that currently write or
access object_id directly to use constructor/factory or the getter; ensure
internal module code still accesses the private field directly or use pub(crate)
only if intra-crate write access is required.
| impl Drop for ObjectId { | ||
| fn drop(&mut self) { | ||
| // Only release if this is the last Arc | ||
| if Arc::strong_count(&self.0) == 1 { | ||
| IdFactory::instance().release_id(*self.0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Race condition in Drop implementation.
The check Arc::strong_count(&self.0) == 1 and the subsequent call to release_id() are not atomic. Between checking the count and calling release_id(), another thread could:
- Clone the Arc (incrementing the count to 2+)
- The original thread proceeds to release the ID
- Result: ID is released while still in use
This can lead to ID collisions where the factory reissues an ID that's still referenced elsewhere.
Unfortunately, there's no perfect solution with the current Drop trait. Consider one of these approaches:
Solution 1: Remove automatic recycling from Drop
Require explicit ID release when objects are no longer needed:
-impl Drop for ObjectId {
- fn drop(&mut self) {
- // Only release if this is the last Arc
- if Arc::strong_count(&self.0) == 1 {
- IdFactory::instance().release_id(*self.0);
- }
- }
-}Then update owner structs (like Player) to explicitly release IDs in their Drop implementations.
Solution 2: Accept the limitation and document it
Add clear documentation that ObjectId clones must not outlive the original in multi-threaded contexts. This is fragile but may be acceptable if your architecture guarantees single-ownership semantics.
Solution 3: Use weak references or other lifecycle management
Redesign to track object lifecycles differently, possibly with explicit lifecycle management at a higher level.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
l2-core/src/id_factory.rs around lines 48-54: the Drop impl for ObjectId uses
Arc::strong_count(...) == 1 then calls IdFactory::instance().release_id(...),
which is racy because the Arc can be cloned between the check and the release;
to fix, remove automatic recycling from ObjectId::drop (make it a no-op) and
instead require explicit release: add/ensure a public release_id API (on
IdFactory or an explicit ObjectId::release/self.release method) and update all
owning types (e.g., Player) to call that release in their Drop implementations
(or where ownership ends), make the explicit release operation thread-safe in
IdFactory, and add a comment documenting the change.
| pub fn release_id(&self, id: impl Into<i32>) { | ||
| let val = id.into(); | ||
| if !self.reusable_ids.insert(val) { | ||
| warn!("Trying to release already released id: {val}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add validation to prevent releasing invalid IDs.
The release_id method accepts any i32 value without validation. This allows:
- Releasing IDs below
FIRST_OIDthat were never allocated - Releasing negative IDs
- Releasing IDs beyond the current
next_idcounter
This could pollute the reusable_ids pool with invalid IDs that break the allocation invariants.
Apply this diff to add validation:
pub fn release_id(&self, id: impl Into<i32>) {
let val = id.into();
+ if val < FIRST_OID {
+ warn!("Attempted to release invalid id {val} (below FIRST_OID)");
+ return;
+ }
+ if val >= self.next_id.load(Ordering::SeqCst) {
+ warn!("Attempted to release unallocated id {val}");
+ return;
+ }
if !self.reusable_ids.insert(val) {
warn!("Trying to release already released id: {val}");
}
}🤖 Prompt for AI Agents
In l2-core/src/id_factory.rs around lines 90 to 95, validate the incoming id
before inserting into reusable_ids: after converting id.into(), check that it is
>= FIRST_OID, non-negative, and strictly less than the current allocation cursor
(read self.next_id with appropriate atomic ordering) — if any check fails, log a
warning and return without inserting; otherwise proceed to insert and keep the
existing duplicate-release warning path. Ensure you read next_id atomically to
avoid races and perform the validations before calling reusable_ids.insert.
…n issue with accessing static global variable
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.