fix: packets send after entity spawned#30
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WrapperLivingEntity
participant WrapperEntityPotionEffect
participant Viewers
Caller->>WrapperLivingEntity: addPotionEffect(...)
WrapperLivingEntity->>WrapperEntityPotionEffect: addPotionEffect(...)
WrapperEntityPotionEffect->>WrapperEntityPotionEffect: Store/update effect in map
alt Notify changes enabled
WrapperEntityPotionEffect->>Viewers: Send effect packet(s)
end
sequenceDiagram
participant WrapperLivingEntity
participant WrapperEntityPotionEffect
participant Equipment
participant Attributes
WrapperLivingEntity->>WrapperLivingEntity: createSpawnPackets()
WrapperLivingEntity->>Attributes: createAttributePackets()
WrapperLivingEntity->>Equipment: createEquipmentPackets()
WrapperLivingEntity->>WrapperEntityPotionEffect: createEffectPackets()
WrapperLivingEntity-->>Caller: Return combined packet list
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
api/src/main/java/me/tofaa/entitylib/EntityLibAPI.java (1)
17-20: Remove unusedConsumerimport to keep the class warning-free
java.util.function.Consumeris imported but never referenced in this interface after the latest changes.
Keeping unused imports triggers compiler/IDE warnings and can hide real issues.-import java.util.function.Consumer;api/src/main/java/me/tofaa/entitylib/wrapper/WrapperLivingEntity.java (2)
7-15: Clean up unused imports to silence compiler warnings
LocationandWrapperPlayServerEntityEquipment(and since you introduced the centralEntityContainer, that too) are no longer referenced in this class.-import com.github.retrooper.packetevents.protocol.world.Location; -import com.github.retrooper.packetevents.wrapper.play.server.WrapperPlayServerEntityEquipment; -import me.tofaa.entitylib.container.EntityContainer;
88-96: Fix Javadoc to reference the correct enum (PotionType)The comment still mentions
PotionTypes, which does not exist anymore and may confuse downstream developers and IDE auto-completion.- * @param type The type of the potion effect {@link com.github.retrooper.packetevents.protocol.potion.PotionTypes} + * @param type The type of the potion effect {@link com.github.retrooper.packetevents.protocol.potion.PotionType}api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntityPotionEffect.java (5)
27-30: Complete the parameter documentation.The JavaDoc comments for parameters
ambient,visible, andshowIconsare incomplete (only showing "A").- * @param ambient A - * @param visible A - * @param showIcons A + * @param ambient Whether the effect is ambient (from a beacon) + * @param visible Whether the effect is visible to the player + * @param showIcons Whether to show particle effects and HUD icons
138-140: Replace Spanish comments with English.The bit flag calculation comments are in Spanish and should be in English for consistency.
- flags |= ambient ? 1 : 0; // Bit 0 para ambient - flags |= visible ? (1 << 1) : 0; // Bit 1 para visible - flags |= icons ? (1 << 2) : 0; // Bit 2 para icons + flags |= ambient ? 1 : 0; // Bit 0 for ambient + flags |= visible ? (1 << 1) : 0; // Bit 1 for visible + flags |= icons ? (1 << 2) : 0; // Bit 2 for icons
142-156: Optimize packet creation by using the proper constructor.The current implementation creates a packet with dummy values then sets the actual values, which is inefficient. Consider using the appropriate constructor directly.
- WrapperPlayServerEntityEffect wrapperPlayServerEntityEffect = new WrapperPlayServerEntityEffect( - 0, - null, - 0, - 0, - (byte) flags - ); - - wrapperPlayServerEntityEffect.setEntityId(this.entity.getEntityId()); - wrapperPlayServerEntityEffect.setPotionType(potionType); - wrapperPlayServerEntityEffect.setEffectAmplifier(amplifier); - wrapperPlayServerEntityEffect.setEffectDurationTicks(duration); - wrapperPlayServerEntityEffect.setFactorData(factorData); + WrapperPlayServerEntityEffect wrapperPlayServerEntityEffect = new WrapperPlayServerEntityEffect( + this.entity.getEntityId(), + potionType, + amplifier, + duration, + (byte) flags + ); + + if (factorData != null) { + wrapperPlayServerEntityEffect.setFactorData(factorData); + }
165-169: Optimize the refresh method to avoid unnecessary ArrayList creation.The current implementation creates a new ArrayList from the values, which is unnecessary and inefficient.
- new ArrayList<>(this.effects.values()).forEach(effect -> { - WrapperPlayServerEntityEffect wrapperPlayServerEntityEffect = createEffectPacket(effect); - - this.entity.sendPacketsToViewers(wrapperPlayServerEntityEffect); - }); + this.effects.values().forEach(effect -> { + WrapperPlayServerEntityEffect wrapperPlayServerEntityEffect = createEffectPacket(effect); + + this.entity.sendPacketsToViewers(wrapperPlayServerEntityEffect); + });
27-30: Complete the incomplete JavaDoc documentation.The JavaDoc comments for the
ambient,visible, andshowIconsparameters are incomplete (marked with "A").- * @param ambient A - * @param visible A - * @param showIcons A + * @param ambient Whether the potion effect is ambient (from a beacon) + * @param visible Whether the potion effect should be visible to the client + * @param showIcons Whether the potion effect icons should be shown
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/src/main/java/me/tofaa/entitylib/APIConfig.java(0 hunks)api/src/main/java/me/tofaa/entitylib/EntityLibAPI.java(3 hunks)api/src/main/java/me/tofaa/entitylib/meta/EntityMeta.java(5 hunks)api/src/main/java/me/tofaa/entitylib/meta/display/TextDisplayMeta.java(0 hunks)api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.java(19 hunks)api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntityPotionEffect.java(1 hunks)api/src/main/java/me/tofaa/entitylib/wrapper/WrapperLivingEntity.java(3 hunks)common/src/main/java/me/tofaa/entitylib/common/AbstractEntityLibAPI.java(4 hunks)
💤 Files with no reviewable changes (2)
- api/src/main/java/me/tofaa/entitylib/meta/display/TextDisplayMeta.java
- api/src/main/java/me/tofaa/entitylib/APIConfig.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
api/src/main/java/me/tofaa/entitylib/wrapper/WrapperLivingEntity.java (3)
api/src/main/java/me/tofaa/entitylib/EntityLib.java (1)
EntityLib(9-39)api/src/main/java/me/tofaa/entitylib/meta/EntityMeta.java (1)
EntityMeta(24-288)api/src/main/java/me/tofaa/entitylib/utils/VersionUtil.java (1)
VersionUtil(7-32)
common/src/main/java/me/tofaa/entitylib/common/AbstractEntityLibAPI.java (2)
api/src/main/java/me/tofaa/entitylib/APIConfig.java (1)
APIConfig(7-85)api/src/main/java/me/tofaa/entitylib/EntityLib.java (1)
EntityLib(9-39)
🔇 Additional comments (16)
common/src/main/java/me/tofaa/entitylib/common/AbstractEntityLibAPI.java (1)
81-88: Guard concurrent mutations ofpacketDispatcher
packetDispatchercan be swapped at runtime throughsetPacketDispatcher.
If another thread is simultaneously dispatching packets, the reference change is not guaranteed to be visible without extra memory-barriers.
Two lightweight options:
- Declare the field
volatile.- Mark
setPacketDispatcher(and ideallygetPacketDispatcher) assynchronized.Either will eliminate a class of hard-to-trace, sporadic
NullPointerException/ stale reference bugs.api/src/main/java/me/tofaa/entitylib/wrapper/WrapperLivingEntity.java (1)
74-82: Optional: add entity-meta packet to spawn list for completeness
createSpawnPackets()now sends attributes, equipment and potion effects, but relies on the caller to remember to send the metadata packet produced inWrapperEntity.
If different code paths call this method directly, the viewers may miss basic metadata (custom name, glow, etc.).Consider either
• addingsuper.createSpawnPackets()’s result to this list, or
• updating the method documentation to state clearly that meta packets must be sent separately.This ensures new maintainers don’t introduce subtle desyncs when spawning entities.
api/src/main/java/me/tofaa/entitylib/meta/EntityMeta.java (1)
142-149: (Informational) No functional changes detected – formatting looks goodThe edited lines only add spacing around casts; behaviour remains identical.
Nothing to act on here.api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntityPotionEffect.java (4)
87-87: Simplify the ternary operator logic.The ternary operator can be simplified for better readability.
- addPotionEffect(type, amplifier, duration, flags, hasFactorData ? factorData : null); + addPotionEffect(type, amplifier, duration, flags, hasFactorData ? factorData : null);Actually, looking at this more carefully, the current logic is fine. The method correctly passes
factorDataonly whenhasFactorDatais true, otherwise passesnull.
13-21: LGTM! Excellent thread-safe design.The use of
ConcurrentHashMapfor the effects storage and proper encapsulation with a reference to the parent entity shows good design patterns for concurrent access scenarios.
115-117: Good defensive programming against concurrent modification.Creating a new
ArrayListfrom the keySet before iterating preventsConcurrentModificationExceptionwhen effects are modified during iteration.
136-141: Correct bit flag implementation.The bit manipulation logic correctly encodes the boolean flags into the appropriate bit positions according to the Minecraft protocol specification.
api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.java (9)
30-31: Excellent immutability improvement.Making the
entityTypeandentityMetafields final is a great improvement for immutability and thread safety.
218-221: Verify consistency with spawn method implementation.This viewer addition logic sends spawn packets to the new viewer UUID, which appears consistent with the intent. However, ensure this aligns with the spawn method at lines 94-97 which sends to the entity's own UUID.
The two spawning contexts (initial spawn vs. new viewer) should handle packets consistently. Verify that both approaches are correct for their respective use cases.
513-513: Excellent centralization of packet dispatching.The change from direct channel access to using
EntityLib.getApi().getPacketDispatcher()is a good architectural improvement that centralizes packet sending logic.
195-227: Good null safety improvements with @NotNull annotations.The extensive addition of
@NotNullannotations to viewer management methods significantly improves null safety and API clarity.Also applies to: 255-320
94-97:Details
❌ Incorrect review comment
Verify the spawn packet sending logic for WrapperLivingEntity.
The new logic sends spawn packets for
WrapperLivingEntityby iterating throughcreateSpawnPackets()and sending each packet individually to a UUID. This looks inconsistent with the pattern used elsewhere where packets are sent to viewers.Please verify that this logic is correct. The current implementation sends packets to the entity's own UUID rather than to viewers:
🏁 Script executed:
#!/bin/bash # Description: Search for createSpawnPackets() method implementation to understand what packets are being sent # Find the createSpawnPackets method in WrapperLivingEntity ast-grep --pattern $'createSpawnPackets() { $$$ }' # Also search for other usages of createSpawnPackets rg -A 10 "createSpawnPackets"Length of output: 3379
🏁 Script executed:
#!/bin/bash # Show the addViewer method context in WrapperEntity.java rg -n "addViewer" -A 15 -B 5 api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.javaLength of output: 2873
The spawn‐packet logic is correct; the review comment is based on a misinterpretation of
uuid.The
uuidin this context is the viewer’s UUID (the parameter ofaddViewer(UUID uuid)), not the entity’s own ID. Thus, iterating overcreateSpawnPackets()and callingsendPacket(uuid, …)correctly sends each packet to the new viewer.No changes required.
Likely an incorrect or invalid review comment.
30-31: Excellent immutability improvement.Making
entityTypeandentityMetafields final prevents accidental modification and improves thread safety.
218-222: Correct implementation for viewer-specific packets.This properly sends spawn packets to the specific viewer being added, which aligns with the PR objective of ensuring equipment and potion effects are sent to new viewers.
511-515: Good centralization of packet dispatching.The new centralized packet dispatcher approach simplifies packet sending logic and provides better maintainability.
195-227:Details
✅ Verification successful
Verify the packet sending logic addresses the PR objective.
The updated
addViewermethod now correctly sends spawn packets for living entities to new viewers, which should resolve the issue described in the PR where equipment and potion effects weren't being sent to viewers added after entity spawn.
🏁 Script executed:
#!/bin/bash # Verify that createSpawnPackets method exists and returns appropriate packets for living entities ast-grep --pattern $'class WrapperLivingEntity { $$$ createSpawnPackets() { $$$ } $$$ }'Length of output: 101
🏁 Script executed:
#!/bin/bash # Search for createSpawnPackets usage and declaration across the repository rg --context 3 "createSpawnPackets" -n .Length of output: 2335
🏁 Script executed:
#!/bin/bash # Extract full createSpawnPackets method body for inspection sed -n '74,110p' api/src/main/java/me/tofaa/entitylib/wrapper/WrapperLivingEntity.java # Search for any equipment packet creation within WrapperLivingEntity rg --context 3 "equipment" -n api/src/main/java/me/tofaa/entitylib/wrapper/WrapperLivingEntity.javaLength of output: 3014
Spawn packet logic for living entities verified
The
createSpawnPackets()method in WrapperLivingEntity returns packets for attributes, equipment, and potion effects. WithaddViewernow invoking this method, new viewers will correctly receive equipment and potion-effect data. No further changes needed.
common/src/main/java/me/tofaa/entitylib/common/AbstractEntityLibAPI.java
Outdated
Show resolved
Hide resolved
api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntityPotionEffect.java
Outdated
Show resolved
Hide resolved
api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntityPotionEffect.java
Show resolved
Hide resolved
|
If possible, please keep the prs strictly for one topic as its harder for me to review especially with misleading titles. However, lgtm thank you for your time and efforts |
2 participants
fix the problems when the entity is spawned and a new viewer is added, it does not send the equipment and potions again
Summary by CodeRabbit
New Features
Improvements
Style