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 ✨ Finishing Touches
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 (
|
|
@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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillReleasePacket.java (1)
48-69: Improved skill release handling with event system integration.The refactored code offers several improvements:
- Integration with Forge event system using
SkillReleaseEvent- More streamlined conditional logic
- Proper handling of skill instance existence and interaction
This implementation is cleaner and better supports mod compatibility by using the Forge event system.
Consider extracting the skill removal logic into a separate method for better readability:
- skillInstance.removeHeldAttributeModifiers(player); - Multimap<UUID, TickingSkill> multimap = TickEventListenerHandler.tickingSkills; - if (multimap.containsKey(player.getUUID())) - multimap.get(player.getUUID()).removeIf(tickingSkill -> tickingSkill.getSkill() == skillInstance.getSkill()); - storage.syncChanges(); + cleanupSkillEffects(player, skillInstance, storage);With a helper method:
private void cleanupSkillEffects(ServerPlayer player, ManasSkillInstance skillInstance, SkillStorage storage) { skillInstance.removeHeldAttributeModifiers(player); Multimap<UUID, TickingSkill> multimap = TickEventListenerHandler.tickingSkills; if (multimap.containsKey(player.getUUID())) multimap.get(player.getUUID()).removeIf(tickingSkill -> tickingSkill.getSkill() == skillInstance.getSkill()); storage.syncChanges(); }src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillActivationPacket.java (1)
42-60: Improved skill activation with clean event-based handling.The refactored code offers several improvements:
- Integration with the Forge event system using
SkillActivationEvent- More straightforward conditional flow
- Better separation of concerns
This change aligns with the PR objective of improving compatibility with other mods by leveraging the Forge event system.
Consider extracting the skill activation logic into a separate method for better readability:
- instance.onPressed(player); - instance.addHeldAttributeModifiers(player); - TickEventListenerHandler.tickingSkills.put(player.getUUID(), new TickingSkill(instance.getSkill())); - storage.syncChanges(); + activateSkill(player, instance, storage);With a helper method:
private void activateSkill(ServerPlayer player, ManasSkillInstance instance, SkillStorage storage) { instance.onPressed(player); instance.addHeldAttributeModifiers(player); TickEventListenerHandler.tickingSkills.put(player.getUUID(), new TickingSkill(instance.getSkill())); storage.syncChanges(); }src/main/java/com/github/manasmods/manascore/capability/skill/event/InternalSkillPacketActions.java (1)
19-22: Update method documentation to reflect current implementationThe method comments still mention filtering skills that meet certain conditions, which is inconsistent with the actual implementation that now handles a single skill directly.
- * This Method filters {@link ManasSkill} that meets the conditions of the {@link SkillActivationEvent} then send packet for them. + * This Method sends a packet for the specified skill to handle {@link SkillActivationEvent}.Similar updates should be made to the other method comments.
Also applies to: 30-33, 41-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
gradle.properties(1 hunks)src/main/java/com/github/manasmods/manascore/api/skills/SkillAPI.java(3 hunks)src/main/java/com/github/manasmods/manascore/capability/skill/TestSkill.java(3 hunks)src/main/java/com/github/manasmods/manascore/capability/skill/event/InternalSkillPacketActions.java(1 hunks)src/main/java/com/github/manasmods/manascore/client/keybinding/KeybindingRegistry.java(2 hunks)src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillActivationPacket.java(2 hunks)src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillReleasePacket.java(2 hunks)src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillTogglePacket.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/github/manasmods/manascore/capability/skill/TestSkill.java
- gradle.properties
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (16)
src/main/java/com/github/manasmods/manascore/api/skills/SkillAPI.java (3)
64-65: Improved API signature for explicit skill targeting.The addition of the
ResourceLocation skillparameter makes the API more explicit and type-safe compared to relying on key numbers alone.
74-75: Enhanced skill toggle API with explicit skill targeting.This change brings the method in line with the other skill packet methods, providing consistent parameter patterns across the API.
54-55:Details
❓ Verification inconclusive
API signature change looks good, but ensure all clients are updated.
The method now includes a
ResourceLocation skillparameter, allowing for explicit skill specification instead of relying on key numbers alone. This is a more robust approach.
🏁 Script executed:
#!/bin/bash # Find potential API usage that needs updating # Search for old usage pattern (skillActivationPacket called with just keyNumber) rg -A 1 "skillActivationPacket\s*\(\s*\d+\s*\)" --glob "*.java"Length of output: 65
API Signature Update: Manual Client Verification Required
The changes to
skillActivationPacket—adding theResourceLocation skillparameter alongsideint keyNumber—improve robustness by making the call explicit. Our automated search did not reveal any legacy invocations using only a key number; however, please manually verify that all client code now uses the new two-parameter signature to ensure full compatibility.
- File:
src/main/java/com/github/manasmods/manascore/api/skills/SkillAPI.java(Lines 54‑55)src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillReleasePacket.java (3)
24-25: Simplified packet structure from skill list to single skill.The packet now handles a single skill rather than a list, which aligns with the PR objective of improving compatibility.
31-35: Constructor updated to match new packet structure.The constructor signature has been properly updated to accept a single ResourceLocation instead of a list.
38-39: Serialization properly handles single skill ResourceLocation.The
toBytesmethod has been correctly updated to serialize the single skill.src/main/java/com/github/manasmods/manascore/client/keybinding/KeybindingRegistry.java (2)
39-40: Skill toggle keybinding updated to use ResourceLocation.This change brings the toggle keybinding in line with the other skill keybindings, providing a consistent pattern across the API.
36-37:Details
❓ Verification inconclusive
Updated keybinding to use explicit skill ResourceLocation.
Keybinding now passes a specific skill ResourceLocation instead of just an integer, which aligns with the SkillAPI changes.
For development/test code, consider extracting the test skill ResourceLocation to a constant:
🏁 Script executed:
#!/bin/bash # Check if there are other test skills defined in the codebase that should be updated rg "example_test_skill" --glob "*.java"Length of output: 825
Extract test skill ResourceLocation as a constant for maintainability
The keybinding update now correctly uses an explicit ResourceLocation according to the SkillAPI changes. However, our grep search shows that
"example_test_skill"is used in several places (both inKeybindingRegistry.javaandSkillRegistry.java). To avoid duplication and ease future updates, please consider extracting the test skill's ResourceLocation into a constant. For example:
In
src/main/java/com/github/manasmods/manascore/client/keybinding/KeybindingRegistry.java, define:private static final ResourceLocation EXAMPLE_TEST_SKILL = new ResourceLocation(ManasCore.MOD_ID, "example_test_skill");Then update the invocations for:
SkillAPI.skillActivationPacketSkillAPI.skillReleasePacketSkillAPI.skillTogglePacketAlso verify that the usage in
src/main/java/com/github/manasmods/manascore/skill/SkillRegistry.javaremains consistent.src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillActivationPacket.java (3)
21-23: Simplified packet structure to handle a single skill.Moving from a list of skills to a single skill per packet simplifies the code and makes interactions more explicit.
27-29: Constructor signature properly updated for new packet structure.The constructor now correctly accepts a single ResourceLocation instead of a list.
33-34: Packet serialization correctly handles the single skill.The
toBytesmethod has been properly updated to match the new packet structure.src/main/java/com/github/manasmods/manascore/network/toserver/RequestSkillTogglePacket.java (3)
7-7: Architecture change from multiple skills to single skill handlingThe packet has been redesigned to handle a single skill instead of multiple skills at once. This architectural change simplifies the code and likely improves compatibility with other mods by making skill interactions more predictable.
Also applies to: 11-11, 18-18, 23-25, 28-28
36-37: Simplified skill retrieval logicThe skill retrieval has been updated to work with a single skill identifier rather than iterating through a collection. This change is consistent with the architectural shift in the packet design.
40-54: Added event-driven skill toggling with proper conditional checksThe skill toggling logic has been enhanced with:
- Introduction of the
SkillToggleEvent- Event cancellation handling
- Proper conditional checks before toggling
- Appropriate callback methods based on toggle state
This implementation provides better integration points for other mods and follows Forge's event-driven design patterns.
src/main/java/com/github/manasmods/manascore/capability/skill/event/InternalSkillPacketActions.java (2)
23-23: Method signatures updated to handle single skillThe method signatures have been updated to accept a single ResourceLocation instead of presumably handling multiple skills. This change aligns with the architectural changes in RequestSkillTogglePacket.
Also applies to: 34-34, 45-45
27-27: Simplified packet creation for skill actionsThe packet creation logic has been simplified to directly use the provided skill identifier instead of filtering or processing multiple skills. This is consistent with the single-skill architecture change.
Also applies to: 38-38, 49-49
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.