Compatibility Focus #43
Conversation
… Remove Anvil Mixin for compatibility
…t still causing incompatibility with mod of the same version
WalkthroughThis pull request introduces several modifications to the ManasCore mod, focusing on configuration management, skill interactions, and projectile handling. The changes include removing configuration-related classes, updating version specifications, modifying skill and event handling logic, and adjusting mixin configurations. The access transformer has been activated, and the mod ID has been explicitly defined. The overall impact appears to be a refinement of the mod's internal mechanisms and configuration approach. Changes
Sequence DiagramsequenceDiagram
participant Skill as TickingSkill
participant Storage as SkillStorage
participant Entity as LivingEntity
Skill->>Storage: tick(storage, entity)
alt Can interact with entity
Skill->>Entity: Perform skill actions
else Cannot interact
Skill->>Skill: Call onRelease()
Skill-->>Storage: Return false
end
This sequence diagram illustrates the updated 🪧 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/github/manasmods/manascore/api/skills/TickingSkill.java (1)
Line range hint
1-36: Consider documenting API changesSince this class is part of the public API and contains breaking changes in skill behavior, consider:
- Adding documentation about the new skill release behavior
- Updating the changelog to reflect these API changes
- Providing migration guidelines for mod developers
This will help maintain compatibility with other mods, which aligns with the PR's compatibility focus.
src/main/java/com/github/manasmods/manascore/capability/skill/TestSkill.java (1)
107-109: Consider handling specific projectile types separatelyWhile broadening to AbstractArrow improves compatibility, consider if different projectile types need different handling logic (e.g., arrows vs tridents).
Consider implementing a more granular approach:
- if (event.getProjectile() instanceof AbstractArrow) { + if (event.getProjectile() instanceof AbstractArrow arrow) { + // Common arrow handling + if (arrow instanceof ThrownTrident) { + // Trident-specific logic if needed + } log.debug("Dodged"); event.setCanceled(true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
build.gradle(1 hunks)gradle.properties(1 hunks)src/main/java/com/github/manasmods/manascore/ManasCore.java(0 hunks)src/main/java/com/github/manasmods/manascore/api/skills/TickingSkill.java(1 hunks)src/main/java/com/github/manasmods/manascore/attribute/ManasCoreAttributeHandler.java(1 hunks)src/main/java/com/github/manasmods/manascore/capability/skill/TestSkill.java(2 hunks)src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java(2 hunks)src/main/java/com/github/manasmods/manascore/config/ManasCoreConfig.java(0 hunks)src/main/java/com/github/manasmods/manascore/core/MixinAnvilMenu.java(0 hunks)src/main/java/com/github/manasmods/manascore/core/MixinAnvilScreen.java(0 hunks)src/main/resources/META-INF/accesstransformer.cfg(1 hunks)src/main/resources/META-INF/mods.toml(2 hunks)src/main/resources/manascore.mixins.json(0 hunks)
💤 Files with no reviewable changes (5)
- src/main/java/com/github/manasmods/manascore/core/MixinAnvilScreen.java
- src/main/java/com/github/manasmods/manascore/core/MixinAnvilMenu.java
- src/main/resources/manascore.mixins.json
- src/main/java/com/github/manasmods/manascore/config/ManasCoreConfig.java
- src/main/java/com/github/manasmods/manascore/ManasCore.java
🔇 Additional comments (15)
src/main/java/com/github/manasmods/manascore/api/skills/TickingSkill.java (3)
34-34: LGTM: Consistent use of getter
The change to use getDuration() instead of direct field access is a good practice and maintains consistency with the class's new style.
9-9: Verify compatibility with existing getter usage
The addition of Lombok's @Getter annotation is a good practice for reducing boilerplate. However, since this is marked as a breaking change PR, ensure that the generated getters maintain the same visibility and behavior as the previously manual implementations.
✅ Verification successful
Based on the search results, I can see that getSkill() and getDuration() methods are used extensively throughout the codebase, particularly in:
- Network packet handling classes
- Capability storage classes
- Event listener handlers
- The
ManasSkillInstanceclass which heavily relies ongetSkill() - The
TickingSkillclass itself using both getters
Let me verify the visibility of these getters to ensure compatibility.
Lombok's @Getter annotation maintains compatibility
The addition of Lombok's @Getter annotation is safe in this context because:
- By default, Lombok generates public getters, which matches the visibility of the existing
getSkill()implementation found in the codebase - The extensive usage of these getters across the codebase will continue to work as the method signatures remain identical
- The generated getters will maintain the same return types and access patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct field access or old getter usage in the codebase
rg -A 2 "getSkill\(\)" --type java
rg -A 2 "getDuration\(\)" --type java
Length of output: 17899
Script:
#!/bin/bash
# Check the original implementation of these getters
ast-grep --pattern 'public $_ getSkill() { $$$ }'
ast-grep --pattern 'public $_ getDuration() { $$$ }'
Length of output: 440
24-27:
Breaking change: New skill release behavior
The addition of instance.onRelease() call when a skill cannot interact with an entity is a significant behavioral change. This could affect existing mods that depend on the previous behavior where skills would silently fail without triggering release events.
src/main/java/com/github/manasmods/manascore/capability/skill/TestSkill.java (1)
16-16: Verify AbstractArrow compatibility across Minecraft versions
The addition of AbstractArrow import broadens projectile handling scope, which aligns with compatibility improvements.
✅ Verification successful
AbstractArrow usage is safe and compatible with Minecraft 1.19.2
The code changes are verified to be compatible:
- AbstractArrow is a stable Minecraft API class used in Forge 1.19.2 (43.3.5)
- Implementation is consistent across the codebase with proper pattern matching and null checks
- Used in standard event handling patterns (ProjectileImpactEvent, EntityJoinLevelEvent)
- No version-specific conditions or implementations that could cause compatibility issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if AbstractArrow is consistently available across supported Minecraft versions
# and verify no conflicts with other mods using similar projectile handling
# Search for other mod interactions with AbstractArrow
rg -l "AbstractArrow" --type java
# Check for potential version-specific implementations
rg "minecraft\.version" --type gradle
Length of output: 336
Script:
#!/bin/bash
# Let's check the actual usage of AbstractArrow in these files to understand the compatibility impact
rg "AbstractArrow" -B 2 -A 2 --type java
# Check build.gradle files for Minecraft version dependencies
fd "build.gradle" --exec cat {}
Length of output: 15789
Script:
#!/bin/bash
# Let's check the gradle.properties file for version information
cat gradle.properties
# Let's also check if there are any version-specific implementations or conditions around AbstractArrow
rg "instanceof.*AbstractArrow" -B 2 -A 2 --type java
Length of output: 2842
src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java (2)
11-11: Verify FastUtil dependency compatibility
The addition of FastUtil's IntOpenHashSet needs version compatibility verification.
Also applies to: 15-15
✅ Verification successful
FastUtil dependency is safe to use - it's a Minecraft/Forge transitive dependency
The IntOpenHashSet is being used correctly in the context of Minecraft's arrow piercing mechanism. This class is available through Minecraft/Forge's dependencies, as evidenced by its usage in AbstractArrow class. The implementation follows Minecraft's standard pattern for handling piercing arrows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check FastUtil dependency declaration and version compatibility
rg -l "fastutil" --type gradle
Length of output: 30
Script:
#!/bin/bash
# Check if FastUtil is declared in any build configuration files with different extensions
rg -l "fastutil" -g "*.gradle*"
# Check if FastUtil is declared in pom.xml (if Maven is used)
fd "pom.xml" --exec rg -l "fastutil" {}
# Check actual usage of IntOpenHashSet in the codebase
rg "IntOpenHashSet" -l
Length of output: 194
Script:
#!/bin/bash
# Check if FastUtil is a transitive dependency from Minecraft/Forge
rg -l "net.minecraftforge" -g "*.gradle*"
rg -l "minecraft" -g "*.gradle*"
# Check the context of IntOpenHashSet usage
rg "IntOpenHashSet" -B 5 -A 5 src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java
Length of output: 1393
154-157:
Review arrow piercing behavior modification
The implementation modifies core arrow piercing behavior which could affect game balance and mod compatibility.
Consider these aspects:
- Thread safety: Multiple skills might modify piercingIgnoreEntityIds concurrently
- Null check: While proper, consider using Optional or initializing in constructor
- Performance: IntOpenHashSet size of 10 might need adjustment based on usage patterns
Suggested implementation:
- if (e.isCanceled() && e.getProjectile() instanceof AbstractArrow arrow) {
- if (arrow.piercingIgnoreEntityIds == null) arrow.piercingIgnoreEntityIds = new IntOpenHashSet(10);
- arrow.piercingIgnoreEntityIds.add(living.getId());
+ if (e.isCanceled() && e.getProjectile() instanceof AbstractArrow arrow) {
+ synchronized(arrow) {
+ if (arrow.piercingIgnoreEntityIds == null) {
+ arrow.piercingIgnoreEntityIds = new IntOpenHashSet(16); // Power of 2 for better performance
+ }
+ arrow.piercingIgnoreEntityIds.add(living.getId());
+ }
+ }gradle.properties (2)
10-10: LGTM: Version bump follows semantic versioning
The patch version increase from 2.1.4.0 to 2.1.4.4 is appropriate for compatibility fixes.
13-13: Verify MixinExtras 0.2.0 compatibility with Forge 1.19.2
While downgrading to 0.2.0 for better mod compatibility is the goal, let's verify this is the optimal version.
✅ Verification successful
Let me gather more information about the specific Mixin usage to understand compatibility requirements better.
Based on the analysis of the codebase, I can now generate the final response:
MixinExtras 0.2.0 is compatible with your Forge 1.19.2 setup
The codebase only uses basic Mixin features (@Inject annotations) without any advanced MixinExtras-specific features like @WrapOperation or @ModifyExpressionValue. Version 0.2.0 fully supports these basic injection points, making it a safe choice for your mod's compatibility goals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check MixinExtras version compatibility with Forge 1.19.2
# Search for MixinExtras version references in the codebase
echo "Searching for MixinExtras usage patterns..."
rg -l "org\.spongepowered\.asm\.mixin"
# Check build files for version constraints
echo "Checking build files for version constraints..."
rg "mixinExtras" --type gradle
Length of output: 1174
Script:
#!/bin/bash
# Check for specific MixinExtras features usage that might affect version compatibility
echo "Checking MixinExtras feature usage..."
rg "import com.llamalad7.mixinextras" -A 2
# Check for @WrapOperation, @ModifyExpressionValue and other MixinExtras annotations
echo "Checking for MixinExtras annotations..."
rg "@(WrapOperation|ModifyExpressionValue|Inject|ModifyVariable)" src/main/java/com/github/manasmods/manascore/core/
# Check mod metadata for Forge version
echo "Checking Forge version in mods.toml..."
cat src/main/resources/META-INF/mods.toml
Length of output: 2291
src/main/resources/META-INF/accesstransformer.cfg (1)
1-1: Verify AbstractArrow field mapping across Minecraft versions
The access transformer entry is correctly formatted. However, ensure the field mapping f_36701_ is consistent across Minecraft 1.19.2 installations.
✅ Verification successful
The search results show consistent usage of the field mapping across the codebase. The field f_36701_ (piercingIgnoreEntityIds) is referenced in the code using its deobfuscated name and matches with the access transformer entry. Let me verify if this is the only access transformer entry and check for any version-specific configurations.
Let me check the specific Minecraft version and verify the build configuration for the access transformer.
Access transformer configuration is correctly mapped for Minecraft 1.19.2
The field mapping is consistent with Minecraft 1.19.2 (confirmed via gradle.properties), and the access transformer is properly configured in the build system. The field is correctly used in the codebase with its deobfuscated name piercingIgnoreEntityIds, matching the AT entry's mapping comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify field mapping consistency
# Search for other references to this field
echo "Checking for other references to AbstractArrow field..."
rg "f_36701_|piercingIgnoreEntityIds" --type java
# Look for any existing access transformer entries
echo "Checking for existing access transformer entries..."
fd -t f "accesstransformer.cfg" -x cat {}
Length of output: 805
Script:
#!/bin/bash
# Check for any version-specific configurations or build files
echo "Checking build.gradle for Minecraft version..."
rg "minecraft" build.gradle
# Check for any other access transformer configurations
echo "Checking for any other AT configurations..."
fd -e toml -e properties -e gradle -x rg -l "accesstransformer"
Length of output: 1649
Script:
#!/bin/bash
# Check gradle.properties for version information
echo "Checking gradle.properties for version information..."
cat gradle.properties
# Check build.gradle for access transformer configuration
echo "Checking build.gradle for AT configuration..."
rg -A 5 "accesstransformer" build.gradle
Length of output: 972
src/main/resources/META-INF/mods.toml (2)
6-6: LGTM: Explicit modId is more reliable than placeholder
Using "manascore" directly instead of "${modId}" reduces potential configuration issues.
18-24: Verify dependency configuration completeness
The dependencies section correctly defines Forge and Minecraft requirements. However, with the removal of MixinExtras dependency, let's verify if any other mod dependencies need to be declared.
Also applies to: 25-32
✅ Verification successful
Based on the imports analysis, I can see that the codebase primarily depends on Forge and Minecraft core functionality. Let me verify the mods.toml content to ensure we have a complete picture.
Dependencies are correctly configured - no additional dependencies required
The mods.toml file correctly declares all necessary dependencies:
- Forge dependency with proper version range
- Minecraft dependency with version check
- No other dependencies are required as the codebase only uses Forge and Minecraft core functionality, as evidenced by the import analysis which shows only usage of
net.minecraft,net.minecraftforge, and internalcom.github.manasmodspackages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential missing dependencies
# Search for import statements in Java files
echo "Checking for mod-related imports..."
rg "^import.*\b(net\.minecraft|net\.minecraftforge|com\.github\.manasmods)\b" --type java
# Look for other mods.toml files in the project
echo "Checking for other mods.toml files..."
fd -t f "mods.toml" -x cat {}
Length of output: 65926
Script:
#!/bin/bash
# Get the content of mods.toml file
cat src/main/resources/META-INF/mods.toml
Length of output: 1169
src/main/java/com/github/manasmods/manascore/attribute/ManasCoreAttributeHandler.java (2)
90-93: LGTM: Fixed the double critical multiplier issue
The changes correctly address the issue where the 1.5x multiplier was being applied twice. The new implementation:
- Captures the vanilla multiplier ratio
- Combines it with the crit multiplier only once
This fix aligns with the PR objective of correcting the Critical Multiplier calculation.
98-98: Verify the critical hit calculation
The event creation now uses the product of vanillaMultiplier and critMultiplier. Let's verify this doesn't affect other mods' critical hit calculations.
✅ Verification successful
Let me check the implementation details of these critical chance events and handlers to better understand the impact.
Critical hit calculation is correctly implemented and safe
The implementation is safe because:
- The
CriticalChanceEventis a custom event that encapsulates both the damage modifier and crit chance - Other mods can modify or cancel the event through the Forge event bus
- The vanilla critical hit multiplier is properly combined with the mod's multiplier, maintaining compatibility
- The event structure allows for clean separation between vanilla and mod-added critical hit mechanics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other mods' critical hit event handlers
rg -l "CriticalHitEvent|CriticalChanceEvent" --type java
# Search for references to our critical multiplier attribute
ast-grep --pattern 'ManasCoreAttributes.CRIT_MULTIPLIER'
Length of output: 386
Script:
#!/bin/bash
# Check the implementation of CriticalChanceEvent
cat src/main/java/com/github/manasmods/manascore/api/attribute/event/CriticalChanceEvent.java
# Check how the critical multiplier is used in the attribute handler
rg -A 10 "critMultiplier" src/main/java/com/github/manasmods/manascore/attribute/ManasCoreAttributeHandler.java
Length of output: 2772
build.gradle (2)
38-38: Verify MixinExtra version downgrade
The PR objectives mention downgrading MixinExtra from 0.3.2 to 0.2.0, but this change isn't visible in the current diff.
38-38: Verify the access transformer configuration
The access transformer has been activated, which aligns with the compatibility focus. However, we should verify:
- The existence of the access transformer configuration file
- The impact on mod compatibility
✅ Verification successful
Access transformer configuration is present and properly configured
The access transformer file exists at the specified path and contains a valid configuration for making the piercingIgnoreEntityIds field in AbstractArrow class public, which is a legitimate use case for mod compatibility.
- The configuration targets a specific Minecraft entity field that's likely needed for the mod's functionality
- The syntax follows the standard Forge access transformer format
- The configuration aligns with the compatibility focus mentioned in the PR objective
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the access transformer file exists
if [ -f "src/main/resources/META-INF/accesstransformer.cfg" ]; then
echo "Access transformer file exists"
cat "src/main/resources/META-INF/accesstransformer.cfg"
else
echo "Access transformer file is missing"
fi
# Search for any other access transformer references
rg -l "accesstransformer" --type java --type properties
Length of output: 373
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillReleasePacket.java (1)
55-59: Ensure robust cooldown logic around skill releases
These lines gracefully handle the distinction between a skill on cooldown and a skill that can ignore cooldown. The nested condition is clear. However, consider adding logging or a hint to players when a skill is on cooldown and they attempt to release it.src/main/java/com/github/manasmods/manascore/api/skills/TickingSkill.java (3)
6-6: Optional: Remove the direct import of @nullable if already present
If@Nullableis already provided by another import or not enforced by your static analysis, you can omit it to reduce clutter. That said, explicit @nullable usage sometimes aids readability.
18-21: Return an Optional instead of @nullable?
This method returnsnullif the skill instance is absent, but an Optional conveys "missing instance" more explicitly. Consider returning anOptional<ManasSkillInstance>for safer usage.-public @Nullable ManasSkillInstance getSkillInstance(SkillStorage storage, LivingEntity entity) { - ... - return optional.orElse(null); -} +public Optional<ManasSkillInstance> getSkillInstance(SkillStorage storage, LivingEntity entity) { + return storage.getSkill(this.getSkill()); +}
23-31: Enhance clarity around forced release
When!instance.canInteractSkill(entity), forcibly releasing the skill is a nice approach. Optionally, consider a logging or feedback mechanism that clarifies why the skill was forcibly released.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gradle.properties(1 hunks)src/main/java/com/github/manasmods/manascore/api/skills/ManasSkill.java(1 hunks)src/main/java/com/github/manasmods/manascore/api/skills/ManasSkillInstance.java(1 hunks)src/main/java/com/github/manasmods/manascore/api/skills/TickingSkill.java(1 hunks)src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java(3 hunks)src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillReleasePacket.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- gradle.properties
- src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java
🔇 Additional comments (4)
src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillReleasePacket.java (1)
61-61: Double-check the invocation of removeHeldAttributeModifiers
Calling skillInstance.removeHeldAttributeModifiers(player) unconditionally ensures modifiers are always removed after the release attempt. This is generally correct, but verify that no leftover scenarios require the removal to be conditional (e.g., if the skill never triggered).
src/main/java/com/github/manasmods/manascore/api/skills/ManasSkill.java (1)
229-231: Signature update for removeHeldAttributeModifiers
Accepting ManasSkillInstance as an additional parameter is a good enhancement, providing context for individualized logic per skill instance. Ensure that any usage outside the immediate scope is updated to avoid signature mismatches.
src/main/java/com/github/manasmods/manascore/api/skills/TickingSkill.java (2)
10-10: @Getter usage for the entire class
Using Lombok’s @Getter at the class level is clean. Confirm that you no longer need any custom getter logic for the skill field (previously handled manually).
38-38: Consistent usage of getters
Continuing to use this.getDuration() fosters consistency. This is good, but watch for performance overhead if getters do more complex logic in the future.
| /** | ||
| * Removes the attribute modifiers of this instance from the {@link LivingEntity} holding the skill activation button. | ||
| * | ||
| * @param entity Affected {@link LivingEntity} owning this Skill. | ||
| */ | ||
| public void removeHeldAttributeModifiers(LivingEntity entity) { | ||
| this.getSkill().addHeldAttributeModifiers(this, entity); | ||
| } | ||
|
|
There was a problem hiding this comment.
Possible oversight in removeHeldAttributeModifiers
The current implementation calls addHeldAttributeModifiers instead of actually removing modifiers. This appears to be an accidental call. Consider reversing the call to the newly updated ManasSkill#removeHeldAttributeModifiers(this, entity) to ensure the modifiers are correctly removed.
Suggested fix:
- public void removeHeldAttributeModifiers(LivingEntity entity) {
- this.getSkill().addHeldAttributeModifiers(this, entity);
- }
+ public void removeHeldAttributeModifiers(LivingEntity entity) {
+ this.getSkill().removeHeldAttributeModifiers(this, entity);
+ }📝 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.
| /** | |
| * Removes the attribute modifiers of this instance from the {@link LivingEntity} holding the skill activation button. | |
| * | |
| * @param entity Affected {@link LivingEntity} owning this Skill. | |
| */ | |
| public void removeHeldAttributeModifiers(LivingEntity entity) { | |
| this.getSkill().addHeldAttributeModifiers(this, entity); | |
| } | |
| /** | |
| * Removes the attribute modifiers of this instance from the {@link LivingEntity} holding the skill activation button. | |
| * | |
| * @param entity Affected {@link LivingEntity} owning this Skill. | |
| */ | |
| public void removeHeldAttributeModifiers(LivingEntity entity) { | |
| this.getSkill().removeHeldAttributeModifiers(this, entity); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java (1)
234-245: Consider refactoring duplicated cleanup logicThe dimension change handler duplicates the logic from the logout handler. Consider extracting the common cleanup code into a private method.
+ private static void cleanupPlayerSkills(Player player) { + Multimap<UUID, TickingSkill> multimap = TickEventListenerHandler.tickingSkills; + if (multimap.containsKey(player.getUUID())) { + for (TickingSkill tickingSkill : multimap.get(player.getUUID())) { + ManasSkillInstance instance = tickingSkill.getSkillInstance(SkillAPI.getSkillsFrom(player), player); + if (instance == null) continue; + instance.removeHeldAttributeModifiers(player); + } + multimap.removeAll(player.getUUID()); + } + } @SubscribeEvent public static void onLogOut(final PlayerEvent.PlayerLoggedOutEvent e) { - Player player = e.getEntity(); - Multimap<UUID, TickingSkill> multimap = TickEventListenerHandler.tickingSkills; - if (multimap.containsKey(player.getUUID())) { - for (TickingSkill tickingSkill : multimap.get(player.getUUID())) { - ManasSkillInstance instance = tickingSkill.getSkillInstance(SkillAPI.getSkillsFrom(player), player); - if (instance == null) continue; - instance.removeHeldAttributeModifiers(player); - } - multimap.removeAll(player.getUUID()); - } + cleanupPlayerSkills(e.getEntity()); } @SubscribeEvent public static void onChangingDimension(final PlayerEvent.PlayerChangedDimensionEvent e) { - Player player = e.getEntity(); - Multimap<UUID, TickingSkill> multimap = TickEventListenerHandler.tickingSkills; - if (multimap.containsKey(player.getUUID())) { - for (TickingSkill tickingSkill : multimap.get(player.getUUID())) { - ManasSkillInstance instance = tickingSkill.getSkillInstance(SkillAPI.getSkillsFrom(player), player); - if (instance == null) continue; - instance.removeHeldAttributeModifiers(player); - } - multimap.removeAll(player.getUUID()); - } + cleanupPlayerSkills(e.getEntity()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/manasmods/manascore/api/skills/ManasSkillInstance.java(1 hunks)src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java(3 hunks)
🔇 Additional comments (3)
src/main/java/com/github/manasmods/manascore/capability/skill/event/ServerEventListenerHandler.java (2)
154-157: LGTM: Proper handling of piercing arrows
The implementation correctly prevents piercing arrows from hitting the same entity multiple times by maintaining a set of ignored entity IDs.
225-232: LGTM: Proper cleanup of attribute modifiers on logout
The implementation correctly retrieves and cleans up skill instances, ensuring attribute modifiers are properly removed when a player logs out.
src/main/java/com/github/manasmods/manascore/api/skills/ManasSkillInstance.java (1)
396-404: LGTM: Fixed attribute modifier removal
The implementation now correctly delegates to the skill's removeHeldAttributeModifiers method, fixing the issue mentioned in previous reviews where it was incorrectly calling addHeldAttributeModifiers.
Description
Critical Multiplier bugged + Downgrade MixinExtra for compatibility + Remove Anvil Mixin for compatibility
List of changes
Type of Change
Summary by CodeRabbit
New Features
ManasSkillInstance.Bug Fixes
Documentation
mods.tomlandaccesstransformer.cfg.Chores
gradle.properties.