Registry - Keybinding - Skill - Config modules#48
Conversation
…oo small to fit into a future module
# Conflicts: # settings.gradle # testing-common/build.gradle
Idk how to check out forked branch
# Conflicts: # settings.gradle # testing-common/build.gradle # testing-common/src/main/java/io/github/manasmods/manascore/testing/ManasCoreTesting.java
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds multiple new subsystems (config, skill, race, attribute, keybind), expanded command argument API, platform entrypoints for Fabric/NeoForge, network payloads for C2S/S2C sync, many mixins/access-wideners/resources, testing/demo content, and extensive Gradle/build/publishing changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Internal as InternalSkillPacketActions
participant Network as NetworkManager
participant Server
participant Storage as SkillStorage
Client->>Internal: handle input (key/scroll)
Internal->>Network: send RequestSkillActivation/Release/Scroll/Toggle
Network->>Server: deliver packet
Server->>Storage: lookup player's Skills
Storage->>Storage: validate via SkillEvents (permission/mode/cooldown)
Storage->>Storage: update ManasSkillInstance / add TickingSkill
Storage->>Server: mark dirty / persist / schedule sync
sequenceDiagram
autonumber
participant Server
participant Net as ManasConfigNetwork
participant Payload as SyncConfigToClientPayload
participant Client
participant Registry as ConfigRegistry
Server->>Registry: getConfigSyncData()
Server->>Net: syncToClients()
Net->>Payload: build payload(map)
Net->>Client: send S2C SyncConfigToClientPayload
Client->>Payload: receive & decode
Payload->>Registry: loadConfigSyncData(map)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 47
🧹 Nitpick comments (50)
skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillTogglePacket.java (1)
22-26: Consider making the skillList parameter immutable.While records provide immutable fields, the List itself can still be modified. Consider using
ImmutableListorList.copyOf()in the constructor to prevent external modifications.public record RequestSkillTogglePacket( - List<ResourceLocation> skillList + List<ResourceLocation> skillList ) implements CustomPacketPayload { + public RequestSkillTogglePacket { + skillList = List.copyOf(skillList); // Make defensive copy + }skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillReleasePacket.java (1)
22-26: Add Javadoc to document the record fields.Please add documentation to explain:
- What
heldTickrepresents (duration of skill hold?)- Purpose of
keyNumber(keybinding identifier?)- Format/requirements for
skillListkeybinding-fabric/src/main/resources/fabric.mod.json (1)
1-14: Add homepage URL for better discoverability.The homepage URL is currently empty. Consider adding a link to your GitHub repository or documentation page to help users find more information about the mod.
skill-common/src/main/java/io/github/manasmods/manascore/skill/api/AbstractSkillRegister.java (4)
15-20: Consider eager initialization of the skills register.The current implementation initializes
skillsas null and performs lazy initialization in theskill()method. This pattern could lead to potential race conditions in concurrent scenarios and makes the code harder to reason about.Consider initializing the register in the constructor:
- protected DeferredRegister<ManasSkill> skills = null; + protected final DeferredRegister<ManasSkill> skills; public AbstractSkillRegister(String modId) { super(modId); + this.skills = DeferredRegister.create(modId, SkillAPI.getSkillRegistryKey()); }
21-24: Add method documentation.The
initmethod lacks documentation explaining its purpose, contract with the parent class, and the significance of thebeforeRegistrationparameter.Add Javadoc explaining:
- Method's purpose
- When it's called
- What
beforeRegistrationdoes- Order of operations (super.init followed by skills registration)
+ /** + * Initializes the skill register by executing any pre-registration tasks and registering skills. + * @param beforeRegistration Runnable to execute before registration occurs + */ public void init(final Runnable beforeRegistration) {
34-51: Document the SkillBuilder class and its methods.The inner class and its methods lack documentation explaining their purpose and usage.
Add comprehensive documentation:
+ /** + * Builder class for creating and configuring skill registrations. + * @param <R> The type of the parent register + * @param <T> The type of skill being built + */ public static class SkillBuilder<R extends AbstractSkillRegister<R>, T extends ManasSkill> extends ContentBuilder<T, R> { protected final Supplier<T> skillFactory; + /** + * Creates a new SkillBuilder instance. + * @param register The parent register + * @param name The unique identifier for the skill + * @param skillFactory Factory for creating skill instances + */ private SkillBuilder(R register, String name, Supplier<T> skillFactory) { + /** + * Finalizes the skill registration and returns a registry supplier. + * @return A RegistrySupplier for the registered skill + */ @Override public RegistrySupplier<T> end() { + /** + * Finalizes the skill registration and returns a holder. + * @return A Holder for the registered skill + */ @Override public Holder<T> endAsHolder() {
1-52: Well-designed skill registration system.The implementation provides a robust foundation for skill registration in a modding context. The use of the builder pattern, generic types, and separation of concerns demonstrates good architectural decisions. The class effectively leverages the Architectury API for cross-platform compatibility.
Consider these architectural improvements:
- Add an interface defining the contract for skill registration
- Consider using a factory interface for skill creation to allow for different implementation strategies
- Add events or hooks for registration lifecycle (pre/post registration)
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/ManasCoreSkillFabric.java (1)
11-16: Consider adding error handling and logging.While the implementation follows Fabric's initialization pattern correctly, consider these improvements for better robustness:
- Add try-catch block to handle potential initialization errors
- Add logging to indicate successful initialization
- Consider adding initialization state verification
Example implementation:
public class ManasCoreSkillFabric implements ModInitializer { + private static final org.slf4j.Logger LOGGER = org.slf4j.LoggerFactory.getLogger("manascore-skill"); @Override public void onInitialize() { - ManasCoreSkill.init(); + try { + LOGGER.info("Initializing ManasCoreSkill module..."); + ManasCoreSkill.init(); + LOGGER.info("ManasCoreSkill module initialized successfully"); + } catch (Exception e) { + LOGGER.error("Failed to initialize ManasCoreSkill module", e); + throw e; // Rethrow to ensure Fabric is aware of initialization failure + } } }skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillActivationPacket.java (1)
50-54: Clarify variable usage ofkeyNumberandmodefor better readability.In the code,
keyNumberis assigned tomode, and both variables are used subsequently. This might cause confusion since they appear to represent the same concept.Consider renaming
keyNumbertomodeif they are intended to be the same, or differentiate them clearly if they represent different concepts. This will improve code readability and maintainability.skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFishingHook.java (1)
23-23: Specify access modifier for the methodThe method
onHitcurrently has package-private visibility. Consider specifying an access modifier (e.g.,privateorpublic) to clarify its intended visibility and adhere to best practices.Apply this diff to specify the access modifier:
- ProjectileDeflection onHit(FishingHook instance, HitResult result, Operation<ProjectileDeflection> original) { + private ProjectileDeflection onHit(FishingHook instance, HitResult result, Operation<ProjectileDeflection> original) {skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinPlayer.java (1)
25-25: Simplify casting of 'this' referenceThe cast
(LivingEntity) (Object) thiscan be simplified to(LivingEntity) thissincePlayerextendsLivingEntity. The extra cast toObjectis unnecessary and can be removed for clarity.Apply this diff to simplify the casting:
- if (EntityEvents.LIVING_HURT.invoker().hurt((LivingEntity) (Object) this, damageSource, changeable).isFalse()) return 0.0F; + if (EntityEvents.LIVING_HURT.invoker().hurt((LivingEntity) this, damageSource, changeable).isFalse()) return 0.0F;skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractHurtingProjectile.java (1)
23-23: Specify access modifier for the methodThe method
onHitcurrently has package-private visibility. Consider specifying an access modifier (e.g.,privateorpublic) to clarify its intended visibility and adhere to best practices.Apply this diff to specify the access modifier:
- ProjectileDeflection onHit(AbstractHurtingProjectile instance, HitResult result, Operation<ProjectileDeflection> original) { + private ProjectileDeflection onHit(AbstractHurtingProjectile instance, HitResult result, Operation<ProjectileDeflection> original) {skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java (5)
41-43: Static Fields Should Befinalif They Are Meant to Be ConstantsThe
keyfield is static but not marked asfinal. Ifkeyis intended to be a constant, it should be declared asstatic final.Apply this diff to make
keya constant:-private static StorageKey<SkillStorage> key = null; +private static final StorageKey<SkillStorage> key = null;
88-89: Unnecessary Return After CheckThe method
checkPlayerOnlyEffectsreturns immediately ifpassiveSkillActivateis false, but there's an additional return after the condition which is redundant.Simplify the code by removing the unnecessary return statement.
170-186: Possible Duplicate Skill RegistrationIn the
learnSkillmethod, if a skill instance already exists, the method logs a debug message and returnsfalse. However, it may be helpful to inform the player that they already have the skill.Consider sending a message to the player indicating they have already learned the skill.
245-252: Exception Handling Could Be More SpecificCatching a general
Exceptionin a loop could mask other potential issues.Apply this diff to catch a more specific exception:
try { ManasSkillInstance instance = ManasSkillInstance.fromNBT((CompoundTag) tag); this.skillInstances.put(instance.getSkillId(), instance); -} catch (Exception e) { +} catch (NBTException e) { ManasCoreSkill.LOG.error("Failed to load skill instance from NBT", e); }
265-266: Typo in Method NameresetDirtyThe method
resetDirtymight be better namedmarkCleanto match common conventions.Consider renaming the method for clarity.
skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkill.java (1)
429-450: Remove redundant constructor and accessors in record.In Java records, the canonical constructor and accessor methods are generated automatically. Explicitly redefining them is unnecessary unless you need custom behavior.
You can simplify the
AttributeTemplaterecord by removing the redundant constructor and accessor methods:public static record AttributeTemplate(ResourceLocation id, double amount, AttributeModifier.Operation operation) { - public AttributeTemplate(ResourceLocation id, double amount, AttributeModifier.Operation operation) { - this.id = id; - this.amount = amount; - this.operation = operation; - } public AttributeModifier create(double i) { return new AttributeModifier(this.id, this.amount * i, this.operation); } - - public ResourceLocation id() { - return this.id; - } - - public double amount() { - return this.amount; - } - - public AttributeModifier.Operation operation() { - return this.operation; - } }skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkillInstance.java (1)
508-510: Clarify the method documentation for damage events.The comment for
onBeingDamagedmentions it is executed beforeonDamageEntity, but in practice,onBeingDamagedis typically called when the entity takes damage, not before it deals damage. Ensure the comments accurately reflect the method's behavior.Consider updating the comment for clarity:
/** - * Gets executed before {@link ManasSkillInstance#onDamageEntity} + * Called when the entity is being damaged. This method is executed before damage is applied. * * @return false will prevent the owner from taking damage. */registry-common/src/main/java/io/github/manasmods/manascore/registry/api/AbstractRegister.java (1)
549-550: Use consistent attribute names in registration.Constructing the attribute name using string formatting can lead to inconsistencies. Consider using
this.id.toString()to ensure the attribute name matches the registry ID.Apply this diff for consistency:
- String name = String.format("%s.attribute.%s", this.id.getNamespace(), this.id.getPath().replaceAll("/", ".")); + String name = this.id.toString();config-common/src/main/java/io/github/manasmods/manascore/config/api/ManasConfig.java (1)
3-7: Add documentation and consider making the path configurable.The abstract class would benefit from:
- JavaDoc explaining its purpose and usage
- Making the parent path configurable through constructor/setter
- Path validation and normalization
Consider this improvement:
+/** + * Base configuration class that provides common functionality for mod configurations. + * Extend this class to create specific configuration handlers for different mod components. + */ public abstract class ManasConfig { + private final String parentPath; + + protected ManasConfig(String parentPath) { + this.parentPath = parentPath != null ? parentPath : "config"; + } + + protected ManasConfig() { + this("config"); + } + public String getParentPath() { - return "config"; + return parentPath; } }registry-common/src/main/java/io/github/manasmods/manascore/registry/ManasCoreRegistry.java (1)
10-14: Add documentation and error handling.The registry initialization would benefit from:
- Class-level JavaDoc explaining its purpose
- Error handling for initialization failures
- Logging for debugging purposes
Consider this improvement:
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Core registry initialization for ManasCore. + * Handles the initialization of all registry components. + */ public class ManasCoreRegistry { + private static final Logger LOGGER = LoggerFactory.getLogger(ManasCoreRegistry.class); + public static void init() { + LOGGER.info("Initializing ManasCore Registry"); + try { ManasAttributeRegistry.init(); + LOGGER.info("ManasCore Registry initialization completed"); + } catch (Exception e) { + LOGGER.error("Failed to initialize ManasCore Registry", e); + throw new RuntimeException("Registry initialization failed", e); + } } }registry-fabric/src/main/java/io/github/manasmods/manascore/registry/ManasCoreRegistryFabric.java (1)
10-15: Add documentation and error handling.The Fabric initializer would benefit from:
- Class-level JavaDoc explaining its purpose
- Error handling for initialization failures
- Logging for debugging purposes
Consider this improvement:
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Fabric-specific initialization for ManasCore Registry. + * Implements ModInitializer to hook into Fabric's mod loading lifecycle. + */ public class ManasCoreRegistryFabric implements ModInitializer { + private static final Logger LOGGER = LoggerFactory.getLogger(ManasCoreRegistryFabric.class); + @Override public void onInitialize() { + LOGGER.info("Initializing ManasCore Registry for Fabric"); + try { ManasCoreRegistry.init(); + LOGGER.info("ManasCore Registry initialization completed for Fabric"); + } catch (Exception e) { + LOGGER.error("Failed to initialize ManasCore Registry for Fabric", e); + throw new RuntimeException("Fabric registry initialization failed", e); + } } }registry-neoforge/src/main/java/io/github/manasmods/manascore/registry/ManasCoreRegistryNeoForge.java (1)
12-14: Consider adding error handling during initialization.The constructor calls
ManasCoreRegistry.init()directly without any error handling. Consider wrapping the initialization in a try-catch block to gracefully handle potential initialization failures.public ManasCoreRegistryNeoForge() { - ManasCoreRegistry.init(); + try { + ManasCoreRegistry.init(); + } catch (Exception e) { + // Use a logging framework instead of System.err in production + System.err.println("Failed to initialize ManasCoreRegistry: " + e.getMessage()); + throw new RuntimeException("Failed to initialize mod", e); + } }keybinding-fabric/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingFabric.java (1)
10-15: Maintain consistency with NeoForge implementation.For consistency with the NeoForge implementation:
- The class should be marked as
finalto prevent inheritance- Consider adding error handling during initialization
-public class ManasCoreKeybindingFabric implements ModInitializer { +public final class ManasCoreKeybindingFabric implements ModInitializer { @Override public void onInitialize() { - ManasCoreKeybinding.init(); + try { + ManasCoreKeybinding.init(); + } catch (Exception e) { + // Use a logging framework instead of System.err in production + System.err.println("Failed to initialize ManasCoreKeybinding: " + e.getMessage()); + throw new RuntimeException("Failed to initialize mod", e); + } } }keybinding-neoforge/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingNeoForge.java (1)
12-14: Consider adding error handling during initialization.The constructor calls
ManasCoreKeybinding.init()directly without any error handling. Consider wrapping the initialization in a try-catch block to gracefully handle potential initialization failures.public ManasCoreKeybindingNeoForge() { - ManasCoreKeybinding.init(); + try { + ManasCoreKeybinding.init(); + } catch (Exception e) { + // Use a logging framework instead of System.err in production + System.err.println("Failed to initialize ManasCoreKeybinding: " + e.getMessage()); + throw new RuntimeException("Failed to initialize mod", e); + } }keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybinding.java (1)
11-17: Strengthen implementation with defensive programming.Consider the following improvements:
- Mark the class as
finalsince it's not designed for inheritance- Add null check for
Platform.getEnvironment()-public class ManasCoreKeybinding { +public final class ManasCoreKeybinding { public static void init() { - if (Platform.getEnvironment() == Env.CLIENT) { + Env currentEnv = Platform.getEnvironment(); + if (currentEnv != null && currentEnv == Env.CLIENT) { ManasCoreKeybindingClient.init(); } } }keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingClient.java (1)
11-15: LGTM! Consider adding class-level documentation.The initialization logic and event registration are well-implemented. Consider adding class-level documentation to explain the client-side responsibility of this class.
Add JavaDoc to explain the class's role:
/** * Client-side initialization handler for the ManasCore keybinding system. * Registers necessary client lifecycle events and initializes the KeybindingManager. */keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingCategory.java (1)
11-18: Add input validation and improve documentation.The class looks good but could benefit from some improvements:
- Add validation for the name parameter
- Enhance documentation for API users
@RequiredArgsConstructor(staticName = "of") +/** + * Represents a category for grouping related keybindings. + * Categories are used to organize keybindings in the game's controls menu. + */ public class KeybindingCategory { private final String name; + /** + * Creates a localization key for this category. + * @return A string in the format "modid.category.categoryname" + */ public String getCategoryString() { + Objects.requireNonNull(name, "Category name cannot be null"); + if (name.isEmpty()) { + throw new IllegalArgumentException("Category name cannot be empty"); + } return String.format("%s.category.%s", ModuleConstants.MOD_ID, this.name); } }skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkill.java (1)
19-26: Document initialization order and add error handling.The initialization sequence looks good, but consider:
- Documenting the required initialization order
- Adding error handling for initialization failures
+ /** + * Initializes the skill system components in the following order: + * 1. SkillRegistry - Registers all available skills + * 2. SkillStorage - Sets up skill data persistence + * 3. ManasSkillNetwork - Initializes networking for skill synchronization + * 4. Client-side initialization (if applicable) + * + * @throws IllegalStateException if any component fails to initialize + */ public static void init() { - SkillRegistry.init(); - SkillStorage.init(); - ManasSkillNetwork.init(); + try { + SkillRegistry.init(); + SkillStorage.init(); + ManasSkillNetwork.init(); + } catch (Exception e) { + LOG.error("Failed to initialize skill system", e); + throw new IllegalStateException("Skill system initialization failed", e); + } if (Platform.getEnvironment() == Env.CLIENT) { - ManasCoreSkillClient.init(); + try { + ManasCoreSkillClient.init(); + } catch (Exception e) { + LOG.error("Failed to initialize client-side skill system", e); + throw new IllegalStateException("Client-side skill system initialization failed", e); + } } }registry-common/src/main/java/io/github/manasmods/manascore/registry/api/Register.java (1)
21-25: Add modId validation and consider thread safety documentation.The implementation looks good, but consider:
- Adding validation for the modId parameter
- Documenting thread safety guarantees
public class Register extends AbstractRegister<Register> { + /** + * Creates a new Register instance for the specified mod. + * This class is not thread-safe and should only be used from the main thread. + * + * @param modId The mod identifier, must not be null or empty + * @throws IllegalArgumentException if modId is null or empty + */ public Register(String modId) { + if (modId == null || modId.trim().isEmpty()) { + throw new IllegalArgumentException("modId must not be null or empty"); + } super(modId); } }skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/Changeable.java (1)
11-15: Consider defensive copying for mutable objectsFor mutable type parameters T, the current implementation doesn't prevent modifications to the stored objects themselves, which could lead to unexpected behavior.
Consider adding a copy constructor or clone method for mutable objects:
protected Changeable(@Nullable T value) { this.original = deepCopy(value); this.value = deepCopy(value); } @SuppressWarnings("unchecked") private T deepCopy(@Nullable T value) { if (value == null) return null; // Implementation depends on type T // Example for Cloneable objects: if (value instanceof Cloneable) { try { return (T) value.getClass().getMethod("clone").invoke(value); } catch (Exception e) { return value; } } return value; }registry-common/src/main/java/io/github/manasmods/manascore/registry/api/ManasAttributeRegistry.java (1)
27-30: Consider adding validation for builder supplierThe
registerNewmethod should validate that the builder supplier produces non-null builders.Consider wrapping the supplier:
@ExpectPlatform public static void registerNew(Supplier<EntityType<? extends LivingEntity>> type, Supplier<AttributeSupplier.Builder> builder) { - throw new AssertionError(); + validateParameters(type, builder); + Supplier<AttributeSupplier.Builder> validatedBuilder = () -> { + AttributeSupplier.Builder result = builder.get(); + if (result == null) { + throw new IllegalStateException("Builder supplier returned null"); + } + return result; + }; + throw new AssertionError("Platform implementation not found for registerNew()"); }skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/ManasSkillNetwork.java (1)
14-21: Consider implementing a packet registry pattern.Instead of directly registering packets in the init method, consider implementing a packet registry pattern for better modularity and testability. This would allow easier addition of new packet types and better unit testing capabilities.
skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/TickingSkill.java (1)
27-37: Simplify boolean chain and use atomic operations.The current implementation has a complex chain of boolean returns and non-atomic duration increment.
public boolean tick(Skills storage, LivingEntity entity) { - if (!entity.isAlive()) return false; - Optional<ManasSkillInstance> optional = storage.getSkill(skill); - if (optional.isEmpty()) return false; - - ManasSkillInstance instance = optional.get(); - if (reachedMaxDuration(instance, entity)) return false; - - if (!instance.canInteractSkill(entity)) return false; - return instance.onHeld(entity, this.duration++, mode); + return entity.isAlive() + && storage.getSkill(skill) + .filter(instance -> !reachedMaxDuration(instance, entity)) + .filter(instance -> instance.canInteractSkill(entity)) + .map(instance -> instance.onHeld(entity, duration.getAndIncrement(), mode)) + .orElse(false); }skill-common/src/main/java/io/github/manasmods/manascore/skill/api/Skills.java (1)
8-11: Standardize null-safety annotations.The interface mixes Lombok's @nonnull with JetBrains' @NotNull annotations. Stick to one annotation library for consistency.
import io.github.manasmods.manascore.skill.impl.SkillStorage; -import lombok.NonNull; import net.minecraft.resources.ResourceLocation; import org.jetbrains.annotations.NotNull;skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkillClient.java (2)
27-31: Consider optimizing the skill iteration process.The current implementation creates a new ArrayList and iterates through all learned skills on every scroll event, which could be inefficient for players with many skills.
Consider:
- Pre-filtering skills that respond to scroll events
- Using a more efficient data structure for frequently accessed skills
- List<ResourceLocation> packetSkills = new ArrayList<>(); - for (ManasSkillInstance skillInstance : SkillAPI.getSkillsFrom(player).getLearnedSkills()) { + List<ResourceLocation> packetSkills = SkillAPI.getSkillsFrom(player).getLearnedSkills().stream() + .filter(skill -> SkillEvents.SKILL_SCROLL.invoker().scroll(skill, player, amountY).isTrue()) + .map(ManasSkillInstance::getSkillId) + .toList(); - if (SkillEvents.SKILL_SCROLL.invoker().scroll(skillInstance, player, amountY).isFalse()) continue; - packetSkills.add(skillInstance.getSkillId()); - }
33-36: Consider rate limiting scroll events.There's no rate limiting on scroll events, which could lead to packet spam if the user scrolls rapidly.
Consider implementing a cooldown mechanism or rate limiting for scroll events to prevent network congestion.
skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/EntityEvents.java (2)
20-26: Add documentation for event fields.The event fields lack documentation explaining their purpose, timing, and usage.
Consider adding JavaDoc for each event field:
+ /** + * Fired before a living entity's tick processing. + * Use this event to modify entity state before the tick occurs. + */ public static Event<LivingTickEvent> LIVING_PRE_TICK = EventFactory.createLoop(); + + /** + * Fired after a living entity's tick processing. + * Use this event to perform actions after the tick is complete. + */ public static Event<LivingTickEvent> LIVING_POST_TICK = EventFactory.createLoop(); // Add similar documentation for other events
59-64: Enhance ProjectileHitResult documentation.The enum comments could be more descriptive to better explain the behavior of each result type.
Consider improving the documentation:
public enum ProjectileHitResult { - DEFAULT, // Hit, damage + possibly continue - HIT, // Hit + damage - HIT_NO_DAMAGE, // Hit - PASS // Pass through + /** + * Default behavior: Projectile hits, applies damage, and may continue based on implementation + */ + DEFAULT, + /** + * Projectile hits and applies damage, then stops + */ + HIT, + /** + * Projectile hits but applies no damage, then stops + */ + HIT_NO_DAMAGE, + /** + * Projectile passes through without interaction + */ + PASS }skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillAPI.java (1)
52-56: Consider consolidating packet sending methods.The three packet sending methods have similar structure and environment checks.
Consider creating a private helper method to reduce code duplication:
+ private static void sendPacketIfClient(Runnable packetSender) { + if (Platform.getEnvironment() == Env.CLIENT) { + packetSender.run(); + } + } + public static void skillActivationPacket(int keyNumber) { - if (Platform.getEnvironment() == Env.CLIENT) { - InternalSkillPacketActions.sendSkillActivationPacket(keyNumber); - } + sendPacketIfClient(() -> InternalSkillPacketActions.sendSkillActivationPacket(keyNumber)); }Also applies to: 64-68, 76-80
config-common/src/main/java/io/github/manasmods/manascore/config/ConfigRegistry.java (1)
48-54: Consider performance optimizations for config retrieval.The current implementation could be improved by:
- Using a more efficient lookup mechanism (e.g., Map) instead of streaming
- Adding caching for frequently accessed configs
+private static final Map<String, ManasConfig> configCache = new ConcurrentHashMap<>(); public static <T extends ManasConfig> T getConfig(Class<T> clazz) { + String key = clazz.getSimpleName().toLowerCase(); + return (T) configCache.computeIfAbsent(key, k -> loadedConfigs.stream() .filter(config -> config.getClass().getSimpleName().equalsIgnoreCase(k)) .findFirst() - .orElse(null)); + .orElse(null)); }skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillEvents.java (1)
16-80: Well-designed event system with comprehensive coverage!The event system is well-structured with clear separation of concerns. Consider adding:
- Documentation for the
modeparameter inSkillUpdateCooldownEvent- Documentation for the return values of
EventResultin each event@FunctionalInterface interface SkillUpdateCooldownEvent { + /** + * Updates the cooldown for a skill + * @param skillInstance The skill instance being updated + * @param owner The entity that owns the skill + * @param currentCooldown The current cooldown value + * @param mode The mode of cooldown update: + * 0 = Reset + * 1 = Increment + * 2 = Decrement + * @return EventResult indicating if the cooldown update should proceed + */ EventResult cooldown(ManasSkillInstance skillInstance, LivingEntity owner, int currentCooldown, int mode); }registry-neoforge/src/main/java/io/github/manasmods/manascore/registry/api/neoforge/ManasAttributeRegistryImpl.java (1)
44-69: Consider adding safeguards for event handling.Potential improvements:
- Add validation for builder results
- Consider synchronization for registry clearing
- Add logging for attribute registration failures
static void registerAttributes(final EntityAttributeModificationEvent e) { + if (e == null) { + ManasCoreConfig.LOG.error("EntityAttributeModificationEvent is null"); + return; + } Multimap<EntityType<? extends LivingEntity>, Consumer<AttributeSupplier.Builder>> keyResolvedMap = ArrayListMultimap.create(); REGISTRY.forEach((key, value) -> keyResolvedMap.put(key.get(), value)); e.getTypes().forEach(type -> { AttributeSupplier.Builder builder = new AttributeSupplier.Builder(); if (keyResolvedMap.containsKey(type)) keyResolvedMap.get(type).forEach(consumer -> consumer.accept(builder)); GLOBAL_REGISTRY.forEach(consumer -> consumer.accept(builder)); - builder.build().instances.forEach((attribute, attributeInstance) -> e.add(type, attribute, attributeInstance.getBaseValue())); + try { + AttributeSupplier supplier = builder.build(); + if (supplier != null && supplier.instances != null) { + supplier.instances.forEach((attribute, attributeInstance) -> + e.add(type, attribute, attributeInstance.getBaseValue())); + } + } catch (Exception ex) { + ManasCoreConfig.LOG.error("Failed to build attributes for type: " + type, ex); + } }); synchronized (REGISTRY) { REGISTRY.clear(); } synchronized (GLOBAL_REGISTRY) { GLOBAL_REGISTRY.clear(); } }skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/InternalSkillPacketActions.java (3)
31-44: Extract magic numbers and add input validation.Consider these improvements:
- Add validation for keyNumber
- Extract packet-related constants
- Add debug logging for skill activation attempts
+ private static final int MIN_KEY_NUMBER = 0; + private static final int MAX_KEY_NUMBER = 9; // Adjust based on your requirements + public static void sendSkillActivationPacket(int keyNumber) { + if (keyNumber < MIN_KEY_NUMBER || keyNumber > MAX_KEY_NUMBER) { + ManasCoreConfig.LOG.warn("Invalid key number: " + keyNumber); + return; + } var minecraft = Minecraft.getInstance(); Player player = minecraft.player; if (player == null) return; List<ResourceLocation> packetSkills = new ArrayList<>(); for (ManasSkillInstance skillInstance : SkillAPI.getSkillsFrom(player).getLearnedSkills()) { + ManasCoreConfig.LOG.debug("Attempting to activate skill: " + skillInstance.getSkillId()); if (SkillEvents.ACTIVATE_SKILL.invoker().activateSkill(skillInstance, player, keyNumber).isFalse()) continue; packetSkills.add(skillInstance.getSkillId()); }
50-63: Add validation for held ticks parameter.Similar improvements as activation packet, plus validation for held ticks:
+ private static final int MIN_HELD_TICKS = 0; + public static void sendSkillReleasePacket(int keyNumber, int heldTicks) { + if (keyNumber < MIN_KEY_NUMBER || keyNumber > MAX_KEY_NUMBER) { + ManasCoreConfig.LOG.warn("Invalid key number: " + keyNumber); + return; + } + if (heldTicks < MIN_HELD_TICKS) { + ManasCoreConfig.LOG.warn("Invalid held ticks: " + heldTicks); + return; + } var minecraft = Minecraft.getInstance(); Player player = minecraft.player; if (player == null) return;
69-83: Add documentation for toggle conditions.The toggle packet handling looks good but would benefit from better documentation:
+ /** + * Sends a packet to toggle skills that meet the following conditions: + * 1. Player must be present on the client + * 2. Skill must be learned by the player + * 3. Skill must be toggleable (canBeToggled returns true) + * 4. Toggle event must not be cancelled + */ public static void sendSkillTogglePacket() { var minecraft = Minecraft.getInstance(); Player player = minecraft.player; if (player == null) return; List<ResourceLocation> packetSkills = new ArrayList<>();keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/ManasKeybinding.java (1)
31-81: Consolidate constructor overloads using builder patternThe class has 6 constructors with varying parameters, which could be simplified using a builder pattern for better maintainability and readability.
Consider implementing a builder:
public static class Builder { private String langKey; private InputConstants.Key defaultKey = InputConstants.UNKNOWN; private KeybindingCategory category; private KeyBindingAction action; private KeyBindingRelease release; public Builder(String langKey, KeybindingCategory category, KeyBindingAction action) { this.langKey = langKey; this.category = category; this.action = action; } public Builder defaultKey(int key) { this.defaultKey = InputConstants.Type.KEYSYM.getOrCreate(key); return this; } public Builder release(KeyBindingRelease release) { this.release = release; return this; } public ManasKeybinding build() { return new ManasKeybinding(langKey, defaultKey, category, action, release); } }config-fabric/src/main/resources/fabric.mod.json (1)
10-12: Add homepage URLThe homepage field is empty. Consider adding the GitHub repository URL or documentation site.
build.gradle (1)
Line range hint
1-85: Consider architectural improvements for better modularity.The build script handles multiple concerns (compilation, publishing, signing, etc.). Consider splitting it into separate Gradle files for better maintainability:
buildSrc/directory for custom plugins and tasks- Separate files for publishing, signing, and platform-specific configurations
Example structure:
buildSrc/ └── src/main/groovy/ ├── manas.java-conventions.gradle ├── manas.publishing-conventions.gradle └── manas.platform-conventions.gradle
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
command-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngcommand-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngconfig-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngconfig-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngconfig-neoforge/src/main/resources/icon.pngis excluded by!**/*.pnginventory-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pnginventory-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngkeybinding-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngkeybinding-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngkeybinding-neoforge/src/main/resources/icon.pngis excluded by!**/*.pngnetwork-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngnetwork-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngregistry-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngregistry-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngregistry-neoforge/src/main/resources/icon.pngis excluded by!**/*.pngskill-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngskill-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngskill-neoforge/src/main/resources/icon.pngis excluded by!**/*.pngstorage-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngstorage-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngtesting-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngtesting-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.png
📒 Files selected for processing (82)
build.gradle(1 hunks)config-common/build.gradle(1 hunks)config-common/src/main/java/io/github/manasmods/manascore/config/ConfigRegistry.java(1 hunks)config-common/src/main/java/io/github/manasmods/manascore/config/ManasCoreConfig.java(1 hunks)config-common/src/main/java/io/github/manasmods/manascore/config/api/ManasConfig.java(1 hunks)config-common/src/main/resources/architectury.common.json(1 hunks)config-common/src/main/resources/manascore_config.accesswidener(1 hunks)config-common/src/main/resources/manascore_config.mixins.json(1 hunks)config-fabric/src/main/java/io/github/manasmods/manascore/config/fabric/ManasCoreConfigFabric.java(1 hunks)config-fabric/src/main/resources/fabric.mod.json(1 hunks)config-neoforge/gradle.properties(1 hunks)config-neoforge/src/main/java/io/github/manasmods/manascore/config/neoforge/ManasCoreConfigNeoForge.java(1 hunks)config-neoforge/src/main/resources/META-INF/neoforge.mods.toml(1 hunks)inventory-fabric/src/main/java/io/github/manasmods/manascore/inventory/fabric/ManasCoreInventoryFabric.java(1 hunks)keybinding-common/build.gradle(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybinding.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingClient.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingCategory.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingManager.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/ManasKeybinding.java(1 hunks)keybinding-common/src/main/resources/architectury.common.json(1 hunks)keybinding-common/src/main/resources/manascore_keybinding.accesswidener(1 hunks)keybinding-common/src/main/resources/manascore_keybinding.mixins.json(1 hunks)keybinding-fabric/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingFabric.java(1 hunks)keybinding-fabric/src/main/resources/fabric.mod.json(1 hunks)keybinding-fabric/src/main/resources/manascore_keybinding-fabric.mixins.json(1 hunks)keybinding-neoforge/build.gradle(1 hunks)keybinding-neoforge/gradle.properties(1 hunks)keybinding-neoforge/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingNeoForge.java(1 hunks)keybinding-neoforge/src/main/resources/META-INF/neoforge.mods.toml(1 hunks)registry-common/build.gradle(1 hunks)registry-common/src/main/java/io/github/manasmods/manascore/registry/ManasCoreRegistry.java(1 hunks)registry-common/src/main/java/io/github/manasmods/manascore/registry/api/AbstractRegister.java(1 hunks)registry-common/src/main/java/io/github/manasmods/manascore/registry/api/ManasAttributeRegistry.java(1 hunks)registry-common/src/main/java/io/github/manasmods/manascore/registry/api/Register.java(1 hunks)registry-common/src/main/resources/architectury.common.json(1 hunks)registry-common/src/main/resources/manascore_registry.accesswidener(1 hunks)registry-common/src/main/resources/manascore_registry.mixins.json(1 hunks)registry-fabric/build.gradle(1 hunks)registry-fabric/src/main/java/io/github/manasmods/manascore/registry/ManasCoreRegistryFabric.java(1 hunks)registry-fabric/src/main/java/io/github/manasmods/manascore/registry/api/fabric/ManasAttributeRegistryImpl.java(1 hunks)registry-fabric/src/main/resources/fabric.mod.json(1 hunks)registry-fabric/src/main/resources/manascore_registry-fabric.mixins.json(1 hunks)registry-neoforge/build.gradle(1 hunks)registry-neoforge/gradle.properties(1 hunks)registry-neoforge/src/main/java/io/github/manasmods/manascore/registry/ManasCoreRegistryNeoForge.java(1 hunks)registry-neoforge/src/main/java/io/github/manasmods/manascore/registry/api/neoforge/ManasAttributeRegistryImpl.java(1 hunks)registry-neoforge/src/main/resources/META-INF/neoforge.mods.toml(1 hunks)settings.gradle(1 hunks)skill-common/build.gradle(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkill.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkillClient.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/AbstractSkillRegister.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkill.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkillInstance.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillAPI.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillEvents.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/Skills.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillRegistry.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/TickingSkill.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/data/SkillTagProvider.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/InternalSkillPacketActions.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/ManasSkillNetwork.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillActivationPacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillReleasePacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillScrollPacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillTogglePacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinLivingEntity.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinPlayer.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/Changeable.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/EntityEvents.java(1 hunks)skill-common/src/main/resources/architectury.common.json(1 hunks)skill-common/src/main/resources/manascore_skill.accesswidener(1 hunks)skill-common/src/main/resources/manascore_skill.mixins.json(1 hunks)skill-fabric/build.gradle(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/ManasCoreSkillFabric.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractArrow.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractHurtingProjectile.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFireworkRocketEntity.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFishingHook.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLivingEntity.java(1 hunks)
⛔ Files not processed due to max files limit (24)
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLlamaSpit.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinMob.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinPlayer.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinShulkerBullet.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinThrowableProjectile.java
- skill-fabric/src/main/resources/fabric.mod.json
- skill-fabric/src/main/resources/manascore_skill-fabric.mixins.json
- skill-neoforge/build.gradle
- skill-neoforge/gradle.properties
- skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/ManasCoreSkillNeoForge.java
- skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/NeoForgeCommonEventInvoker.java
- skill-neoforge/src/main/resources/META-INF/neoforge.mods.toml
- testing-common/build.gradle
- testing-common/src/main/java/io/github/manasmods/manascore/testing/ManasCoreTesting.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/client/KeybindingTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/client/ManasCoreTestingClient.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/configs/SkillConfig.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/configs/TestConfig.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/module/ConfigModuleTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/RegistryTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestRegister.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestSkill.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestTags.java
- testing-neoforge/src/main/java/io/github/manasmods/manascore/testing/neoforge/ManasCoreTestingNeoForge.java
✅ Files skipped from review due to trivial changes (23)
- config-common/src/main/resources/manascore_config.accesswidener
- skill-common/src/main/resources/manascore_skill.accesswidener
- registry-fabric/build.gradle
- keybinding-common/build.gradle
- keybinding-common/src/main/resources/manascore_keybinding.accesswidener
- keybinding-neoforge/gradle.properties
- inventory-fabric/src/main/java/io/github/manasmods/manascore/inventory/fabric/ManasCoreInventoryFabric.java
- config-neoforge/src/main/java/io/github/manasmods/manascore/config/neoforge/ManasCoreConfigNeoForge.java
- skill-common/src/main/resources/architectury.common.json
- keybinding-common/src/main/resources/architectury.common.json
- config-neoforge/gradle.properties
- config-common/src/main/resources/architectury.common.json
- config-common/src/main/java/io/github/manasmods/manascore/config/ManasCoreConfig.java
- keybinding-common/src/main/resources/manascore_keybinding.mixins.json
- registry-common/src/main/resources/architectury.common.json
- config-common/src/main/resources/manascore_config.mixins.json
- registry-neoforge/gradle.properties
- registry-common/src/main/resources/manascore_registry.mixins.json
- registry-fabric/src/main/resources/manascore_registry-fabric.mixins.json
- keybinding-fabric/src/main/resources/manascore_keybinding-fabric.mixins.json
- config-fabric/src/main/java/io/github/manasmods/manascore/config/fabric/ManasCoreConfigFabric.java
- registry-common/build.gradle
- skill-common/src/main/resources/manascore_skill.mixins.json
🔇 Additional comments (42)
skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillTogglePacket.java (2)
1-21: LGTM! Well-structured package and imports.The package structure clearly indicates client-to-server communication, and the imports are properly organized with cross-platform networking support via Architectury API.
60-62: LGTM! Clean type method implementation.The method is properly annotated and returns the correct type.
skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillReleasePacket.java (4)
1-21: LGTM! Well-organized package structure and imports.The package name and imports are well-organized, following Java conventions. The c2s package name clearly indicates this is a client-to-server packet.
27-28: LGTM! Proper packet type and codec implementation.The packet type and codec are correctly implemented following Minecraft's networking patterns.
30-38: LGTM! Consistent encoding and decoding implementation.The constructor and encode method maintain perfect symmetry in their read/write operations, ensuring reliable packet transmission.
49-51: LGTM! Correct type() method implementation.The type() method correctly implements the CustomPacketPayload interface requirement.
keybinding-fabric/src/main/resources/fabric.mod.json (2)
22-25: Verify the existence of mixin configuration files.Please ensure both mixin configuration files exist and are properly configured:
${mod_id}.mixins.json${mod_id}-fabric.mixins.json✅ Verification successful
✓ Mixin configuration files verified
Both mixin configuration files exist in the repository:
manascore_keybinding.mixins.jsonin keybinding-commonmanascore_keybinding-fabric.mixins.jsonin keybinding-fabric🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mixin configuration files fd -e json -x grep -l "\"package\": \".*mixin.*\"" {}Length of output: 817
15-21: Verify if client-side initialization is required.The
cliententrypoints array is empty, but since this is a keybinding module, it might need client-side initialization. Please verify if any client-specific setup is required.✅ Verification successful
Client entrypoint is not required for this implementation
The keybinding system uses Architectury's client event system (
ClientTickEvent) in the common module, which handles the client-side initialization internally. The empty client entrypoints array is correct for this implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the keybinding implementation has client-specific code ast-grep --pattern 'class $_ implements ClientModInitializer { $$$ }' # Check for client-only annotations or methods rg -l "(@Environment|@OnlyIn).*\.CLIENT"Length of output: 209
Script:
#!/bin/bash # Check the keybinding implementation class ast-grep --pattern 'class ManasCoreKeybindingFabric { $$$ }' # Look for keybinding-related code rg -l "KeyBinding|registerKeyBinding|onKeyPress" # Check if there are any client-specific method calls or registrations rg "MinecraftClient|ClientTickEvent" -A 3Length of output: 1445
skill-common/src/main/java/io/github/manasmods/manascore/skill/api/AbstractSkillRegister.java (1)
1-14: LGTM! Well-structured package and imports.The file has proper copyright notice, well-organized imports, and follows good package naming conventions.
keybinding-neoforge/src/main/resources/META-INF/neoforge.mods.toml (4)
14-14: Verify the existence of the icon file.Ensure that the referenced
icon.pngfile exists in the correct resources directory.✅ Verification successful
Icon file verification successful
The referenced
icon.pngfile exists at the correct location in the resources directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if icon.png exists in the resources directory fd -g 'icon.png' keybinding-neoforge/src/main/resources/Length of output: 104
1-2: Consider constraining the loader version range.The version range
[2,)is very broad and might include incompatible future versions. Consider setting an upper bound to avoid potential compatibility issues with future NeoForge versions.
19-19: Consider constraining the NeoForge version range.Similar to the loader version, the NeoForge version range
[21,)is very broad. Consider setting an upper bound to avoid potential compatibility issues.
30-35: Verify Architectury dependency ordering.The Architectury dependency is set to load
AFTERwhile others areNONE. Verify this ordering is consistent with other modules and necessary for proper functionality.✅ Verification successful
Architectury dependency ordering is correct and consistent
The "AFTER" ordering for Architectury is consistently implemented across all neoforge modules and follows a logical pattern where core dependencies use "NONE" while framework dependencies use "AFTER". This ensures proper initialization order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Architectury dependency ordering in other modules rg -g '*.toml' 'modId.*=.*"architectury".*\n.*type.*=.*"required".*\n.*ordering.*=.*"AFTER"' .Length of output: 288
Script:
#!/bin/bash # Search for Architectury dependencies with multiline mode echo "=== Searching for all Architectury dependencies ===" rg -U --multiline -g '*.toml' '(?s)\[\[dependencies\.[^\]]+\]\].*?modId.*=.*"architectury".*?side.*=.*"BOTH"' . echo -e "\n=== Searching specifically for different ordering values ===" rg -U --multiline -g '*.toml' 'modId.*=.*"architectury".*ordering.*=.*"[^"]+"' .Length of output: 18408
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/ManasCoreSkillFabric.java (3)
1-5: LGTM! License header is properly formatted.The copyright notice and GPL-3.0 license declaration are correctly specified.
6-10: LGTM! Package and imports are well-organized.The package structure follows Java conventions, and imports are minimal and necessary.
13-15: Verify integration with other modules.The PR introduces multiple modules (Registry, Keybinding, Config), but we only see the Skill module initialization here. Ensure that:
- The initialization order is correct if there are dependencies between modules
- All modules are properly initialized in the Fabric environment
Let's verify the module integration:
✅ Verification successful
Module initialization is correctly implemented.
The current implementation follows best practices:
- Each module handles its own initialization and dependencies
- Client/Server components are properly separated
- No cross-module initialization dependencies require specific ordering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other module initializers echo "Searching for other module initializers..." rg -l "implements ModInitializer" --type java # Check for potential module dependencies echo "Checking for module dependencies..." rg -l "ManasCoreRegistry|ManasCoreKeybinding|ManasCoreConfig" --type javaLength of output: 2293
Script:
#!/bin/bash # Check initialization code in ModInitializer implementations echo "Checking ModInitializer implementations..." for file in $(rg -l "implements ModInitializer" --type java); do echo "=== $file ===" rg -A 5 "onInitialize\(\)" "$file" done # Look for cross-module dependencies in initialization echo -e "\nChecking for cross-module dependencies..." ast-grep --pattern 'class $_ implements ModInitializer { $$$ public void onInitialize() { $$$ } $$$ }'Length of output: 5090
Script:
#!/bin/bash # Check the content of init() methods in common modules echo "Checking init() methods in common modules..." for module in "Registry" "Keybinding" "Skill" "Storage" "Network" "Inventory" "Command"; do echo "=== Manascore${module} init() ===" rg -A 10 "class ManasCore${module}" --type java rg -A 10 "init\(\)" "*common*/src/main/java/*/${module,,}*/ManasCore${module}.java" || true doneLength of output: 25552
registry-neoforge/src/main/resources/META-INF/neoforge.mods.toml (3)
7-14: Verify resource files and template variables.
- Ensure that
icon.pngexists in the resources directory.- Verify that
${mod_id},${version}, and${mod_name}variables are properly defined.Run this script to verify resources and variables:
✅ Verification successful
Resource files and template variables verified successfully
The
icon.pngfile exists in the correct location, and the template variables follow the standard mod development pattern used consistently across the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for icon.png in resources directories echo "Checking for icon.png..." fd -g 'icon.png' 'registry-neoforge/src' # Check for template variable definitions echo "Checking for template variable definitions..." rg -l '\$\{mod_(id|name)\}|\$\{version\}' | while read -r file; do echo "Found variable references in: $file" rg 'mod_(id|name)|version.*=' "$file" doneLength of output: 9106
3-4: Add issue tracker URL and verify license variable.
- The empty issue tracker URL might make it difficult for users to report issues.
- Ensure that the
${license}variable is properly defined in your build configuration.Run this script to verify the license variable resolution:
16-35: Verify dependency version constraints.The dependency configuration looks good with appropriate ordering for architectury. Please verify:
- The NeoForge version constraint "[21,)" is appropriate for your mod's features
- The template variables
${minecraft_version}and${architectury_version}are properly definedRun this script to verify version variables:
✅ Verification successful
Version constraints and variables are properly configured ✓
The dependency configuration is correct:
- Template variables are properly defined in gradle.properties
- NeoForge version constraint "[21,)" is appropriate for the used version 21.1.28
- Minecraft and Architectury version ranges are correctly templated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for version variable definitions echo "Checking for version variable definitions..." rg -l '\$\{(minecraft|architectury)_version\}' | while read -r file; do echo "Found version references in: $file" rg '(minecraft|architectury).*version.*=' "$file" doneLength of output: 5526
Script:
#!/bin/bash # Check build configuration files for version definitions echo "Checking build configuration files..." # First, find all gradle and properties files echo "=== Gradle and Properties files ===" fd -e gradle -e properties # Then search for version definitions echo -e "\n=== Version definitions ===" rg -i '(minecraft|architectury).*version\s*=' --type properties --type groovyLength of output: 1538
Script:
#!/bin/bash echo "=== Checking root gradle.properties ===" cat gradle.properties echo -e "\n=== Checking module gradle.properties ===" for f in */gradle.properties; do if [ -f "$f" ]; then echo "=== $f ===" cat "$f" fi doneLength of output: 3152
skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinPlayer.java (1)
29-32: Ensure inject and modify variable mixins do not conflictBoth
modifyDamage(lines 23-27) andcancelActuallyHurt(lines 29-32) are targeting theactuallyHurtmethod at the same injection point. Confirm that these mixins do not interfere with each other and execute in the intended order.Consider reviewing the mixin priority or adjusting the injection points to prevent potential conflicts.
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFireworkRocketEntity.java (1)
29-31: Appropriate use of@Uniqueand@NullableannotationsThe declaration of
onHitEventResultwith@Uniqueand@Nullableis correct. Using@Uniqueensures that the mixin introduces this field without causing conflicts, and@Nullableaccurately represents that the field can benull.skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinLivingEntity.java (1)
51-54: Validate method parameters incancelActuallyHurtIn the
cancelActuallyHurtmethod, ensure that the parametersDamageSource damageSourceandfloat damageAmountmatch those of theactuallyHurtmethod inLivingEntity. If they do not match, the injection may fail.Run the following script to verify the method signature:
✅ Verification successful
Parameters in
cancelActuallyHurtare correctly definedThe parameter types in the Mixin injection match the target method signature as evidenced by the fully qualified method reference in the
@Injectannotation. The code would fail to compile if there was a mismatch.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the parameters of the `actuallyHurt` method in LivingEntity. # Search for the method signature in the codebase. rg -A 3 'void actuallyHurt\(DamageSource damageSrc, float damageAmount\)' src/Length of output: 125
Script:
#!/bin/bash # First, find all Java files recursively fd -e java . | xargs rg -l "actuallyHurt.*DamageSource.*float" # Then use ast-grep to find the exact method signature ast-grep --pattern 'void actuallyHurt(DamageSource $_, float $_) { $$$ }'Length of output: 142
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractArrow.java (1)
81-85: Possible Unintended Behavior inonImpulseSetMethodThe
onImpulseSetmethod setsthis.onHitEventResulttonullregardless of the condition. Ifthis.onHitEventResultis notnull, andoriginal.call(instance, value)is not called, this might skip necessary logic.Please verify if the intended behavior is to always reset
onHitEventResulttonull. If not, adjust the condition accordingly.skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillRegistry.java (2)
42-52:⚠️ Potential issueNull Check Missing for Optional in
LIVING_CHANGE_TARGETEventIn the
LIVING_CHANGE_TARGETevent handler, the code checksif (!changeableTarget.isPresent()) return EventResult.pass();, but later useschangeableTarget.get()without verifying if it's present, which could lead to aNoSuchElementException.Apply this diff to safely handle the optional:
if (!changeableTarget.isPresent()) return EventResult.pass(); -LivingEntity owner = changeableTarget.get(); +LivingEntity owner = changeableTarget.orElse(null); +if (owner == null) return EventResult.pass(); for (ManasSkillInstance instance : SkillAPI.getSkillsFrom(owner).getLearnedSkills()) {Likely invalid or redundant comment.
63-72:⚠️ Potential issueIncorrect Casting in
SKILL_DAMAGE_PRE_CALCULATIONEventThe code casts
source.getEntity()toLivingEntitywithout checking ifsource.getEntity()is indeed an instance ofLivingEntity, which could lead to aClassCastException.Apply this diff to ensure safe casting:
-if (!(source.getEntity() instanceof LivingEntity owner)) return EventResult.pass(); +Entity sourceEntity = source.getEntity(); +if (!(sourceEntity instanceof LivingEntity owner)) return EventResult.pass();Likely invalid or redundant comment.
skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java (1)
52-58: Potential Issue with Damage Calculation Event RegistrationThe events
SKILL_DAMAGE_PRE_CALCULATION,SKILL_DAMAGE_CALCULATION, andSKILL_DAMAGE_POST_CALCULATIONare invoked sequentially even if one returnsinterruptFalse(). This might not be the intended behavior.Please verify if the intention is to stop the chain of event invocations when one of them returns
interruptFalse(). If so, consider restructuring the logic to respect the interruption.registry-common/src/main/resources/manascore_registry.accesswidener (1)
1-4: Document the purpose of widening access to these fields.While the access widener syntax is correct, please add comments explaining:
- Why these fields need to be exposed
- Potential security implications
- How these fields will be used by the mod
Please ensure these fields are still present in the target Minecraft version:
✅ Verification successful
Access widening is justified by implementation requirements
The widened fields are essential for the mod's core functionality:
AttributeSupplier.instancesis used to manage attribute system integration across platformsMobEffect.particleFactoryenables custom particle effects for mod-specific status effects🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of fields in decompiled Minecraft code # Note: This assumes the presence of a decompiled Minecraft codebase # Search for AttributeSupplier.instances rg -l "class AttributeSupplier" | xargs rg "Map.*instances" # Search for MobEffect.particleFactory rg -l "class MobEffect" | xargs rg "Function.*particleFactory"Length of output: 126
Script:
#!/bin/bash # Check for mod configuration files and documentation echo "=== Searching for mod configuration files ===" fd -e toml -e json "^(fabric|forge|mod|gradle)\." --exec head -n 20 {} echo -e "\n=== Searching for mapping references ===" rg -l "net\.minecraft\.(world|entity)" --type yaml --type properties echo -e "\n=== Checking for documentation ===" fd "README" --exec cat {}Length of output: 4560
Script:
#!/bin/bash # Search for usage of the widened fields echo "=== Searching for AttributeSupplier.instances usage ===" rg -l "AttributeSupplier" --type java rg "instances" --type java -C 3 echo -e "\n=== Searching for MobEffect.particleFactory usage ===" rg -l "MobEffect" --type java rg "particleFactory" --type java -C 3 echo -e "\n=== Looking for related registry code ===" fd . -e java --full-path ".*registry.*" --exec head -n 50 {}Length of output: 9043
skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/ManasSkillNetwork.java (1)
15-20: Consider adding synchronization for packet handling.The network packet handlers should be thread-safe as they might be called concurrently from multiple client connections.
Run this script to check if packet handlers implement thread safety:
skill-common/src/main/java/io/github/manasmods/manascore/skill/api/Skills.java (1)
50-50: Consider thread safety in forEachSkill implementation.The forEachSkill method could lead to deadlocks or inconsistent state if the consumer modifies the skill storage.
Consider providing a read-only view of skills during iteration or documenting thread-safety requirements.
config-common/src/main/java/io/github/manasmods/manascore/config/ConfigRegistry.java (1)
16-22: Well-structured configuration setup!Good choice of thread-safe collections and proper Gson configuration for JSON handling.
registry-neoforge/src/main/java/io/github/manasmods/manascore/registry/api/neoforge/ManasAttributeRegistryImpl.java (1)
26-42: Excellent implementation of attribute registry!Good practices observed:
- Thread-safe collections
- Lazy evaluation with suppliers
- Clear separation of registry types
config-common/build.gradle (1)
1-3: LGTM!The Loom configuration for access widener path follows standard Minecraft modding practices.
registry-neoforge/build.gradle (1)
11-11: Verify mod_id project property is definedThe
project.mod_idis used to construct the access widener filename. Ensure this property is defined in the project's gradle.properties or build script.✅ Verification successful
Property
mod_idis properly defined in build configurationThe
mod_idproperty is dynamically constructed in the root build.gradle for all subprojects using the pattern${rootProject.archives_name}_$module_name, wheremodule_nameis derived from the project path. This ensures unique and consistent mod IDs across all modules.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if mod_id is defined in gradle.properties if ! grep -q "mod_id=" gradle.properties; then echo "Warning: mod_id property not found in gradle.properties" fiLength of output: 156
Script:
#!/bin/bash # Check build.gradle and settings.gradle for mod_id definition echo "=== Checking build.gradle ===" rg "mod_id" registry-neoforge/build.gradle -A 2 -B 2 echo -e "\n=== Checking settings.gradle ===" if [ -f "settings.gradle" ]; then rg "mod_id" settings.gradle fi echo -e "\n=== Checking for other gradle files ===" fd "build.gradle" --type fLength of output: 1220
Script:
#!/bin/bash echo "=== Checking root build.gradle for project configuration ===" rg "subprojects|allprojects|mod_id" build.gradle -A 3 -B 3 echo -e "\n=== Looking for gradle.properties files ===" fd "gradle.properties" --type f echo -e "\n=== Checking root project files ===" ls -la *.gradle *.properties 2>/dev/nullLength of output: 2479
keybinding-neoforge/build.gradle (1)
11-11: Verify mod_id project property is definedSame concern as in registry-neoforge/build.gradle regarding the mod_id property definition.
skill-fabric/build.gradle (1)
10-13: Verify the scope of dependenciesThe dependencies on network-common and storage-common look appropriate for a skill system. However, consider if these dependencies should be marked as
apiinstead ofimplementationif they expose public APIs that skill-fabric consumers might need to access directly.✅ Verification successful
Dependencies can remain as 'implementation'
The skill-fabric module doesn't expose any network or storage APIs through its public interface. It only provides Fabric-specific integration through mixins and delegates core functionality to the main skill module. The current dependency scope is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any public APIs from network-common or storage-common are exposed through skill-fabric's public interface rg -t java "import.*(?:network|storage).*" ./skill-fabric/src/main/java/Length of output: 74
Script:
#!/bin/bash # Check if the directory exists and list Java files echo "=== Checking directory structure ===" fd -e java . skill-fabric/src/ echo -e "\n=== Checking all imports to understand package structure ===" fd -e java . skill-fabric/src/ -x rg -l "^import" echo -e "\n=== List all package declarations to understand structure ===" fd -e java . skill-fabric/src/ -x rg "^package"Length of output: 3319
Script:
#!/bin/bash # Check content of main class and mixins for any network/storage related code echo "=== Checking ManasCoreSkillFabric.java ===" cat skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/ManasCoreSkillFabric.java echo -e "\n=== Checking all imports in all files ===" fd -e java . skill-fabric/src/ -x cat {} | rg "^import"Length of output: 6803
skill-common/build.gradle (1)
1-3: Verify the access widener file existsThe Loom configuration references an access widener file. Let's ensure it exists and contains the necessary wideners.
✅ Verification successful
Access widener file exists and is properly configured ✅
The file exists with the correct format header (
accessWidener v2 named). While currently empty, this is valid as wideners can be added as needed during development.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the access widener file exists and examine its contents if [ -f "skill-common/src/main/resources/manascore_skill.accesswidener" ]; then echo "Access widener file exists" cat "skill-common/src/main/resources/manascore_skill.accesswidener" else echo "Access widener file is missing" fiLength of output: 228
settings.gradle (1)
Line range hint
17-21: LGTM! Module structure follows the established patternThe new modules (registry, keybinding, skill, config) are properly integrated into the existing module structure, maintaining consistency with the common/fabric/neoforge pattern.
✅ Verification successful
✓ Module structure is complete and properly implemented
All modules (registry, keybinding, skill, config) are properly structured with:
- Common variants containing core APIs
- Platform-specific implementations for both Fabric and NeoForge
- Consistent naming patterns and package structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all module directories exist for module in registry keybinding skill config; do for variant in common fabric neoforge; do if [ ! -d "$module-$variant" ]; then echo "Missing directory: $module-$variant" fi done doneLength of output: 1070
Script:
#!/bin/bash # Check for source files in module directories for module in registry keybinding skill config; do for variant in common fabric neoforge; do echo "=== Checking $module-$variant ===" fd -e java -e kt . "$module-$variant" || true done doneLength of output: 7338
config-fabric/src/main/resources/fabric.mod.json (1)
28-28: Consider the Java version requirementRequiring Java 21 (released in September 2023) might be too restrictive for many users. Consider if Java 17 (the current LTS version) would be sufficient for your needs.
registry-fabric/src/main/resources/fabric.mod.json (1)
28-29: ** Consider the Java version requirement**Same concern as in config-fabric: requiring Java 21 might be too restrictive. Consider if Java 17 would be sufficient.
config-neoforge/src/main/resources/META-INF/neoforge.mods.toml (2)
16-35: Verify dependency version constraints.The dependency version ranges use open-ended constraints (
[21,),[${minecraft_version},),[${architectury_version},)). This could potentially lead to compatibility issues with future versions.Consider adding upper bounds to version ranges for better version control:
- versionRange = "[21,)" + versionRange = "[21,22)" - versionRange = "[${minecraft_version},)" + versionRange = "[${minecraft_version},${minecraft_version_next})" - versionRange = "[${architectury_version},)" + versionRange = "[${architectury_version},${architectury_version_next})"
37-38: Verify mixin configuration file existence.Ensure that the referenced mixin configuration file exists and is properly configured.
✅ Verification successful
Mixin configuration verified successfully ✓
The referenced mixin configuration file exists at
config-common/src/main/resources/manascore_config.mixins.jsonand is properly configured with all required fields.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the mixin configuration file exists fd -e json -x grep -l "\"package\": \".*${mod_id}.*\"" {}Length of output: 812
Script:
#!/bin/bash # Get the mod_id from neoforge.mods.toml echo "=== Mod ID from neoforge.mods.toml ===" cat config-neoforge/src/main/resources/META-INF/neoforge.mods.toml | grep -A 1 "^modId" echo -e "\n=== Looking for mixin files in neoforge paths ===" fd -e json . config-neoforge/Length of output: 366
Script:
#!/bin/bash # Look for gradle files defining mod_id echo "=== Searching for mod_id in gradle files ===" rg "mod_id" -A 2 --glob "*.gradle*" echo -e "\n=== Looking for mixin configuration templates ===" fd ".*mixins?.json.*" config-neoforge/Length of output: 1772
Script:
#!/bin/bash echo "=== Searching for archives_name in gradle files ===" rg "archives_name" --glob "*.gradle*" -A 2 echo -e "\n=== Looking for config module mixin files ===" fd "manascore.*config.*mixins?.json"Length of output: 664
Script:
#!/bin/bash echo "=== Checking mixin configuration content ===" cat config-common/src/main/resources/manascore_config.mixins.jsonLength of output: 390
build.gradle (1)
84-85: Review mixin configuration changes.Two significant changes to the mixin configuration:
- Setting
useLegacyMixinAp = falseindicates a switch to the modern mixin annotation processor- Commenting out
defaultRefmapNamemight affect mixin reference mappingLet's verify the impact of these changes:
✅ Verification successful
Mixin configuration changes are safe to proceed ✅
The changes are well-aligned with modern Minecraft mod development practices:
- Setting
useLegacyMixinAp = falseis the recommended approach for new projects- Removing the custom refmap name is safe as the modern mixin annotation processor handles refmap generation automatically for each module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for mixin configurations across the project fd -e json -x grep -l "\"refmap\": " {} # Check for any custom refmap references in code rg -l "refmap.*json"Length of output: 77
Script:
#!/bin/bash # Check for mixin usage in the codebase rg "@Mixin" -A 2 # Look for mixin configuration classes ast-grep --pattern 'class $_ implements IMixinConfigPlugin'Length of output: 10593
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractHurtingProjectile.java (1)
27-27: 🛠️ Refactor suggestionUse '==' operator for enum comparison instead of 'Objects.equals'
When comparing enum constants in Java, it's recommended to use the
==operator rather thanObjects.equals, as enums are singletons and==provides a more efficient and type-safe comparison.- if (!Objects.equals(resultChangeable.get(), EntityEvents.ProjectileHitResult.DEFAULT)) return deflectionChangeable.get(); + if (resultChangeable.get() != EntityEvents.ProjectileHitResult.DEFAULT) return deflectionChangeable.get();keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/ManasKeybinding.java (1)
16-16:⚠️ Potential issueConsider thread safety for shared state
The static
PRESSED_KEYBINDINGSHashMap lacks synchronization, which could lead to race conditions in a game environment with multiple input events.
🧹 Nitpick comments (11)
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLlamaSpit.java (2)
1-4: Update the copyright year.The copyright notice shows the year 2025, which is in the future. Consider updating it to the current year or the year when the code was written.
- * Copyright (c) 2025. ManasMods + * Copyright (c) 2024. ManasMods
22-30: Add method documentation and improve null safety.The method implementation is solid, but could benefit from these improvements:
- Add Javadoc explaining the purpose and behavior of the method
- Make the null check more explicit
Consider applying these changes:
+ /** + * Wraps the hit detection of LlamaSpit to allow modification through events. + * + * @param instance The LlamaSpit entity instance + * @param result The original hit result + * @param original The original operation to be wrapped + * @return The resulting ProjectileDeflection + */ @WrapOperation(method = "tick", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/entity/projectile/LlamaSpit;hitTargetOrDeflectSelf(Lnet/minecraft/world/phys/HitResult;)Lnet/minecraft/world/entity/projectile/ProjectileDeflection;")) ProjectileDeflection onHit(LlamaSpit instance, HitResult result, Operation<ProjectileDeflection> original) { Changeable<EntityEvents.ProjectileHitResult> resultChangeable = Changeable.of(EntityEvents.ProjectileHitResult.DEFAULT); Changeable<ProjectileDeflection> deflectionChangeable = Changeable.of(ProjectileDeflection.NONE); EntityEvents.PROJECTILE_HIT.invoker().hit(result, instance, deflectionChangeable, resultChangeable); - if (!Objects.equals(resultChangeable.get(), EntityEvents.ProjectileHitResult.DEFAULT)) return deflectionChangeable.get(); + // If the event modified the result, return the modified deflection + if (resultChangeable.get() != null && resultChangeable.get() != EntityEvents.ProjectileHitResult.DEFAULT) { + return deflectionChangeable.get(); + } original.call(instance, result); return deflectionChangeable.get(); }skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractHurtingProjectile.java (2)
2-2: Update the copyright yearThe copyright year is set to 2025, but it should be the current year or the year when the code was written.
- * Copyright (c) 2025. ManasMods + * Copyright (c) 2024. ManasMods
22-23: Add method documentationAdd Javadoc to explain the purpose of this method, its parameters, and return value. This will help other developers understand the mixin's functionality.
+ /** + * Wraps the hitTargetOrDeflectSelf method to allow modification of projectile hit behavior. + * @param instance The projectile instance + * @param result The hit result + * @param original The original operation + * @return The projectile deflection result + */ @WrapOperation(method = "tick", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/entity/projectile/AbstractHurtingProjectile;hitTargetOrDeflectSelf(Lnet/minecraft/world/phys/HitResult;)Lnet/minecraft/world/entity/projectile/ProjectileDeflection;"))keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingCategory.java (1)
11-13: Add parameter validation and documentation.The
nameparameter lacks validation and documentation. Consider:
- Adding parameter validation to prevent null/empty values
- Adding Javadoc to document the expected format/constraints
@RequiredArgsConstructor(staticName = "of") public class KeybindingCategory { + /** + * @param name The category name (non-null, non-empty) + * @throws IllegalArgumentException if name is null or empty + */ private final String name; + + private static String validateName(String name) { + if (name == null || name.trim().isEmpty()) { + throw new IllegalArgumentException("Category name cannot be null or empty"); + } + return name; + }skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkill.java (1)
301-305: Correct Javadoc syntax for@paramtagThe Javadoc comment incorrectly uses
{@param heldTicks}. The correct syntax for documenting parameters is@param parameterName description.Apply this diff to fix the Javadoc:
- * Called when the {@link LivingEntity} owning this Skill releases the skill activation button after {@param heldTicks}. + * Called when the {@link LivingEntity} owning this Skill releases the skill activation button after a certain number of ticks. + * + * @param heldTicks The number of ticks the skill activation button was held.skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/InternalSkillPacketActions.java (2)
31-83: Reduce code duplication in packet action methods.The packet action methods share similar patterns. Consider extracting common logic into a helper method:
+ private static <T> void processSkillPacket(int keyNumber, + Function<ManasSkillInstance, Boolean> skillFilter, + BiFunction<Integer, List<ResourceLocation>, T> packetCreator) { + var minecraft = Minecraft.getInstance(); + Player player = minecraft.player; + if (player == null) return; + List<ResourceLocation> packetSkills = new ArrayList<>(); + + for (ManasSkillInstance skillInstance : SkillAPI.getSkillsFrom(player).getLearnedSkills()) { + if (skillFilter.apply(skillInstance)) { + packetSkills.add(skillInstance.getSkillId()); + } + } + + if (!packetSkills.isEmpty()) { + NetworkManager.sendToServer(packetCreator.apply(keyNumber, packetSkills)); + } + }This would simplify the implementation of each packet action method.
34-34: Add logging for failed packet operations.Consider adding debug logging when packet operations fail (e.g., null player) to aid in troubleshooting.
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class InternalSkillPacketActions { + private static final Logger LOGGER = LoggerFactory.getLogger(InternalSkillPacketActions.class); public static void sendSkillActivationPacket(int keyNumber) { var minecraft = Minecraft.getInstance(); Player player = minecraft.player; - if (player == null) return; + if (player == null) { + LOGGER.debug("Cannot send skill activation packet: player is null"); + return; + }Also applies to: 53-53, 72-72
skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/EntityEvents.java (2)
20-27: Enhance event documentation.Consider adding more detailed documentation for each event, including:
- When the event is fired
- Expected usage scenarios
- Example implementations
+ /** + * Fired before a living entity's tick processing. + * Use this event to modify entity behavior at the start of each tick. + */ public static Event<LivingTickEvent> LIVING_PRE_TICK = EventFactory.createLoop(); + + /** + * Fired after a living entity's tick processing. + * Use this event to perform post-tick modifications or calculations. + */ public static Event<LivingTickEvent> LIVING_POST_TICK = EventFactory.createLoop();
59-64: Improve ProjectileHitResult enum documentation.The enum states would benefit from more detailed documentation explaining their use cases and implications.
public enum ProjectileHitResult { - DEFAULT, // Hit, damage + possibly continue - HIT, // Hit + damage - HIT_NO_DAMAGE, // Hit - PASS // Pass through + /** + * Default behavior: Hit the target, apply damage, and possibly continue trajectory + */ + DEFAULT, + /** + * Hit the target and apply damage, then stop + */ + HIT, + /** + * Hit the target without applying damage + */ + HIT_NO_DAMAGE, + /** + * Pass through the target without interaction + */ + PASS }skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillRegistry.java (1)
32-119: Enhance error handling in event handlers.Consider adding try-catch blocks around event handler logic to prevent exceptions from propagating to the event system.
public static void init() { EntityEvents.LIVING_EFFECT_ADDED.register((entity, source, changeableTarget) -> { + try { for (ManasSkillInstance instance : SkillAPI.getSkillsFrom(entity).getLearnedSkills()) { if (!instance.canInteractSkill(entity)) continue; if (!instance.onEffectAdded(entity, source, changeableTarget)) return EventResult.interruptFalse(); } return EventResult.pass(); + } catch (Exception e) { + LOGGER.error("Error processing LIVING_EFFECT_ADDED event", e); + return EventResult.pass(); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
command-common/build.gradle(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/CommandAnnotationHandler.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/ManasCoreCommand.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/PlatformCommandUtils.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/Command.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/CommandArgumentRegistrationEvent.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/CommandRegistry.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/Execute.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/Permission.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Bool.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/DoubleNumber.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Enum.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/FloatNumber.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/IntNumber.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Literal.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/LongNumber.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Sender.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Text.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Uuid.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/internal/CommandArgumentRegistry.java(1 hunks)command-common/src/main/java/io/github/manasmods/manascore/command/internal/CommandCreationContext.java(1 hunks)command-fabric/build.gradle(1 hunks)command-fabric/src/main/java/io/github/manasmods/manascore/command/fabric/ManasCoreCommandFabric.java(1 hunks)command-fabric/src/main/java/io/github/manasmods/manascore/command/fabric/PlatformCommandUtilsImpl.java(1 hunks)command-fabric/src/main/java/io/github/manasmods/manascore/command/fabric/integrations/FabricPermissionsApiIntegration.java(1 hunks)command-neoforge/build.gradle(1 hunks)command-neoforge/gradle.properties(1 hunks)command-neoforge/src/main/java/io/github/manasmods/manascore/command/neoforge/ManasCoreCommandNeoForge.java(1 hunks)command-neoforge/src/main/java/io/github/manasmods/manascore/command/neoforge/PlatformCommandUtilsImpl.java(1 hunks)config-neoforge/src/main/java/io/github/manasmods/manascore/config/neoforge/ManasCoreConfigNeoForge.java(1 hunks)inventory-common/build.gradle(1 hunks)inventory-common/src/main/java/io/github/manasmods/manascore/inventory/VanillaInventoryTab.java(1 hunks)inventory-common/src/main/java/io/github/manasmods/manascore/inventory/api/AbstractInventoryTab.java(1 hunks)inventory-common/src/main/java/io/github/manasmods/manascore/inventory/client/ManasCoreInventoryClient.java(1 hunks)inventory-common/src/main/java/io/github/manasmods/manascore/inventory/client/widget/InventoryTabSwitcherWidget.java(1 hunks)inventory-common/src/main/java/io/github/manasmods/manascore/inventory/mixin/MixinAbstractContainerScreen.java(1 hunks)inventory-fabric/build.gradle(1 hunks)inventory-fabric/src/main/java/io/github/manasmods/manascore/inventory/fabric/ManasCoreInventoryFabric.java(1 hunks)inventory-neoforge/build.gradle(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybinding.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingClient.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingCategory.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingManager.java(1 hunks)keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/ManasKeybinding.java(1 hunks)keybinding-fabric/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingFabric.java(1 hunks)keybinding-neoforge/build.gradle(1 hunks)keybinding-neoforge/gradle.properties(1 hunks)keybinding-neoforge/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingNeoForge.java(1 hunks)network-neoforge/build.gradle(1 hunks)settings.gradle(2 hunks)skill-common/build.gradle(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkill.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkillClient.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkill.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkillInstance.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillAPI.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillEvents.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/api/Skills.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillRegistry.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/TickingSkill.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/data/SkillTagProvider.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/InternalSkillPacketActions.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/ManasSkillNetwork.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillActivationPacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillReleasePacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillScrollPacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillTogglePacket.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinLivingEntity.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinPlayer.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/Changeable.java(1 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/EntityEvents.java(1 hunks)skill-fabric/build.gradle(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/ManasCoreSkillFabric.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractArrow.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractHurtingProjectile.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFireworkRocketEntity.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFishingHook.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLivingEntity.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLlamaSpit.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinMob.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinPlayer.java(1 hunks)
⛔ Files not processed due to max files limit (27)
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinShulkerBullet.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinThrowableProjectile.java
- skill-neoforge/build.gradle
- skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/ManasCoreSkillNeoForge.java
- skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/NeoForgeCommonEventInvoker.java
- storage-common/build.gradle
- storage-common/src/main/java/io/github/manasmods/manascore/storage/ManasCoreStorage.java
- storage-common/src/main/java/io/github/manasmods/manascore/storage/impl/StorageManager.java
- storage-common/src/main/java/io/github/manasmods/manascore/storage/mixin/MixinServerLevel.java
- storage-fabric/build.gradle
- storage-fabric/src/main/java/io/github/manasmods/manascore/storage/fabric/ManasCoreStorageFabric.java
- storage-fabric/src/main/java/io/github/manasmods/manascore/storage/fabric/mixin/MixinPlayerChunkSender.java
- storage-neoforge/src/main/java/io/github/manasmods/manascore/storage/neoforge/ManasCoreStorageNeoForge.java
- testing-common/build.gradle
- testing-common/src/main/java/io/github/manasmods/manascore/testing/ManasCoreTesting.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/client/KeybindingTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/client/ManasCoreTestingClient.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/module/CommandModuleTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/module/InventoryTabsTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/module/StorageModuleTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/RegistryTest.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestSkill.java
- testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestTags.java
- testing-fabric/build.gradle
- testing-fabric/src/main/java/io/github/manasmods/manascore/testing/fabric/ManasCoreTestingFabric.java
- testing-neoforge/build.gradle
- testing-neoforge/src/main/java/io/github/manasmods/manascore/testing/neoforge/ManasCoreTestingNeoForge.java
✅ Files skipped from review due to trivial changes (38)
- command-neoforge/build.gradle
- command-fabric/build.gradle
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Literal.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Bool.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Enum.java
- command-neoforge/gradle.properties
- inventory-neoforge/build.gradle
- command-common/src/main/java/io/github/manasmods/manascore/command/PlatformCommandUtils.java
- inventory-common/src/main/java/io/github/manasmods/manascore/inventory/VanillaInventoryTab.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Uuid.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/LongNumber.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/CommandRegistry.java
- inventory-common/src/main/java/io/github/manasmods/manascore/inventory/api/AbstractInventoryTab.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/Permission.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/CommandArgumentRegistrationEvent.java
- command-common/build.gradle
- command-fabric/src/main/java/io/github/manasmods/manascore/command/fabric/ManasCoreCommandFabric.java
- command-fabric/src/main/java/io/github/manasmods/manascore/command/fabric/integrations/FabricPermissionsApiIntegration.java
- network-neoforge/build.gradle
- inventory-fabric/build.gradle
- command-neoforge/src/main/java/io/github/manasmods/manascore/command/neoforge/PlatformCommandUtilsImpl.java
- command-common/src/main/java/io/github/manasmods/manascore/command/internal/CommandArgumentRegistry.java
- inventory-common/src/main/java/io/github/manasmods/manascore/inventory/client/widget/InventoryTabSwitcherWidget.java
- command-common/src/main/java/io/github/manasmods/manascore/command/internal/CommandCreationContext.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/Execute.java
- command-common/src/main/java/io/github/manasmods/manascore/command/CommandAnnotationHandler.java
- command-fabric/src/main/java/io/github/manasmods/manascore/command/fabric/PlatformCommandUtilsImpl.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Text.java
- command-common/src/main/java/io/github/manasmods/manascore/command/ManasCoreCommand.java
- command-neoforge/src/main/java/io/github/manasmods/manascore/command/neoforge/ManasCoreCommandNeoForge.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/FloatNumber.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/IntNumber.java
- inventory-common/src/main/java/io/github/manasmods/manascore/inventory/client/ManasCoreInventoryClient.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/DoubleNumber.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/Command.java
- inventory-common/build.gradle
- inventory-common/src/main/java/io/github/manasmods/manascore/inventory/mixin/MixinAbstractContainerScreen.java
- command-common/src/main/java/io/github/manasmods/manascore/command/api/parameter/Sender.java
🚧 Files skipped from review as they are similar to previous changes (33)
- keybinding-neoforge/gradle.properties
- inventory-fabric/src/main/java/io/github/manasmods/manascore/inventory/fabric/ManasCoreInventoryFabric.java
- skill-common/build.gradle
- settings.gradle
- keybinding-fabric/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingFabric.java
- keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingClient.java
- keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybinding.java
- skill-fabric/build.gradle
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/ManasCoreSkillFabric.java
- keybinding-neoforge/build.gradle
- keybinding-neoforge/src/main/java/io/github/manasmods/manascore/keybinding/ManasCoreKeybindingNeoForge.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLivingEntity.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/data/SkillTagProvider.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/ManasSkillNetwork.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/utils/Changeable.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkillClient.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFishingHook.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinFireworkRocketEntity.java
- skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractArrow.java
- keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingManager.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkillInstance.java
- config-neoforge/src/main/java/io/github/manasmods/manascore/config/neoforge/ManasCoreConfigNeoForge.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillActivationPacket.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/ManasCoreSkill.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillTogglePacket.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillReleasePacket.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinPlayer.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/network/c2s/RequestSkillScrollPacket.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/api/Skills.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/TickingSkill.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinLivingEntity.java
- skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillEvents.java
🔇 Additional comments (12)
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLlamaSpit.java (2)
20-21: Well-structured Mixin class!The class follows Mixin best practices with proper annotation and abstract declaration.
22-30: Verify consistent implementation across projectile mixins.Since this Mixin modifies core projectile behavior, we should ensure consistency with other projectile-related mixins.
Let's check for similar patterns in other projectile mixins:
✅ Verification successful
Implementation verified as consistent with projectile mixin patterns
The implementation follows the standard pattern used across other projectile mixins in the codebase, particularly matching MixinThrowableProjectile and MixinShulkerBullet exactly. MixinAbstractArrow has justified variations due to arrow-specific mechanics but maintains the same core event-driven approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other projectile-related mixins and their event usage echo "Checking for other projectile mixins..." rg -l "ProjectileDeflection" --type java echo "Checking for PROJECTILE_HIT event usage..." rg -l "PROJECTILE_HIT" --type java echo "Checking for similar @WrapOperation patterns..." ast-grep --pattern 'class $_ { @WrapOperation(method = "tick") $_(ProjectileDeflection $_) { $$$ } }'Length of output: 2654
Script:
#!/bin/bash # Check implementation patterns in other projectile mixins echo "Checking MixinThrowableProjectile implementation..." rg -A 10 "@WrapOperation.*tick" skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinThrowableProjectile.java echo "Checking MixinAbstractArrow implementation..." rg -A 10 "@WrapOperation.*tick" skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractArrow.java echo "Checking MixinShulkerBullet implementation..." rg -A 10 "@WrapOperation.*tick" skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinShulkerBullet.javaLength of output: 4257
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinAbstractHurtingProjectile.java (1)
27-29: Simplify return value handlingThe current implementation calls the original operation but ignores its result, always returning the deflection from the changeable. Consider whether the original operation's result should be used when no custom deflection is set.
- if (resultChangeable.get() != EntityEvents.ProjectileHitResult.DEFAULT) return deflectionChangeable.get(); - original.call(instance, result); - return deflectionChangeable.get(); + ProjectileDeflection originalDeflection = original.call(instance, result); + return deflectionChangeable.get() != ProjectileDeflection.NONE ? deflectionChangeable.get() : originalDeflection;Let's verify if there are any other mixins or event handlers that might rely on the current behavior:
keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/KeybindingCategory.java (1)
15-17: Add null check for ModuleConstants.MOD_ID.The
getCategoryString()method assumesModuleConstants.MOD_IDis non-null. Consider adding a null check.keybinding-common/src/main/java/io/github/manasmods/manascore/keybinding/api/ManasKeybinding.java (1)
15-127: Add missing @OverRide annotationsThe class extends
KeyMappingbut lacks@Overrideannotations on inherited methods.skill-common/src/main/java/io/github/manasmods/manascore/skill/api/ManasSkill.java (1)
87-89: Use correct constructor forResourceLocationThe method
ResourceLocation.fromNamespaceAndPathmay not exist. Instead, you should use the constructornew ResourceLocation(namespace, path)to create aResourceLocationinstance.Apply this diff to fix the issue:
- return ResourceLocation.fromNamespaceAndPath(id.getNamespace(), "icons/skills/" + id.getPath()); + return new ResourceLocation(id.getNamespace(), "icons/skills/" + id.getPath());skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinPlayer.java (1)
20-25: Ensure correct use of@ModifyVariableand local variablesThe use of
@ModifyVariablewith@Local(argsOnly = true)assumes thatdamageSourceis a local variable within the target method at the specified injection point. Verify thatdamageSourceis indeed available as a local variable at ordinal 6 in theactuallyHurtmethod ofPlayerclass. If not, this mixin may fail at runtime.Please confirm the availability of
damageSourceat the injection point. You can use the following script to decompile the target method and inspect the local variables:skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinMob.java (1)
19-25: Verify parameters in@WrapOperationmethodIn the mixin method
onSetTarget, ensure that the parameters match the expected signature when using@WrapOperation. TheOperationparameter should be last, and@WrapOperationmethods may require additional annotations or adjustments depending on the Mixin version.Please verify that the method signature correctly matches the expectations of the Mixin framework. Refer to the Mixin documentation for
@WrapOperationto ensure compatibility.skill-common/src/main/java/io/github/manasmods/manascore/skill/api/SkillAPI.java (2)
19-22: LGTM! Well-structured utility class.The class follows the utility class pattern with a private constructor to prevent instantiation.
42-44: Add runtime null validation.While the method uses @nonnull annotation, it lacks runtime validation for the entity parameter.
skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillRegistry.java (2)
25-30: Correct Generic Type Parameters in Registrar Builder.The generic type parameter specification in the Registrar builder needs correction.
110-118: Potential Null Pointer Exception inPROJECTILE_HITEvent.The event handler needs null checking for the entity from hitResult.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (18)
skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/NeoForgeCommonEventInvoker.java (3)
1-4: Update the copyright year to 2024.The copyright notice shows 2025, which is incorrect as it's currently 2024.
- * Copyright (c) 2025. ManasMods + * Copyright (c) 2024. ManasMods
36-44: Add validation for negative damage values.The modified damage value should be validated to ensure it's not negative.
if (EntityEvents.LIVING_DAMAGE.invoker().damage(e.getEntity(), e.getSource(), changeableDamage).isFalse()) { e.setNewDamage(0); } else { - e.setNewDamage(changeableDamage.get()); + float newDamage = changeableDamage.get(); + e.setNewDamage(Math.max(0, newDamage)); }
20-21: Add class-level documentation.The class would benefit from documentation explaining its role in the skill system and how it bridges NeoForge events with the ManasCore skill system.
@EventBusSubscriber +/** + * Bridges NeoForge events with ManasCore's skill system. + * This class listens to NeoForge events and translates them into skill system events, + * allowing skills to modify entity targeting, damage, and projectile behavior. + */ public class NeoForgeCommonEventInvoker {skill-neoforge/src/main/resources/META-INF/neoforge.mods.toml (1)
3-3: Add issue tracker URL.An empty issue tracker URL makes it difficult for users to report issues. Consider adding the GitHub repository's issue tracker URL.
-issueTrackerURL = "" +issueTrackerURL = "https://github.com/ManasMods/ManasCore/issues"testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestSkill.java (2)
40-44: Consider adding @OverRide annotations to overridden methodsAdding
@Overrideannotations to methods that override superclass methods enhances code readability and ensures that any discrepancies in method signatures are caught at compile time.
126-138: Review damage handling for balance considerationsThe method
onDamageEntitymodifies damage amounts significantly, such as instant-killing Creepers and increasing damage to Players tenfold. While acceptable for testing, consider the impact on game balance if this code is used beyond testing scenarios.keybind-common/src/main/java/io/github/manasmods/manascore/keybind/api/KeybindingCategory.java (2)
11-14: Add input validation and documentation for the name parameter.Consider adding:
- Input validation to ensure the name parameter is not null or empty
- JavaDoc explaining the expected format and constraints for the name parameter
@RequiredArgsConstructor(staticName = "of") public class KeybindingCategory { + /** + * @param name The category name. Must not be null or empty. + * Should only contain lowercase letters, numbers, and underscores. + */ private final String name; + + public static KeybindingCategory of(String name) { + if (name == null || name.trim().isEmpty()) { + throw new IllegalArgumentException("Category name must not be null or empty"); + } + return new KeybindingCategory(name.trim()); + }
15-17: Consider externalizing the category string format.The hardcoded format string could be problematic for internationalization. Consider making it configurable.
+ private static final String CATEGORY_FORMAT = "%s.category.%s"; + public String getCategoryString() { - return String.format("%s.category.%s", ModuleConstants.MOD_ID, this.name); + return String.format(CATEGORY_FORMAT, ModuleConstants.MOD_ID, this.name); }keybind-common/src/main/java/io/github/manasmods/manascore/keybind/api/ManasKeybinding.java (2)
98-103: Add cleanup mechanism for stale entries.The current implementation might lead to memory leaks if a key release event is missed. Consider adding a cleanup mechanism or timeout for stale entries in the
PRESSED_KEYBINDINGSmap.this.action = () -> { if (!PRESSED_KEYBINDINGS.containsKey(this)) { + // Clean up stale entries older than 5 minutes + long currentTime = System.currentTimeMillis(); + PRESSED_KEYBINDINGS.entrySet().removeIf(entry -> + (currentTime - entry.getValue()) > 300000); PRESSED_KEYBINDINGS.put(this, System.currentTimeMillis()); action.onPress(); } };
107-108: Consider using System.nanoTime() for better precision.For accurate duration measurements,
System.nanoTime()provides better precision thancurrentTimeMillis(). This is especially important for gaming input handling.- long start = PRESSED_KEYBINDINGS.remove(this); - long end = System.currentTimeMillis(); + long start = PRESSED_KEYBINDINGS.remove(this); + long end = System.nanoTime(); + long durationNanos = end - start; + long durationMillis = durationNanos / 1_000_000;keybind-fabric/src/main/resources/fabric.mod.json (2)
10-12: Consider adding homepage URL.The homepage field is currently empty. Consider adding the GitHub repository URL for better discoverability.
"contact": { - "homepage": "" + "homepage": "https://github.com/ManasMods/ManasCore" },
28-28: Consider supporting older Java versions.Java 21 is a very recent version. Consider supporting Java 17 (LTS) to increase adoption, as many users might not have upgraded yet.
keybind-neoforge/src/main/resources/META-INF/neoforge.mods.toml (1)
3-3: Add issue tracker URL.The issue tracker URL is empty. Consider adding the GitHub issues URL for better user support.
-issueTrackerURL = "" +issueTrackerURL = "https://github.com/ManasMods/ManasCore/issues"build.gradle (1)
270-278: Consider using a more robust approach for modifying configuration files.The current string-based modification of configuration files could be fragile. Consider:
- Using a proper TOML parser for neoforge.mods.toml
- Adding error handling for malformed files
- Validating the modified content before writing
Example approach using a TOML parser:
def toml = new TomlParser().parse(file('META-INF/neoforge.mods.toml')) if (project.name.startsWith("skill")) { toml.accessWidener = "manascore_skill-neoforge.accesswidener" } file.text = toml.write()keybind-neoforge/src/main/java/io/github/manasmods/manascore/keybind/neoforge/ManasCoreKeybindNeoForge.java (2)
1-4: Update the copyright year to the current year.The copyright year is set to 2025, which is in the future. Consider updating it to 2024 or using a range (e.g., 2024-2025) if you want to future-proof it.
- * Copyright (c) 2025. ManasMods + * Copyright (c) 2024. ManasMods
12-17: Enhance robustness and documentation.Consider the following improvements:
- Add JavaDoc to explain the class purpose and why inheritance is prevented
- Add error handling around the init() call
- Add logging to indicate successful initialization
Here's a suggested implementation:
+/** + * NeoForge entry point for the ManasCore Keybinding module. + * This class is marked final as it serves as a singleton mod instance. + */ @Mod(ModuleConstants.MOD_ID) public final class ManasCoreKeybindNeoForge { + private static final Logger LOGGER = LogUtils.getLogger(); + public ManasCoreKeybindNeoForge() { + try { ManasCoreKeybind.init(); + LOGGER.info("ManasCore Keybinding module initialized successfully"); + } catch (Exception e) { + LOGGER.error("Failed to initialize ManasCore Keybinding module", e); + throw e; + } } }skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/mixin/MixinPlayer.java (1)
28-32: Extract common damage handling logicThis code duplicates the logic from MixinLivingEntity. Consider extracting the common damage handling logic into a shared utility class.
Create a new utility class:
public class DamageHandlingUtils { public static boolean handleDamage(LivingEntity entity, DamageSource source, float amount, CallbackInfo ci, Stack<DamageContainer> containers) { Changeable<Float> changeable = Changeable.of(amount); if (EntityEvents.LIVING_HURT.invoker().hurt(entity, source, changeable).isFalse()) { ci.cancel(); return false; } Float newDamage = changeable.get(); if (newDamage != null && !containers.isEmpty()) { containers.peek().setNewDamage(newDamage); } return true; } }Then use it in both mixins:
void onHurt(DamageSource source, float amount, CallbackInfo ci) { - Changeable<Float> changeable = Changeable.of(amount); - if (EntityEvents.LIVING_HURT.invoker().hurt((Player) (Object) this, source, changeable).isFalse()) ci.cancel(); - else damageContainers.peek().setNewDamage(changeable.get()); + DamageHandlingUtils.handleDamage((Player)(Object)this, source, amount, ci, damageContainers); }skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinPlayer.java (1)
20-40: Consider a unified cross-platform damage handling approachThe current implementation duplicates damage handling logic across four different mixin classes (NeoForge and Fabric for both Player and LivingEntity).
Consider these improvements:
- Create a common damage handling interface:
public interface DamageHandler { float modifyDamage(float amount, DamageSource source, LivingEntity entity); boolean shouldCancelDamage(float amount, DamageSource source, LivingEntity entity); float modifyTotalDamage(float amount, DamageSource source, LivingEntity entity); }
- Implement platform-specific handlers:
public class FabricDamageHandler implements DamageHandler { // Fabric-specific implementation } public class NeoForgeDamageHandler implements DamageHandler { // NeoForge-specific implementation }
- Use a factory pattern to get the appropriate handler:
public class DamageHandlerFactory { public static DamageHandler getHandler() { // Return platform-specific handler } }This approach would:
- Reduce code duplication
- Make platform differences explicit
- Simplify maintenance
- Make testing easier
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
keybind-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngkeybind-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngkeybind-neoforge/src/main/resources/icon.pngis excluded by!**/*.png
📒 Files selected for processing (32)
build.gradle(1 hunks)keybind-common/build.gradle(1 hunks)keybind-common/src/main/java/io/github/manasmods/manascore/keybind/ManasCoreKeybind.java(1 hunks)keybind-common/src/main/java/io/github/manasmods/manascore/keybind/ManasCoreKeybindClient.java(1 hunks)keybind-common/src/main/java/io/github/manasmods/manascore/keybind/api/KeybindingCategory.java(1 hunks)keybind-common/src/main/java/io/github/manasmods/manascore/keybind/api/KeybindingManager.java(1 hunks)keybind-common/src/main/java/io/github/manasmods/manascore/keybind/api/ManasKeybinding.java(1 hunks)keybind-common/src/main/resources/architectury.common.json(1 hunks)keybind-common/src/main/resources/manascore_keybind.accesswidener(1 hunks)keybind-common/src/main/resources/manascore_keybind.mixins.json(1 hunks)keybind-fabric/build.gradle(1 hunks)keybind-fabric/src/main/java/io/github/manasmods/manascore/keybind/fabric/ManasCoreKeybindFabric.java(1 hunks)keybind-fabric/src/main/resources/fabric.mod.json(1 hunks)keybind-neoforge/build.gradle(1 hunks)keybind-neoforge/gradle.properties(1 hunks)keybind-neoforge/src/main/java/io/github/manasmods/manascore/keybind/neoforge/ManasCoreKeybindNeoForge.java(1 hunks)keybind-neoforge/src/main/resources/META-INF/neoforge.mods.toml(1 hunks)settings.gradle(2 hunks)skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinLivingEntity.java(1 hunks)skill-common/src/main/resources/manascore_skill.accesswidener(1 hunks)skill-common/src/main/resources/manascore_skill.mixins.json(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLivingEntity.java(1 hunks)skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinPlayer.java(1 hunks)skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/NeoForgeCommonEventInvoker.java(1 hunks)skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/mixin/MixinLivingEntity.java(1 hunks)skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/mixin/MixinPlayer.java(1 hunks)skill-neoforge/src/main/resources/META-INF/neoforge.mods.toml(1 hunks)skill-neoforge/src/main/resources/manascore_skill-neoforge.mixins.json(1 hunks)testing-common/build.gradle(2 hunks)testing-common/src/main/java/io/github/manasmods/manascore/testing/client/KeybindingTest.java(1 hunks)testing-common/src/main/java/io/github/manasmods/manascore/testing/module/CommandModuleTest.java(3 hunks)testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestSkill.java(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- keybind-neoforge/gradle.properties
- keybind-common/src/main/resources/architectury.common.json
- keybind-common/src/main/resources/manascore_keybind.accesswidener
- keybind-fabric/build.gradle
- keybind-common/build.gradle
- keybind-common/src/main/resources/manascore_keybind.mixins.json
- skill-neoforge/src/main/resources/manascore_skill-neoforge.mixins.json
🚧 Files skipped from review as they are similar to previous changes (4)
- skill-common/src/main/resources/manascore_skill.mixins.json
- skill-common/src/main/resources/manascore_skill.accesswidener
- skill-common/src/main/java/io/github/manasmods/manascore/skill/mixin/MixinLivingEntity.java
- settings.gradle
🔇 Additional comments (19)
skill-neoforge/src/main/java/io/github/manasmods/manascore/skill/neoforge/NeoForgeCommonEventInvoker.java (1)
20-24: Well-structured event invoker setup!Good practices observed:
- Appropriate use of @EventBusSubscriber annotation
- Private constructor prevents instantiation, which is correct for a utility class
skill-neoforge/src/main/resources/META-INF/neoforge.mods.toml (4)
14-14: Verify logo file existence.Ensure that
icon.pngexists in the correct resources directory.✅ Verification successful
Logo file verification successful
The referenced
icon.pngexists at the correct location in the resources directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the icon file exists in the resources directory fd -t f "icon.png" skill-neoforge/src/main/resources/Length of output: 96
19-19: Consider constraining NeoForge version range.Similar to the loader version, the NeoForge version range
[21,)is very broad. Consider setting an upper bound to ensure compatibility.
1-2: Consider constraining the loader version range.The version range
[2,)is very broad and might include incompatible future versions. Consider setting an upper bound to ensure compatibility.✅ Verification successful
Consider using a more specific loader version range
The current range
[2,)is too broad. Based on the NeoForge version patterns, consider using[2,3)to ensure compatibility with the current major version while preventing potential issues with future major versions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the latest NeoForge versions to determine an appropriate upper bound curl -s https://maven.neoforged.net/releases/net/neoforged/neoforge/maven-metadata.xml | grep -oP '<version>\K[^<]+'Length of output: 10141
37-42: Verify circular dependency.The
manascore_networkdependency might create a circular dependency since it's using the same version as this module. Please verify the dependency structure.✅ Verification successful
No circular dependency detected
The dependency structure appears to be hierarchical, with
manascore_networkserving as a core module that other feature modules depend on. Using the same version across modules is a common practice in monorepo setups where components are released together. No reverse dependencies were found that would create a circular relationship.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential circular dependencies in other mod configuration files fd -t f "mods.toml" | xargs rg "modId.*=.*manascore_skill"Length of output: 58
Script:
#!/bin/bash # Find the mod_id value and dependencies in both directions echo "=== Searching for mod_id definition ===" fd -t f "mods.toml" | xargs rg "mod_id.*=|modId.*=.*skill" echo -e "\n=== Searching for manascore dependencies ===" fd -t f "mods.toml" | xargs rg "modId.*=.*manascore" echo -e "\n=== Searching for version definition ===" fd -t f "gradle.properties" | xargs catLength of output: 1568
testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestSkill.java (3)
43-43: Verify the usage ofResourceLocation.withDefaultNamespaceIn the constructor, you use
ResourceLocation.withDefaultNamespace("skill.speed"). Please verify that this method correctly creates aResourceLocationwith the intended namespace. If not, consider usingnew ResourceLocation("skill.speed")for clarity.
118-124: Ensure damage cancellation works as expectedIn the
onBeingDamagedmethod, returningfalsewhen the source is cactus indicates that the damage should be canceled. Confirm that this behavior appropriately prevents damage from cacti in the game logic.
150-157: Check healing logic in damage taken eventIn the
onTakenDamagemethod, when the owner is sneaking, they heal the amount of damage taken. Ensure this logic aligns with intended gameplay mechanics and doesn't introduce unintended invincibility.keybind-fabric/src/main/java/io/github/manasmods/manascore/keybind/fabric/ManasCoreKeybindFabric.java (1)
10-14: Initialization code is correctly implementedThe
ManasCoreKeybindFabricclass correctly implements theModInitializerinterface and callsManasCoreKeybind.init()inonInitialize().keybind-common/src/main/java/io/github/manasmods/manascore/keybind/ManasCoreKeybind.java (1)
11-16: Proper client-side initialization checkThe
init()method appropriately checks the environment and initializes client-specific keybinds when running on the client side.keybind-common/src/main/java/io/github/manasmods/manascore/keybind/ManasCoreKeybindClient.java (1)
11-15: Clean implementation of client-side initialization!The class follows single responsibility principle and correctly uses Architectury's event system for client-side initialization.
keybind-neoforge/build.gradle (1)
6-12: LGTM! Build configuration is properly set up.The access widener path is correctly configured and the jar remapping is properly set up.
testing-common/build.gradle (1)
11-14: LGTM! Module additions align with PR objectives.The new modules
:skill,:keybind, and:confighave been properly added to theincludedProjectslist, maintaining consistency with the existing structure.keybind-fabric/src/main/resources/fabric.mod.json (1)
16-21: Verify if client entrypoints are needed for keybinding functionality.The keybinding module typically requires client-side initialization, but the client entrypoints array is empty.
keybind-neoforge/src/main/java/io/github/manasmods/manascore/keybind/neoforge/ManasCoreKeybindNeoForge.java (2)
6-10: LGTM!The package structure follows Java conventions, and imports are minimal and necessary.
14-16: Verify the initialization sequence.Based on the AI summary,
ManasCoreKeybind.init()should only execute client-side code when in the client environment. Let's verify this implementation:✅ Verification successful
Initialization sequence is properly implemented
The code correctly ensures client-side initialization by checking the environment before executing client-specific code. This follows the established pattern used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization sequence and environment handling # Check ManasCoreKeybind.init() implementation ast-grep --pattern 'class ManasCoreKeybind { $$$ static void init() { $$$ } $$$ }' # Check for proper environment checks rg -A 5 'Env\.CLIENT'Length of output: 5669
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinLivingEntity.java (2)
33-38: Add null check and document the ordinal valueThe previous review comment about adding null checks and documenting the ordinal value is still valid.
21-26:⚠️ Potential issueAdd null check for damage modification
Similar to the NeoForge implementation, add null safety checks.
Apply these changes:
float modifyDamage(float amount, @Local(argsOnly = true) DamageSource damageSource) { Changeable<Float> changeable = Changeable.of(amount); - if (EntityEvents.LIVING_HURT.invoker().hurt((LivingEntity) (Object) this, damageSource, changeable).isFalse()) return 0.0F; - return changeable.get(); + if (EntityEvents.LIVING_HURT.invoker().hurt((LivingEntity) (Object) this, damageSource, changeable).isFalse()) { + return 0.0F; + } + Float newDamage = changeable.get(); + return newDamage != null ? newDamage : amount; }Likely invalid or redundant comment.
skill-fabric/src/main/java/io/github/manasmods/manascore/skill/fabric/mixin/MixinPlayer.java (1)
1-40: Enhance test coverage for damage handlingThe damage handling modifications are critical to gameplay, but only basic tests are present in
test_coderabbit_num.py.Consider adding these test scenarios:
- Null damage values
- Empty damage containers
- Event cancellation
- Platform-specific behavior differences
Run this script to analyze current test coverage:
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (21)
attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/mixin/MixinLivingEntity.java (1)
24-26: Injection point atHEADcan cause conflicts with other mixins.
Injecting at the head ofhurtis fine but can conflict if other mixins also inject at the head for the same method in a different mod. Consider using a more precise injection point (e.g., BEFORE a particular line or method call) if you foresee potential mod conflicts.attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/mixin/MixinPlayer.java (2)
26-37: Consider extracting repeated critical-hit logic into a shared helper method.
The logic for determining multipliers, chance, and triggering critical events appears similar to the code in lines 42-55. Extracting this logic to a single private or utility method would reduce duplication and improve maintainability.Example refactor sketch:
+private float applyCritLogic(LivingEntity attacker, LivingEntity target, float baseDamage) { + Changeable<Float> multiplier = Changeable.of((float) attacker.getAttributeValue(ManasCoreAttributes.CRITICAL_DAMAGE_MULTIPLIER)); + Changeable<Double> chance = Changeable.of(attacker.getAttributeValue(ManasCoreAttributes.CRITICAL_ATTACK_CHANCE) / 100); + if (AttributeEvents.CRITICAL_ATTACK_CHANCE_EVENT.invoker().applyCrit(attacker, target, 1, multiplier, chance).isFalse()) { + return baseDamage; + } + if (target.getRandom().nextFloat() > chance.get()) { + return baseDamage; + } + ManasCoreAttributeUtils.triggerCriticalAttackEffect(target, attacker); + return baseDamage * multiplier.get(); +}
42-55: Clarify method name or add documentation for damage modifications.
The method signaturegetCritChanceDamagealso adjusts the final damage to incorporate enchantment or events. Consider documenting the exact logic and side effects, or rename to reflect that it may perform advanced critical-hit checks that include enchantment.attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/ManasCoreAttributeRegisterImpl.java (4)
1-4: Update the copyright year.The copyright year is set to 2025, which is in the future. Consider updating it to the current year (2024).
- * Copyright (c) 2025. ManasMods + * Copyright (c) 2024. ManasMods
25-27: Improve entityTypes initialization.Consider the following improvements:
- Add
@SuppressWarnings("unchecked")to acknowledge the unchecked cast- Make the field
finalfor immutability- Combine filter and map operations for better readability
+ @SuppressWarnings("unchecked") - private static final List<EntityType<? extends LivingEntity>> entityTypes = BuiltInRegistries.ENTITY_TYPE.stream() - .filter(DefaultAttributes::hasSupplier).map(entityType -> (EntityType<? extends LivingEntity>) entityType) - .collect(Collectors.toList()); + private static final List<EntityType<? extends LivingEntity>> ENTITY_TYPES = BuiltInRegistries.ENTITY_TYPE.stream() + .filter(entityType -> DefaultAttributes.hasSupplier(entityType)) + .map(entityType -> (EntityType<? extends LivingEntity>) entityType) + .collect(Collectors.toUnmodifiableList());
29-35: Enhance registration methods with validation and documentation.The registration methods could benefit from:
- Null parameter validation
- Duplicate entry prevention
- JavaDoc documentation
+ /** + * Registers an attribute to be applied to player entities. + * @param holder The attribute holder to register + * @throws NullPointerException if holder is null + */ public static void registerToPlayers(Holder<Attribute> holder) { + Objects.requireNonNull(holder, "Attribute holder cannot be null"); + if (!PLAYER_REGISTRY.contains(holder)) { PLAYER_REGISTRY.add(holder); + } } + /** + * Registers an attribute to be applied to all living entities. + * @param holder The attribute holder to register + * @throws NullPointerException if holder is null + */ public static void registerToGeneric(Holder<Attribute> holder) { + Objects.requireNonNull(holder, "Attribute holder cannot be null"); + if (!GENERIC_REGISTRY.contains(holder)) { GENERIC_REGISTRY.add(holder); + } }
22-61: Consider architectural improvements for robustness.The current implementation could benefit from the following architectural improvements:
- Add logging for debugging and monitoring
- Implement registration verification
- Add error handling for registration failures
- Consider adding a registration status query mechanism
Would you like me to provide a detailed implementation for these improvements?
attribute-common/src/main/java/io/github/manasmods/manascore/attribute/ManasCoreAttribute.java (1)
3-7: Add class-level documentation.The initialization logic is clean and follows good separation of concerns. Consider adding Javadoc to describe the class's purpose and initialization sequence.
Add class documentation:
+/** + * Common entry point for ManasCore attribute system initialization. + * This class delegates the initialization to platform-specific implementations + * through the ManasCoreAttributeRegister. + */ public class ManasCoreAttribute {attribute-neoforge/src/main/java/io/github/manasmods/manascore/attribute/neoforge/ManasCoreAttributeNeoForge.java (1)
7-12: Add error handling around initialization.While the implementation follows NeoForge conventions, consider adding error handling around the initialization call to gracefully handle potential failures.
@Mod(ModuleConstants.MOD_ID) public final class ManasCoreAttributeNeoForge { public ManasCoreAttributeNeoForge() { + try { ManasCoreAttribute.init(); + } catch (Exception e) { + LOGGER.error("Failed to initialize ManasCore attributes", e); + throw new RuntimeException("Failed to initialize ManasCore attributes", e); + } } + + private static final org.slf4j.Logger LOGGER = org.slf4j.LoggerFactory.getLogger(ManasCoreAttributeNeoForge.class); }attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/ManasCoreAttributeFabric.java (2)
1-4: Fix copyright year.The copyright year is set to 2025, which appears to be incorrect as it's in the future.
/* - * Copyright (c) 2025. ManasMods + * Copyright (c) 2024. ManasMods * GNU General Public License 3 */
11-16: Improve consistency with NeoForge implementation.Consider the following improvements for consistency:
- Mark class as final (like NeoForge implementation)
- Add error handling around initialization
- Add logging support
-public class ManasCoreAttributeFabric implements ModInitializer { +public final class ManasCoreAttributeFabric implements ModInitializer { + private static final org.slf4j.Logger LOGGER = org.slf4j.LoggerFactory.getLogger(ManasCoreAttributeFabric.class); + @Override public void onInitialize() { + try { ManasCoreAttribute.init(); + } catch (Exception e) { + LOGGER.error("Failed to initialize ManasCore attributes", e); + throw new RuntimeException("Failed to initialize ManasCore attributes", e); + } } }attribute-common/src/main/java/io/github/manasmods/manascore/attribute/ManasCoreAttributeRegister.java (1)
12-27: Add class-level documentation to explain the platform-specific implementation pattern.The class serves as a platform-agnostic registration point for attributes, but this isn't immediately clear to other developers. Consider adding class-level JavaDoc to explain:
- The purpose of the register
- How platform-specific implementations should be provided
- Usage examples for each method
+/** + * Platform-agnostic attribute registration handler. + * Platform-specific implementations must be provided for each method. + * <p> + * Usage: + * - Use {@link #registerToPlayers} to register attributes that apply to players + * - Use {@link #registerToGeneric} to register attributes that apply to any entity + * - Call {@link #init} during mod initialization to set up the attribute system + */ public class ManasCoreAttributeRegister {attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/Changeable.java (2)
11-48: Enhance thread safety and null handling.The current implementation has several areas for improvement:
- Consider using
volatileorAtomicReferenceinstead of method-level synchronization- Add immutable access to the original value
- Strengthen null handling with Optional
+/** + * Thread-safe wrapper for mutable values that tracks changes from an original value. + * @param <T> The type of the wrapped value + */ public class Changeable<T> { @Nullable private final T original; - private T value; + private volatile T value; protected Changeable(@Nullable T value) { this.original = value; this.value = value; } public static <T> Changeable<T> of(@Nullable T value) { return new Changeable<>(value); } - @Synchronized @Nullable public T get() { return value; } - @Synchronized public void set(@Nullable T value) { this.value = value; } + /** + * @return The original value this instance was created with + */ + @Nullable + public T getOriginal() { + return original; + } + + /** + * @return Optional wrapper of the current value + */ + public Optional<T> getOptional() { + return Optional.ofNullable(value); + } public boolean isPresent() { return value != null; } public boolean isEmpty() { return value == null; } public boolean hasChanged() { - if (original == null) return value != null; - return !original.equals(value); + return !Objects.equals(original, value); } }
25-34: Consider adding atomic operations for thread safety.The current synchronized methods could be replaced with atomic operations for better performance in concurrent scenarios.
- @Synchronized - @Nullable - public T get() { - return value; - } - - @Synchronized - public void set(@Nullable T value) { - this.value = value; - } + private final AtomicReference<T> valueRef; + + protected Changeable(@Nullable T value) { + this.original = value; + this.valueRef = new AtomicReference<>(value); + } + + @Nullable + public T get() { + return valueRef.get(); + } + + public void set(@Nullable T value) { + valueRef.set(value); + } + + public boolean compareAndSet(@Nullable T expect, @Nullable T update) { + return valueRef.compareAndSet(expect, update); + }attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/ManasCoreAttributes.java (2)
27-29: Consider adding validation for critical attack chance.While the range is set correctly (0-100), consider adding runtime validation in case the attribute is modified during gameplay to ensure it stays within bounds.
public static final Holder<Attribute> CRITICAL_ATTACK_CHANCE = registerGenericAttribute(ModuleConstants.MOD_ID, "critical_attack_chance", "manascore.attribute.critical_attack_chance", - 0, 0, 100, true, Attribute.Sentiment.POSITIVE); + 0, 0, 100, true, Attribute.Sentiment.POSITIVE) { + @Override + public double sanitizeValue(double value) { + return Mth.clamp(value, 0.0D, 100.0D); + } +};
31-37: Consider consolidating duplicate registration methods.The
registerPlayerAttributeandregisterGenericAttributemethods are nearly identical except for the final registration call. Consider refactoring to reduce code duplication.+private static Holder<Attribute> registerAttribute(String modID, String id, String name, double amount, + double min, double max, boolean syncable, + Attribute.Sentiment sentiment, + Consumer<Holder<Attribute>> registrationHandler) { + Holder<Attribute> attribute = Registry.registerForHolder(BuiltInRegistries.ATTRIBUTE, + getResourceKey(modID, id), + new RangedAttribute(name, amount, min, max) + .setSyncable(syncable) + .setSentiment(sentiment)); + registrationHandler.accept(attribute); + return attribute; +} public static Holder<Attribute> registerPlayerAttribute(String modID, String id, String name, double amount, double min, double max, boolean syncable, Attribute.Sentiment sentiment) { - Holder<Attribute> attribute = Registry.registerForHolder(BuiltInRegistries.ATTRIBUTE, - getResourceKey(modID, id), - new RangedAttribute(name, amount, min, max) - .setSyncable(syncable) - .setSentiment(sentiment)); - ManasCoreAttributeRegister.registerToPlayers(attribute); - return attribute; + return registerAttribute(modID, id, name, amount, min, max, syncable, sentiment, + ManasCoreAttributeRegister::registerToPlayers); } public static Holder<Attribute> registerGenericAttribute(String modID, String id, String name, double amount, double min, double max, boolean syncable, Attribute.Sentiment sentiment) { - Holder<Attribute> attribute = Registry.registerForHolder(BuiltInRegistries.ATTRIBUTE, - getResourceKey(modID, id), - new RangedAttribute(name, amount, min, max) - .setSyncable(syncable) - .setSentiment(sentiment)); - ManasCoreAttributeRegister.registerToGeneric(attribute); - return attribute; + return registerAttribute(modID, id, name, amount, min, max, syncable, sentiment, + ManasCoreAttributeRegister::registerToGeneric); }Also applies to: 39-45
attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/ManasCoreAttributeUtils.java (1)
52-60: Consider caching trigonometric calculations.The
getLookTowardVecmethod performs multiple trigonometric calculations that could be optimized by caching intermediate results.public static Vec3 getLookTowardVec(Player player, double distance) { float f = player.getXRot(); float g = player.getYRot(); - float h = Mth.cos(-g * 0.017453292F - 3.1415927F); - float i = Mth.sin(-g * 0.017453292F - 3.1415927F); - float j = -Mth.cos(-f * 0.017453292F); - float k = Mth.sin(-f * 0.017453292F); + float yawRad = -g * 0.017453292F - 3.1415927F; + float pitchRad = -f * 0.017453292F; + float h = Mth.cos(yawRad); + float i = Mth.sin(yawRad); + float j = -Mth.cos(pitchRad); + float k = Mth.sin(pitchRad); return new Vec3(i * j * distance, k * distance, h * j * distance); }attribute-neoforge/src/main/java/io/github/manasmods/manascore/attribute/neoforge/NeoForgeCommonEventInvoker.java (1)
25-38: Consider extracting critical hit logic to a separate method.The
applyEntityCritmethod contains complex critical hit logic that could be extracted for better maintainability and reusability.+private static boolean shouldApplyCriticalHit(LivingEntity attacker, LivingEntity target, + Changeable<Float> multiplier, + Changeable<Double> chance) { + if (AttributeEvents.CRITICAL_ATTACK_CHANCE_EVENT.invoker() + .applyCrit(attacker, target, 1, multiplier, chance) + .isFalse()) { + return false; + } + return target.getRandom().nextFloat() <= chance.get(); +} @SubscribeEvent(priority = EventPriority.HIGH) public static void applyEntityCrit(final LivingIncomingDamageEvent e) { if (!(e.getSource().getDirectEntity() instanceof LivingEntity attacker)) return; if (attacker instanceof Player) return; LivingEntity target = e.getEntity(); Changeable<Float> multiplier = Changeable.of((float) attacker.getAttributeValue(ManasCoreAttributes.CRITICAL_DAMAGE_MULTIPLIER)); Changeable<Double> chance = Changeable.of(attacker.getAttributeValue(ManasCoreAttributes.CRITICAL_ATTACK_CHANCE) / 100); - if (AttributeEvents.CRITICAL_ATTACK_CHANCE_EVENT.invoker().applyCrit(attacker, target, 1, multiplier, chance).isFalse()) return; - - if (target.getRandom().nextFloat() > chance.get()) return; + if (!shouldApplyCriticalHit(attacker, target, multiplier, chance)) return; ManasCoreAttributeUtils.triggerCriticalAttackEffect(target, attacker); e.setAmount(e.getAmount() * multiplier.get()); }attribute-fabric/src/main/resources/fabric.mod.json (1)
11-12: Add homepage URL for better discoverability.The homepage field is empty. Consider adding a URL to the project's GitHub repository or documentation.
testing-common/src/main/resources/assets/manascore/lang/en_us.json (1)
8-11: Consider more descriptive potion names.The potion names are generic ("Test Potion", etc.). Consider using more descriptive names that reflect their effects.
- "item.minecraft.lingering_potion.effect.lmao_potion": "Test Lingering Potion", - "item.minecraft.splash_potion.effect.lmao_potion": "Test Splash Potion", - "item.minecraft.potion.effect.lmao_potion": "Test Potion", - "item.minecraft.tipped_arrow.effect.lmao_potion": "Test Tipped Arrow", + "item.minecraft.lingering_potion.effect.lmao_potion": "Test Explosive Lingering Potion", + "item.minecraft.splash_potion.effect.lmao_potion": "Test Explosive Splash Potion", + "item.minecraft.potion.effect.lmao_potion": "Test Explosive Potion", + "item.minecraft.tipped_arrow.effect.lmao_potion": "Test Explosive Arrow",attribute-neoforge/src/main/resources/META-INF/neoforge.mods.toml (1)
16-35: Consider using consistent version constraint format.The version constraints use different formats:
- NeoForge: Hardcoded
"[21,)"- Minecraft:
"[${minecraft_version},)"- Architectury:
"[${architectury_version},)"Consider using placeholders for NeoForge version as well to maintain consistency and ease version updates.
- versionRange = "[21,)" + versionRange = "[${neoforge_version},)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
attribute-fabric/src/main/resources/assets/manascore/icon.pngis excluded by!**/*.pngattribute-fabric/src/main/resources/assets/manascore/logo.pngis excluded by!**/*.pngattribute-neoforge/src/main/resources/icon.pngis excluded by!**/*.png
📒 Files selected for processing (30)
attribute-common/build.gradle(1 hunks)attribute-common/src/main/java/io/github/manasmods/manascore/attribute/ManasCoreAttribute.java(1 hunks)attribute-common/src/main/java/io/github/manasmods/manascore/attribute/ManasCoreAttributeRegister.java(1 hunks)attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/AttributeEvents.java(1 hunks)attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/Changeable.java(1 hunks)attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/ManasCoreAttributeUtils.java(1 hunks)attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/ManasCoreAttributes.java(1 hunks)attribute-common/src/main/resources/architectury.common.json(1 hunks)attribute-common/src/main/resources/assets/manascore/lang/en_us.json(1 hunks)attribute-common/src/main/resources/manascore_attribute.accesswidener(1 hunks)attribute-common/src/main/resources/manascore_attribute.mixins.json(1 hunks)attribute-fabric/build.gradle(1 hunks)attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/ManasCoreAttributeFabric.java(1 hunks)attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/ManasCoreAttributeRegisterImpl.java(1 hunks)attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/mixin/MixinLivingEntity.java(1 hunks)attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/mixin/MixinPlayer.java(1 hunks)attribute-fabric/src/main/resources/fabric.mod.json(1 hunks)attribute-fabric/src/main/resources/manascore_attribute-fabric.mixins.json(1 hunks)attribute-neoforge/build.gradle(1 hunks)attribute-neoforge/gradle.properties(1 hunks)attribute-neoforge/src/main/java/io/github/manasmods/manascore/attribute/neoforge/ManasCoreAttributeNeoForge.java(1 hunks)attribute-neoforge/src/main/java/io/github/manasmods/manascore/attribute/neoforge/ManasCoreAttributeRegisterImpl.java(1 hunks)attribute-neoforge/src/main/java/io/github/manasmods/manascore/attribute/neoforge/NeoForgeCommonEventInvoker.java(1 hunks)attribute-neoforge/src/main/resources/META-INF/neoforge.mods.toml(1 hunks)config-common/src/main/java/io/github/manasmods/manascore/config/ConfigRegistry.java(1 hunks)settings.gradle(2 hunks)testing-common/build.gradle(1 hunks)testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/RegistryTest.java(1 hunks)testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/TestAttributeRegistry.java(1 hunks)testing-common/src/main/resources/assets/manascore/lang/en_us.json(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- attribute-neoforge/gradle.properties
- attribute-common/build.gradle
- attribute-fabric/build.gradle
- attribute-common/src/main/resources/assets/manascore/lang/en_us.json
- attribute-fabric/src/main/resources/manascore_attribute-fabric.mixins.json
- attribute-common/src/main/resources/architectury.common.json
- attribute-common/src/main/resources/manascore_attribute.mixins.json
🚧 Files skipped from review as they are similar to previous changes (3)
- settings.gradle
- testing-common/build.gradle
- config-common/src/main/java/io/github/manasmods/manascore/config/ConfigRegistry.java
🔇 Additional comments (18)
attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/mixin/MixinLivingEntity.java (3)
1-4: License header looks appropriate.
No issues found with the licensing information.
8-9: Confirm mixin-extras version compatibility.
Make sure your project's build.gradle or equivalent includes the correct version of Mixin Extras, as this library is required for@Localinjection. Otherwise, this code may fail at runtime.
27-38: Enhance coverage of non-playerLivingEntityattacks.
- The code currently excludes players (
Playerinstance check), which seems intentional. However, note that indirect player attacks (e.g., arrows) are ignored. If you want to handle those, consider an explicit check for projectiles.- Ensure attribute values are valid (non-null), thus preventing edge cases where the attributes might not be present.
- Consider using the attacker’s random for the critical chance rather than the target’s random, to more consistently reflect the attacker’s capabilities.
attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/mixin/MixinPlayer.java (3)
1-4: Add or confirm inclusion of complete license texts.
While the header references the GNU General Public License v3, ensure that the project fully includes and distributes the relevant GPL license texts in accordance with compliance guidelines.
25-25: Verify local injection fornewAmount.
Injecting atHEADand referencing a local float might fail if the expected local variable doesn't exist or is re-ordered in the bytecode. Confirm that the method's local variable structure matches your injection site by verifying any relevant local variable mapping or specifying the correctordinalif needed.
57-60: Handle unexpected or absent attribute values.
When overriding the vanilla 1.5F critical multiplier, ensure that any missing or zero attribute value is handled gracefully to avoid inadvertently causing zero or negative multipliers. A sensible default or a safety clamp might be beneficial.attribute-fabric/src/main/java/io/github/manasmods/manascore/attribute/fabric/ManasCoreAttributeRegisterImpl.java (1)
37-60: 🛠️ Refactor suggestionImprove initialization robustness and documentation.
Several improvements could enhance the initialization logic:
- The null check suggests potential nulls in ENTITY_TYPES, which should be prevented during list creation
- Registry clearing could cause issues if init() is called multiple times
- Comments could be more descriptive
Consider this refactored implementation:
public static void init() { + // Prevent multiple initialization + if (!GENERIC_REGISTRY.isEmpty() || !PLAYER_REGISTRY.isEmpty()) { + LOGGER.warn("Attribute registration has already been initialized"); + return; + } + LifecycleEvent.SETUP.register(() -> { - entityTypes.forEach(entityType -> { - if (entityType == null) return; + ENTITY_TYPES.forEach(entityType -> { AttributeSupplier.Builder builder = new AttributeSupplier.Builder(); - // Apply existing attributes + // Copy vanilla attributes to preserve default behavior if (DefaultAttributes.hasSupplier(entityType)) { DefaultAttributes.getSupplier(entityType).instances.forEach((attribute, attributeInstance) -> { builder.add(attribute, attributeInstance.getBaseValue()); }); } + // Apply player-specific attributes if this is a player entity if (entityType.equals(EntityType.PLAYER)) PLAYER_REGISTRY.forEach(builder::add); - // Apply global custom attributes + // Apply global attributes to all entity types GENERIC_REGISTRY.forEach(builder::add); - // Register the attributes + // Register the combined attribute set with Fabric FabricDefaultAttributeRegistry.register(entityType, builder); }); - // Clear the registry + // Clear registries to prevent duplicate registrations and free memory PLAYER_REGISTRY.clear(); GENERIC_REGISTRY.clear(); }); }Let's verify that init() is only called once:
attribute-common/src/main/resources/manascore_attribute.accesswidener (1)
1-2: LGTM! Verify access widener configuration.The access widener configuration is correctly structured and follows Minecraft modding best practices. The field access modification is appropriate for attribute registration purposes.
Run this script to verify the access widener configuration is properly recognized:
✅ Verification successful
Access widener configuration verified and correctly integrated
The access widener is properly configured in the build system and follows Fabric modding best practices. The configuration is clean, minimal, and correctly referenced in the build files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify access widener configuration is properly referenced in build files # Check if access widener path is configured in build.gradle files rg -l "accessWidenerPath.*manascore_attribute.accesswidener" --type gradleLength of output: 106
Script:
#!/bin/bash # Check the actual access widener configuration in build.gradle cat attribute-common/build.gradleLength of output: 202
attribute-common/src/main/java/io/github/manasmods/manascore/attribute/api/ManasCoreAttributes.java (1)
23-25: Review the default value and maximum limit for critical damage multiplier.The critical damage multiplier has a maximum value of 1024x which seems excessive and could lead to balance issues. Consider reducing this to a more reasonable value (e.g., 5x or 10x) to prevent potential exploits.
attribute-neoforge/src/main/java/io/github/manasmods/manascore/attribute/neoforge/ManasCoreAttributeRegisterImpl.java (1)
19-20: LGTM! Thread-safe implementation using CopyOnWriteArrayList.Good choice using CopyOnWriteArrayList for thread-safe concurrent modifications during registration.
testing-common/src/main/java/io/github/manasmods/manascore/testing/registry/RegistryTest.java (3)
193-195: LGTM: Block entity creation is properly implemented.The block entity creation is correctly implemented using the registered block entity type.
161-176: Consider adding safety checks for explosion radius.The explosion radius (
g) is set to a random value between 10.0F and 12.0F, which is quite large and could cause significant damage. Consider adding safety checks or configuration options.
62-67: Review attribute modifier values for balance.The attribute modifiers for critical damage and chance seem extremely high:
- Critical damage multiplier: 10x (1000% increase)
- Critical attack chance: 50% (50% increase)
These values might be too powerful for gameplay balance, even in a testing environment.
attribute-neoforge/build.gradle (1)
6-8: LGTM: Access widener configuration is correct.The access widener path is properly inherited from the common module.
attribute-fabric/src/main/resources/fabric.mod.json (1)
27-31: Verify Java version compatibility.The mod requires Java 21, which is a very recent version. This might limit compatibility with some mod loaders and other mods.
attribute-neoforge/src/main/resources/META-INF/neoforge.mods.toml (3)
1-4: Add issue tracker URL and verify license placeholder.
- The empty issue tracker URL should be populated to help users report issues.
- Ensure that the
${license}placeholder is correctly replaced during the build process.Run this script to verify the license placeholder replacement:
✅ Verification successful
Add issue tracker URL - license placeholder is correctly configured
The
${license}placeholder is properly configured inbuild.gradleand will be replaced with "GPLv3" during the build process. However, the issue tracker URL is still empty and should be set to "https://github.com/ManasMods/ManasCore" to match the project's repository.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify license placeholder replacement in build process # Check build configuration files for license variable definition echo "Checking for license variable in build files..." fd -e gradle -e properties | xargs grep -l "license" # Check if there are any remaining unresolved placeholders in built artifacts echo "Checking for unresolved placeholders in built artifacts..." fd -e toml | xargs grep -l "\${license}"Length of output: 916
Script:
#!/bin/bash # Check build.gradle for license configuration echo "=== build.gradle license configuration ===" rg "license" build.gradle -A 3 -B 3 # Check if this is a common pattern in other similar projects echo -e "\n=== Checking root project structure ===" ls -laLength of output: 3582
37-38: Verify mixin configuration file existence.Ensure that the mixin configuration file exists and is properly named according to the mod ID.
Run this script to verify the mixin configuration:
✅ Verification successful
Mixin configuration is correctly set up ✓
The mixin configuration references
manascore_attribute.mixins.jsonwhich exists in the common module atattribute-common/src/main/resources/manascore_attribute.mixins.json. This is a valid setup as NeoForge can use the mixin configuration from the common module.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify mixin configuration file # First, find the mod_id value from build files echo "Checking for mod_id in build files..." fd -e gradle -e properties | xargs grep "mod_id" # Then, check for the corresponding mixins.json file echo "Checking for mixins.json file..." fd "mixins.json" -t fLength of output: 1864
Script:
#!/bin/bash # Get archives_name from gradle.properties and construct expected mod_id echo "Getting archives_name from gradle.properties..." grep "archives_name" gradle.properties # Check for any mixin config file specifically for attribute-neoforge echo -e "\nChecking for attribute-neoforge specific mixin configs..." fd . attribute-neoforge/src/main/resources -t fLength of output: 461
6-14: Verify placeholders and logo file existence.
- Ensure that all placeholders (
${mod_id},${version},${mod_name}) are properly configured in the build system.- Verify that
icon.pngexists in the correct resources directory.Run this script to verify the configuration:
✅ Verification successful
✓ Placeholders and icon file configuration verified
All placeholders are properly configured in the build system and the icon file exists at the correct location. The build process will correctly populate:
${version}from version 4.0.0.0${mod_id}as "manascore_attribute"${mod_name}as "ManasCore - Attribute"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify mod metadata configuration # Check build configuration files for variable definitions echo "Checking for mod variables in build files..." fd -e gradle -e properties | xargs grep -l "mod_id\|version\|mod_name" # Verify icon.png existence echo "Checking for icon.png..." fd "icon.png" -t fLength of output: 1465
Script:
#!/bin/bash # Check variable definitions in main configuration files echo "=== Checking gradle.properties ===" cat gradle.properties echo -e "\n=== Checking root build.gradle ===" cat build.gradle echo -e "\n=== Checking attribute-neoforge/build.gradle ===" cat attribute-neoforge/build.gradleLength of output: 11202
Attempt at diagnosing Invalid Player Data crash causes
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
race-common/src/main/java/io/github/manasmods/manascore/race/impl/RaceStorage.java (3)
46-55: Remove dead commented-out code.The old static-listener approach has been superseded by the inline anonymous class in
init(). Leaving it causes confusion about which pattern is authoritative.♻️ Proposed fix
- /* - private static final StorageEvents.RegisterStorage<Entity> listener = new StorageEvents.RegisterStorage<Entity>() { - `@Override` - public void register(StorageEvents.StorageRegistry<Entity> registry) { - ManasCoreRace.LOG.info("ManasRace storage event triggered"); - key = registry.register(ResourceLocation.fromNamespaceAndPath(ModuleConstants.MOD_ID, "race_storage"), - RaceStorage.class, LivingEntity.class::isInstance, target -> new RaceStorage((LivingEntity) target)); - ManasCoreRace.LOG.info(key != null ? "storage Key registered " + key.toString() : "storage Key failed to register"); - } - };*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@race-common/src/main/java/io/github/manasmods/manascore/race/impl/RaceStorage.java` around lines 46 - 55, Remove the dead commented-out static listener block (the /* ... */ containing StorageEvents.RegisterStorage<Entity> listener and its register override) from RaceStorage; this duplicates the current inline anonymous listener used in init(), so delete that entire commented block (including references to key, register, ResourceLocation.fromNamespaceAndPath, and ManasCoreRace.LOG) to avoid confusion and keep only the active init() registration logic.
44-44:tickingRacesshould not bepublic.Exposing a mutable
Multimapas a public field allows any external code to add or remove entries, bypassing the lifecycle cleanup registered ininit()(PLAYER_QUIT, CHANGE_DIMENSION handlers). Restrict visibility to package-private or private with a controlled accessor.♻️ Proposed fix
- public static final Multimap<UUID, TickingRace> tickingRaces = ArrayListMultimap.create(); + static final Multimap<UUID, TickingRace> tickingRaces = ArrayListMultimap.create();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@race-common/src/main/java/io/github/manasmods/manascore/race/impl/RaceStorage.java` at line 44, The public mutable Multimap tickingRaces exposes internal state; change its visibility to private (or package-private) and provide controlled accessors instead: keep the field named tickingRaces but make it private, add a read-only accessor (e.g., getTickingRaces() returning an unmodifiable or immutable view) and/or explicit manipulation methods (e.g., addTickingRace(UUID, TickingRace), removeTickingRace(UUID, TickingRace)) so external code cannot bypass lifecycle cleanup in init() and the PLAYER_QUIT / CHANGE_DIMENSION handlers; update all call sites to use the new accessor/mutators.
58-69: Use the class's own@Log4j2-injectedloginstead ofManasCoreRace.LOG.
RaceStoragealready declares@Log4j2, so Lombok injects aprivate static final Logger logfield. UsingManasCoreRace.LOGinstead couples the two classes and muddies log attribution (all messages appear under"ManasCore - Race"rather thanRaceStorage).♻️ Proposed fix
- ManasCoreRace.LOG.info("event registration"); + log.info("event registration"); StorageEvents.RegisterStorage<Entity> listener = new StorageEvents.RegisterStorage<Entity>() { `@Override` public void register(StorageEvents.StorageRegistry<Entity> registry) { - ManasCoreRace.LOG.info("storage event triggered"); + log.info("storage event triggered"); key = registry.register(ResourceLocation.fromNamespaceAndPath(ModuleConstants.MOD_ID, "race_storage"), RaceStorage.class, LivingEntity.class::isInstance, target -> new RaceStorage((LivingEntity) target)); - ManasCoreRace.LOG.info(key != null ? "storage Key registered " + key.toString() : "storage Key failed to register"); + log.info(key != null ? "storage Key registered " + key.toString() : "storage Key failed to register"); } }; StorageEvents.REGISTER_ENTITY_STORAGE.register(listener); - ManasCoreRace.LOG.info("storage event registered? {}", String.valueOf(StorageEvents.REGISTER_ENTITY_STORAGE.isRegistered(listener))); + log.info("storage event registered? {}", String.valueOf(StorageEvents.REGISTER_ENTITY_STORAGE.isRegistered(listener)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@race-common/src/main/java/io/github/manasmods/manascore/race/impl/RaceStorage.java` around lines 58 - 69, Replace usages of ManasCoreRace.LOG inside RaceStorage's storage registration block with the class's Lombok-injected logger (log). In the StorageEvents.RegisterStorage<Entity> listener and its register(StorageEvents.StorageRegistry<Entity> registry) method, change all logging calls that reference ManasCoreRace.LOG (the info calls around "event registration", "storage event triggered", the key registration success/failure, and the final isRegistered check) to use log.info(...) so the messages are attributed to RaceStorage; keep the same messages and variables (key, registry, target) and the existing listener registration (StorageEvents.REGISTER_ENTITY_STORAGE.register(listener)).race-common/src/main/java/io/github/manasmods/manascore/race/ManasCoreRace.java (1)
19-19: Improve the initialization log message.
LOG.info("init")is a no-op for debugging — it doesn't identify which module is starting or when it finishes.♻️ Proposed fix
- LOG.info("init"); + LOG.info("Initializing ManasCore Race module");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@race-common/src/main/java/io/github/manasmods/manascore/race/ManasCoreRace.java` at line 19, Replace the vague LOG.info("init") call in the ManasCoreRace class with a more descriptive initialization log that identifies the module and lifecycle event (e.g., "ManasCoreRace initializing" and optionally a corresponding completion message like "ManasCoreRace initialized"), so update the LOG.info invocation(s) in the ManasCoreRace class to include the class/module name and event to aid debugging and tracing.skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java (5)
48-56: Remove commented-out listener field.The static listener block has been superseded by the inline registration in
init()and serves no purpose now.🧹 Proposed cleanup
- /* - private static final StorageEvents.RegisterStorage<Entity> listener = new StorageEvents.RegisterStorage<Entity>() { - `@Override` - public void register(StorageEvents.StorageRegistry<Entity> registry) { - ManasCoreSkill.LOG.info("ManasSkill storage event triggered"); - key = registry.register(ResourceLocation.fromNamespaceAndPath(ModuleConstants.MOD_ID, "skill_storage"), SkillStorage.class, LivingEntity.class::isInstance, target -> new SkillStorage((LivingEntity) target)); - ManasCoreSkill.LOG.info(key != null ? "ManasSkill storage Key registered " + key.toString() : "ManasSkill storage Key failed to register"); - } - };*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java` around lines 48 - 56, Remove the dead/commented-out static listener block: delete the commented StorageEvents.RegisterStorage<Entity> listener field (the entire /* ... */ block that defines listener, its register(...) method, the ManasCoreSkill.LOG.info calls, and the key registration logic) since registration is now handled inline in init(); keep SkillStorage and any referenced fields used by init() intact.
321-331: Unnecessary local alias inremoveTickingSkill.Line 322 assigns
tickingSkillsto a local variablemultimapthat is used for the entirety of the method — purely an alias with no benefit. Reference the static field directly.♻️ Proposed cleanup
public static void removeTickingSkill(Player player) { - Multimap<UUID, TickingSkill> multimap = tickingSkills; - if (multimap.containsKey(player.getUUID())) { - for (TickingSkill skill : multimap.get(player.getUUID())) { + if (tickingSkills.containsKey(player.getUUID())) { + for (TickingSkill skill : tickingSkills.get(player.getUUID())) { Optional<ManasSkillInstance> instance = SkillAPI.getSkillsFrom(player).getSkill(skill.getSkill()); if (instance.isEmpty()) continue; skill.getSkill().removeAttributeModifiers(instance.get(), player, skill.getMode()); } - multimap.removeAll(player.getUUID()); + tickingSkills.removeAll(player.getUUID()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java` around lines 321 - 331, The method removeTickingSkill creates an unnecessary local alias "multimap" for the static field tickingSkills; remove that local variable and reference tickingSkills directly throughout the method (in the containsKey, get iteration, and removeAll calls) so the code uses the static Multimap<UUID, TickingSkill> tickingSkills in place of the local Multimap variable while keeping the existing logic that iterates TickingSkill, checks SkillAPI.getSkillsFrom(player).getSkill(...), and calls removeAttributeModifiers on the found ManasSkillInstance.
247-250:forEachSkillunconditionally marks storage dirty.
markDirty()is called on line 249 regardless of whether theskillInstanceConsumeractually modified anything. This will trigger a sync cycle even for read-only consumers.Consider only marking dirty if any instance
isDirty()after the iteration, or document that callers are responsible for dirtying instances explicitly and remove the blanket call.♻️ Proposed fix
public void forEachSkill(BiConsumer<SkillStorage, ManasSkillInstance> skillInstanceConsumer) { List.copyOf(this.skillInstances.values()).forEach(skillInstance -> skillInstanceConsumer.accept(this, skillInstance)); - markDirty(); + if (this.skillInstances.values().stream().anyMatch(ManasSkillInstance::isDirty)) markDirty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java` around lines 247 - 250, The forEachSkill method in SkillStorage currently always calls markDirty() after running the BiConsumer, causing unnecessary syncs; modify forEachSkill to only call markDirty() if any ManasSkillInstance became dirty by checking instance.isDirty() after the consumer runs (e.g., track a local boolean dirtyFlag set when any instance.isDirty() returns true) or alternatively remove the unconditional markDirty() and update documentation to require callers to mark dirty explicitly; reference the forEachSkill method in SkillStorage, the markDirty() call, and ManasSkillInstance.isDirty() when making the change.
72-99: Entity event routing does not belong inSkillStorage.init().Lines 72–99 wire up three cross-cutting event chains (
LIVING_CHANGE_TARGETearly/late split,EntityEvent.LIVING_HURT→LIVING_PRE_DAMAGED/LIVING_ON_BEING_DAMAGED,LIVING_DEATHpriority chain) that have nothing specific to do with storage. Placing them here means:
- They are only active once
SkillStorage.init()is called; if that call order changes, the event plumbing silently disappears.- Readers looking for entity lifecycle behaviour must hunt inside a storage class.
Consider moving this routing to
SkillRegistry.init()or a dedicatedSkillEntityEventRouterinitialised fromManasCoreSkill.init(), consistent with howRaceStoragedelegates non-storage setup toRaceRegistry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java` around lines 72 - 99, The event-routing code inside SkillStorage.init() (wiring EntityEvents.LIVING_CHANGE_TARGET, EntityEvent.LIVING_HURT handlers and EntityEvent.LIVING_DEATH priority chain) is misplaced; move those registrations out of SkillStorage into a focused initializer so they are always registered regardless of storage init order. Create either a new SkillEntityEventRouter class (or add to SkillRegistry.init()) and relocate the three blocks (the LIVING_CHANGE_TARGET early/late invoker chain, the LIVING_HURT → LIVING_PRE_DAMAGED/LIVING_ON_BEING_DAMAGED and the SKILL_DAMAGE_* calculation chain, and the LIVING_DEATH priority chain) into that router, then invoke the router from ManasCoreSkill.init() (or from SkillRegistry.init()) during module startup; leave SkillStorage.init() responsible only for persistence-related setup, mirroring RaceStorage/RaceRegistry separation.
127-145: Rename local variable to avoid shadowing static field; remove redundant lookup.The
tickSkillsmethod has two issues:
Variable shadowing — Line 128 declares
List<ManasSkillInstance> tickingSkills, which shadows the staticMultimap<UUID, TickingSkill> tickingSkillsfield at line 48. While technically correct, the identical name makes the code confusing. Rename the local variable (e.g.,skillsToTick).Redundant lookup — Lines 130–131 call
storage.getSkill(instance.getSkill())to re-fetch an instance that already exists in the iteration. SincegetLearnedSkills()returns the liveskillInstances.values(), the re-fetchedskillInstanceis the same object asinstance. Remove the redundant lookup and useinstancedirectly.♻️ Proposed cleanup
- private static void tickSkills(LivingEntity entity, Skills storage) { - List<ManasSkillInstance> tickingSkills = new ArrayList<>(); - for (ManasSkillInstance instance : storage.getLearnedSkills()) { - Optional<ManasSkillInstance> optional = storage.getSkill(instance.getSkill()); - if (optional.isEmpty()) continue; - - ManasSkillInstance skillInstance = optional.get(); - if (!skillInstance.canInteractSkill(entity)) continue; - if (!skillInstance.canTick(entity)) continue; - if (SkillEvents.SKILL_PRE_TICK.invoker().tick(skillInstance, entity).isFalse()) continue; - tickingSkills.add(skillInstance); + private static void tickSkills(LivingEntity entity, Skills storage) { + List<ManasSkillInstance> skillsToTick = new ArrayList<>(); + for (ManasSkillInstance instance : storage.getLearnedSkills()) { + if (!instance.canInteractSkill(entity)) continue; + if (!instance.canTick(entity)) continue; + if (SkillEvents.SKILL_PRE_TICK.invoker().tick(instance, entity).isFalse()) continue; + skillsToTick.add(instance); } - for (ManasSkillInstance instance : tickingSkills) { + for (ManasSkillInstance instance : skillsToTick) { instance.onTick(entity); SkillEvents.SKILL_POST_TICK.invoker().tick(instance, entity); storage.checkAndMarkDirty(instance); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java` around lines 127 - 145, In SkillStorage.tickSkills: avoid shadowing the static Multimap tickingSkills and remove the redundant storage.getSkill(...) lookup — rename the local List tickingSkills to skillsToTick (or similar) and iterate using the existing loop variable instance directly (call instance.canInteractSkill(...), instance.canTick(...), invoke SkillEvents.SKILL_PRE_TICK/POST_TICK and instance.onTick(...)) instead of re-fetching storage.getSkill(instance.getSkill()); keep the final loop to call instance.onTick(entity) and storage.checkAndMarkDirty(instance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@storage-common/src/main/java/io/github/manasmods/manascore/storage/impl/StorageManager.java`:
- Around line 113-126: The current constructStorageFor method swallows all
Exceptions and logs a misleading message; instead, first null-check each
registry lookup (ENTITY_STORAGE_REGISTRY.registry.get(id),
CHUNK_STORAGE_REGISTRY.registry.get(id),
LEVEL_STORAGE_REGISTRY.registry.get(id)) and if null log
ManasCoreStorage.LOG.warn("No storage registered for id {}", id) and return
null, then perform the cast and call to create(...) in a narrow try/catch that
catches specific failures (ClassCastException and RuntimeException) and log them
with the throwable parameter (e.g., ManasCoreStorage.LOG.warn("Failed to
construct storage for {}: {}", id, e.getMessage(), e)) so stack traces aren’t
lost and messages aren’t misleading.
- Around line 58-62: The PLAYER_CLONE event handler can NPE when
oldPlayer.manasCore$getCombinedStorage() is null; guard that call before
invoking toNBT(). Update the lambda registered on PlayerEvent.PLAYER_CLONE to
check oldPlayer.manasCore$getCombinedStorage() for null and, if non-null, call
toNBT() and load into the new CombinedStorage; if it is null, either initialize
newStorage empty and set it on newPlayer via
newPlayer.manasCore$setCombinedStorage(newStorage) or skip loading so no NPE
occurs. Ensure you reference the existing CombinedStorage constructor, the
manasCore$getCombinedStorage() accessor, toNBT(), and
newPlayer.manasCore$setCombinedStorage(...) when applying the fix.
In `@testing-fabric/build.gradle`:
- Around line 30-31: Replace the incorrect mod dependency declarations that use
modImplementation for Night Config with the bundling pattern used in the
production module: stop using
modImplementation("com.electronwill.night-config:toml:3.8.1") and
modImplementation("com.electronwill.night-config:core:3.8.1") and instead
declare them as implementation dependencies and wrap them with
include(implementation(...)) so they are packaged (mirror the approach from
config-fabric's build.gradle); also bump the version from 3.8.1 to 3.8.3 for
both toml and core coordinates.
---
Duplicate comments:
In
`@race-common/src/main/java/io/github/manasmods/manascore/race/impl/RaceStorage.java`:
- Line 151: The null-pointer risk comes from auto-unboxing teleport.get() in
RaceStorage; update the conditional to use a null-safe check (e.g., replace the
teleport.get() test with Boolean.TRUE.equals(teleport.get())) so the code
becomes null-safe when a Changeable listener sets the value to null, keeping the
rest of the condition (getOwner() instanceof ServerPlayer player &&
SpawnPointHelper.teleportToNewSpawn(player)) unchanged.
- Line 111: Wrap the call to instance.onTick(entity) in a try/catch so a thrown
exception from a Race implementation doesn't crash the server tick loop;
specifically, catch Throwable around the invocation in RaceStorage (where
instance.onTick(entity) is called), log the error with context (e.g.,
instance.getClass().getName() and the entity or race id) using the existing
logger, and ensure the loop continues processing other entities after catching
the exception.
- Around line 91-95: The method handleRaceHeldTick currently unconditionally
calls storage.markDirty(), causing unnecessary syncs; change it to only call
markDirty when the tickingRaces entry actually changed by capturing the boolean
result of tickingRaces.get(player.getUUID()).removeIf(...) (removeIf returns
true if any element was removed) and invoke storage.markDirty() only when that
boolean is true; reference tickingRaces, handleRaceHeldTick, removeIf, and
storage.markDirty to locate and update the logic.
- Around line 142-144: Guard the sendSystemMessage call so it only runs when
getOwner() is a Player (e.g., if (getOwner() instanceof Player)
((Player)getOwner()).sendSystemMessage(raceMessage.get())); and make applying
attribute modifiers conditional: if the current instance (variable instance) is
different from the new race, first remove the old modifiers from the owner
(e.g., if (instance != null && instance != race)
instance.removeAttributeModifiers(owner)), then call
race.addAttributeModifiers(owner); do NOT call addAttributeModifiers
unconditionally to avoid stacking when re-applying the same race.
- Around line 162-166: The load(CompoundTag) should guard against
deserialization failures and restore live attribute modifiers: wrap the
ManasRaceInstance.fromNBT call in a try/catch, log the error and clear
this.raceInstance (or leave null) on failure instead of letting exceptions
propagate; on successful load, reapply the race's attribute modifiers to the
live entity by invoking the same codepath used when a race is initially assigned
(e.g., call the existing method that applies a ManasRaceInstance to an entity —
reuse setRace/assignRace/applyRaceAttributes or whatever helper is used
elsewhere) so attributes are added back after chunk reload. Ensure you reference
RACE_KEY, load(CompoundTag), this.raceInstance and ManasRaceInstance.fromNBT
when making the change.
In
`@skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java`:
- Around line 252-272: In handleSkillRelease, after invoking
SkillEvents.RELEASE_SKILL and obtaining the possibly-modified instance via
Changeable.of(...) into variable skill, replace both calls to
this.checkAndMarkDirty(skillInstance) with this.checkAndMarkDirty(skill) so the
post-event, potentially swapped ManasSkillInstance is persisted; keep the
existing null check for skill and ensure the two locations (after
skill.onRelease(...) and at the end of the method) reference the local skill
variable rather than the original skillInstance parameter.
- Around line 230-244: The current forgetSkill method removes the instance via
this.getLearnedSkills().remove(instance), which only removes from the Collection
view and won't modify the underlying skillInstances map; instead call
this.skillInstances.remove(skillId) to remove the entry by key (keep the
existing instance.onForgetSkill(...), instance.markDirty(),
this.hasRemovedSkills = true and markDirty() behavior intact) so the skill is
actually removed from the backing map; locate the forgetSkill method and replace
the collection-view removal with skillInstances.remove(skillId).
- Line 46: The public static Multimap tickingSkills in SkillStorage is created
with ArrayListMultimap.create() which is not thread-safe; wrap or replace it
with a thread-safe variant so concurrent access is safe. Change the declaration
of tickingSkills (the Multimap<UUID, TickingSkill> field in SkillStorage) to use
a synchronized or concurrent implementation — e.g., use
com.google.common.collect.Multimaps.synchronizedMultimap(ArrayListMultimap.create())
or build a ConcurrentHashMap-backed structure (ConcurrentHashMap<UUID,
Collection<TickingSkill>> or a ConcurrentHashMultimap alternative) and update
any code that mutates or iterates tickingSkills accordingly. Ensure the field
type remains Multimap<UUID, TickingSkill> (or switch to the chosen concurrent
type) and keep usage of TickingSkill and UUID consistent.
---
Nitpick comments:
In
`@race-common/src/main/java/io/github/manasmods/manascore/race/impl/RaceStorage.java`:
- Around line 46-55: Remove the dead commented-out static listener block (the /*
... */ containing StorageEvents.RegisterStorage<Entity> listener and its
register override) from RaceStorage; this duplicates the current inline
anonymous listener used in init(), so delete that entire commented block
(including references to key, register, ResourceLocation.fromNamespaceAndPath,
and ManasCoreRace.LOG) to avoid confusion and keep only the active init()
registration logic.
- Line 44: The public mutable Multimap tickingRaces exposes internal state;
change its visibility to private (or package-private) and provide controlled
accessors instead: keep the field named tickingRaces but make it private, add a
read-only accessor (e.g., getTickingRaces() returning an unmodifiable or
immutable view) and/or explicit manipulation methods (e.g., addTickingRace(UUID,
TickingRace), removeTickingRace(UUID, TickingRace)) so external code cannot
bypass lifecycle cleanup in init() and the PLAYER_QUIT / CHANGE_DIMENSION
handlers; update all call sites to use the new accessor/mutators.
- Around line 58-69: Replace usages of ManasCoreRace.LOG inside RaceStorage's
storage registration block with the class's Lombok-injected logger (log). In the
StorageEvents.RegisterStorage<Entity> listener and its
register(StorageEvents.StorageRegistry<Entity> registry) method, change all
logging calls that reference ManasCoreRace.LOG (the info calls around "event
registration", "storage event triggered", the key registration success/failure,
and the final isRegistered check) to use log.info(...) so the messages are
attributed to RaceStorage; keep the same messages and variables (key, registry,
target) and the existing listener registration
(StorageEvents.REGISTER_ENTITY_STORAGE.register(listener)).
In
`@race-common/src/main/java/io/github/manasmods/manascore/race/ManasCoreRace.java`:
- Line 19: Replace the vague LOG.info("init") call in the ManasCoreRace class
with a more descriptive initialization log that identifies the module and
lifecycle event (e.g., "ManasCoreRace initializing" and optionally a
corresponding completion message like "ManasCoreRace initialized"), so update
the LOG.info invocation(s) in the ManasCoreRace class to include the
class/module name and event to aid debugging and tracing.
In
`@skill-common/src/main/java/io/github/manasmods/manascore/skill/impl/SkillStorage.java`:
- Around line 48-56: Remove the dead/commented-out static listener block: delete
the commented StorageEvents.RegisterStorage<Entity> listener field (the entire
/* ... */ block that defines listener, its register(...) method, the
ManasCoreSkill.LOG.info calls, and the key registration logic) since
registration is now handled inline in init(); keep SkillStorage and any
referenced fields used by init() intact.
- Around line 321-331: The method removeTickingSkill creates an unnecessary
local alias "multimap" for the static field tickingSkills; remove that local
variable and reference tickingSkills directly throughout the method (in the
containsKey, get iteration, and removeAll calls) so the code uses the static
Multimap<UUID, TickingSkill> tickingSkills in place of the local Multimap
variable while keeping the existing logic that iterates TickingSkill, checks
SkillAPI.getSkillsFrom(player).getSkill(...), and calls removeAttributeModifiers
on the found ManasSkillInstance.
- Around line 247-250: The forEachSkill method in SkillStorage currently always
calls markDirty() after running the BiConsumer, causing unnecessary syncs;
modify forEachSkill to only call markDirty() if any ManasSkillInstance became
dirty by checking instance.isDirty() after the consumer runs (e.g., track a
local boolean dirtyFlag set when any instance.isDirty() returns true) or
alternatively remove the unconditional markDirty() and update documentation to
require callers to mark dirty explicitly; reference the forEachSkill method in
SkillStorage, the markDirty() call, and ManasSkillInstance.isDirty() when making
the change.
- Around line 72-99: The event-routing code inside SkillStorage.init() (wiring
EntityEvents.LIVING_CHANGE_TARGET, EntityEvent.LIVING_HURT handlers and
EntityEvent.LIVING_DEATH priority chain) is misplaced; move those registrations
out of SkillStorage into a focused initializer so they are always registered
regardless of storage init order. Create either a new SkillEntityEventRouter
class (or add to SkillRegistry.init()) and relocate the three blocks (the
LIVING_CHANGE_TARGET early/late invoker chain, the LIVING_HURT →
LIVING_PRE_DAMAGED/LIVING_ON_BEING_DAMAGED and the SKILL_DAMAGE_* calculation
chain, and the LIVING_DEATH priority chain) into that router, then invoke the
router from ManasCoreSkill.init() (or from SkillRegistry.init()) during module
startup; leave SkillStorage.init() responsible only for persistence-related
setup, mirroring RaceStorage/RaceRegistry separation.
- Around line 127-145: In SkillStorage.tickSkills: avoid shadowing the static
Multimap tickingSkills and remove the redundant storage.getSkill(...) lookup —
rename the local List tickingSkills to skillsToTick (or similar) and iterate
using the existing loop variable instance directly (call
instance.canInteractSkill(...), instance.canTick(...), invoke
SkillEvents.SKILL_PRE_TICK/POST_TICK and instance.onTick(...)) instead of
re-fetching storage.getSkill(instance.getSkill()); keep the final loop to call
instance.onTick(entity) and storage.checkAndMarkDirty(instance).
| PlayerEvent.PLAYER_CLONE.register((oldPlayer, newPlayer, wonGame) -> { | ||
| CombinedStorage newStorage = new CombinedStorage(newPlayer); | ||
| newStorage.load(oldPlayer.manasCore$getCombinedStorage().toNBT()); | ||
| newPlayer.manasCore$setCombinedStorage(newStorage); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages and implementations of manasCore$getCombinedStorage
rg -n --type java 'manasCore\$getCombinedStorage' -B 2 -A 5Repository: ManasMods/ManasCore
Length of output: 15198
🏁 Script executed:
#!/bin/bash
# Check MixinEntity storage field initialization
rg -n --type java -A 10 'class MixinEntity' storage-common/src/main/java/io/github/manasmods/manascore/storage/mixin/MixinEntity.java
# Check StorageManager.initialStorageFilling() to see when storage is initialized
rg -n --type java -A 20 'public static void initialStorageFilling' storage-common/src/main/java/io/github/manasmods/manascore/storage/impl/StorageManager.javaRepository: ManasMods/ManasCore
Length of output: 1335
🏁 Script executed:
#!/bin/bash
# Find ENTITY_STORAGE_REGISTRY definition and attach method
rg -n --type java 'ENTITY_STORAGE_REGISTRY' -B 2 -A 10 storage-common/src/main/java/
# Check for calls to initialStorageFilling
rg -n --type java 'initialStorageFilling' -B 2 -A 2Repository: ManasMods/ManasCore
Length of output: 9645
Add null guard to prevent NPE in PLAYER_CLONE event handler.
The @NotNull annotation on manasCore$getCombinedStorage() is not runtime-enforced. If the old player's storage is null (whether due to initialization order, edge cases, or mod interference), calling .toNBT() throws NullPointerException inside the event handler, crashing the server on every respawn. The codebase already uses defensive null checks for similar cases (MixinLevelChunk, line 80-83).
Proposed fix
PlayerEvent.PLAYER_CLONE.register((oldPlayer, newPlayer, wonGame) -> {
+ var oldStorage = oldPlayer.manasCore$getCombinedStorage();
+ if (oldStorage == null) return;
CombinedStorage newStorage = new CombinedStorage(newPlayer);
- newStorage.load(oldPlayer.manasCore$getCombinedStorage().toNBT());
+ newStorage.load(oldStorage.toNBT());
newPlayer.manasCore$setCombinedStorage(newStorage);
});📝 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.
| PlayerEvent.PLAYER_CLONE.register((oldPlayer, newPlayer, wonGame) -> { | |
| CombinedStorage newStorage = new CombinedStorage(newPlayer); | |
| newStorage.load(oldPlayer.manasCore$getCombinedStorage().toNBT()); | |
| newPlayer.manasCore$setCombinedStorage(newStorage); | |
| }); | |
| PlayerEvent.PLAYER_CLONE.register((oldPlayer, newPlayer, wonGame) -> { | |
| var oldStorage = oldPlayer.manasCore$getCombinedStorage(); | |
| if (oldStorage == null) return; | |
| CombinedStorage newStorage = new CombinedStorage(newPlayer); | |
| newStorage.load(oldStorage.toNBT()); | |
| newPlayer.manasCore$setCombinedStorage(newStorage); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@storage-common/src/main/java/io/github/manasmods/manascore/storage/impl/StorageManager.java`
around lines 58 - 62, The PLAYER_CLONE event handler can NPE when
oldPlayer.manasCore$getCombinedStorage() is null; guard that call before
invoking toNBT(). Update the lambda registered on PlayerEvent.PLAYER_CLONE to
check oldPlayer.manasCore$getCombinedStorage() for null and, if non-null, call
toNBT() and load into the new CombinedStorage; if it is null, either initialize
newStorage empty and set it on newPlayer via
newPlayer.manasCore$setCombinedStorage(newStorage) or skip loading so no NPE
occurs. Ensure you reference the existing CombinedStorage constructor, the
manasCore$getCombinedStorage() accessor, toNBT(), and
newPlayer.manasCore$setCombinedStorage(...) when applying the fix.
| @Nullable | ||
| public static Storage constructStorageFor(StorageType type, ResourceLocation id, StorageHolder holder) { | ||
| return switch (type) { | ||
| case ENTITY -> ENTITY_STORAGE_REGISTRY.registry.get(id).getSecond().create((Entity) holder); | ||
| case CHUNK -> CHUNK_STORAGE_REGISTRY.registry.get(id).getSecond().create((LevelChunk) holder); | ||
| case WORLD -> LEVEL_STORAGE_REGISTRY.registry.get(id).getSecond().create((Level) holder); | ||
| }; | ||
| try { | ||
| return switch (type) { | ||
| case ENTITY -> ENTITY_STORAGE_REGISTRY.registry.get(id).getSecond().create((Entity) holder); | ||
| case CHUNK -> CHUNK_STORAGE_REGISTRY.registry.get(id).getSecond().create((LevelChunk) holder); | ||
| case WORLD -> LEVEL_STORAGE_REGISTRY.registry.get(id).getSecond().create((Level) holder); | ||
| }; | ||
| } | ||
| catch (Exception e) { | ||
| ManasCoreStorage.LOG.warn("Failed to find storage in registry {}", id.toString()); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Overly broad catch(Exception) silently discards the stack trace and misattributes all failures.
Three distinct problems here:
-
Exception is swallowed: The caught
eis never passed to the logger. Providing only the ID string makes it impossible to distinguish an unregistered key (NPE fromregistry.get(id)returningnull), a type mismatch (ClassCastException), or a factory-side error. -
Misleading message:
"Failed to find storage in registry"implies a missing registration, but the same message fires forClassCastExceptionor any exception thrown inside a factory. -
id.toString()is redundant: SLF4J's{}placeholder callstoString()automatically.
🔧 Proposed fix: explicit null-check + pass exception to logger
`@Nullable`
public static Storage constructStorageFor(StorageType type, ResourceLocation id, StorageHolder holder) {
try {
- return switch (type) {
- case ENTITY -> ENTITY_STORAGE_REGISTRY.registry.get(id).getSecond().create((Entity) holder);
- case CHUNK -> CHUNK_STORAGE_REGISTRY.registry.get(id).getSecond().create((LevelChunk) holder);
- case WORLD -> LEVEL_STORAGE_REGISTRY.registry.get(id).getSecond().create((Level) holder);
- };
+ var entry = switch (type) {
+ case ENTITY -> ENTITY_STORAGE_REGISTRY.registry.get(id);
+ case CHUNK -> CHUNK_STORAGE_REGISTRY.registry.get(id);
+ case WORLD -> LEVEL_STORAGE_REGISTRY.registry.get(id);
+ };
+ if (entry == null) {
+ ManasCoreStorage.LOG.warn("No storage registered for id {}", id);
+ return null;
+ }
+ return switch (type) {
+ case ENTITY -> entry.getSecond().create((Entity) holder);
+ case CHUNK -> entry.getSecond().create((LevelChunk) holder);
+ case WORLD -> entry.getSecond().create((Level) holder);
+ };
}
catch (Exception e) {
- ManasCoreStorage.LOG.warn("Failed to find storage in registry {}", id.toString());
+ ManasCoreStorage.LOG.warn("Failed to construct storage for id {}", id, e);
return null;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@storage-common/src/main/java/io/github/manasmods/manascore/storage/impl/StorageManager.java`
around lines 113 - 126, The current constructStorageFor method swallows all
Exceptions and logs a misleading message; instead, first null-check each
registry lookup (ENTITY_STORAGE_REGISTRY.registry.get(id),
CHUNK_STORAGE_REGISTRY.registry.get(id),
LEVEL_STORAGE_REGISTRY.registry.get(id)) and if null log
ManasCoreStorage.LOG.warn("No storage registered for id {}", id) and return
null, then perform the cast and call to create(...) in a narrow try/catch that
catches specific failures (ClassCastException and RuntimeException) and log them
with the throwable parameter (e.g., ManasCoreStorage.LOG.warn("Failed to
construct storage for {}: {}", id, e.getMessage(), e)) so stack traces aren’t
lost and messages aren’t misleading.
| modImplementation("com.electronwill.night-config:toml:3.8.1") | ||
| modImplementation("com.electronwill.night-config:core:3.8.1") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all build.gradle files and search for night-config references
find . -name "build.gradle" -o -name "build.gradle.kts" | head -20Repository: ManasMods/ManasCore
Length of output: 666
🏁 Script executed:
#!/bin/bash
# Search for night-config usage in gradle files
rg "night-config" --type groovy -B 2 -A 2Repository: ManasMods/ManasCore
Length of output: 1359
🌐 Web query:
latest stable version com.electronwill.night-config Maven Central 2025
💡 Result:
Latest stable release on Maven Central for com.electronwill.night-config (2025) is 3.8.3 (released Aug 8, 2025). [1], [2]
Example (core module):
<dependency>
<groupId>com.electronwill.night-config</groupId>
<artifactId>core</artifactId>
<version>3.8.3</version>
</dependency>
``` [1]
Sources:
[1] Maven Central directory for `com/electronwill/night-config/*` showing `3.8.3` published 2025-08-08.
[2] MvnRepository group page listing “Last Release on Aug 8, 2025” for NightConfig modules.
---
</details>
**`modImplementation` is incorrect for a plain Java library; use `include(implementation(...))` instead.**
Night Config is not a Fabric mod and should not use `modImplementation`. The correct pattern, as shown in the production `config-fabric` module, is:
```diff
- modImplementation("com.electronwill.night-config:toml:3.8.1")
- modImplementation("com.electronwill.night-config:core:3.8.1")
+ include(implementation("com.electronwill.night-config:toml:3.8.1"))
+ include(implementation("com.electronwill.night-config:core:3.8.1"))This mirrors the approach in config-fabric/build.gradle and ensures Night Config is properly bundled if needed. Additionally, the current version (3.8.1) is from July 2024; consider updating to 3.8.3 (August 2025).
📝 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.
| modImplementation("com.electronwill.night-config:toml:3.8.1") | |
| modImplementation("com.electronwill.night-config:core:3.8.1") | |
| include(implementation("com.electronwill.night-config:toml:3.8.1")) | |
| include(implementation("com.electronwill.night-config:core:3.8.1")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testing-fabric/build.gradle` around lines 30 - 31, Replace the incorrect mod
dependency declarations that use modImplementation for Night Config with the
bundling pattern used in the production module: stop using
modImplementation("com.electronwill.night-config:toml:3.8.1") and
modImplementation("com.electronwill.night-config:core:3.8.1") and instead
declare them as implementation dependencies and wrap them with
include(implementation(...)) so they are packaged (mirror the approach from
config-fabric's build.gradle); also bump the version from 3.8.1 to 3.8.3 for
both toml and core coordinates.
1.21.1/minh
Replaced EventFactory in StorageEvents with a new class ManasEventFactory, which uses a ConcurrentLinkedList instead of the ArrayList that EventFactory uses
Previously, since we were using the ConcurrentLinkedQueue as is, the registers would have potentially be un-registered after first use. To prevent that, I used non-destructive methods of getting the listeners from the queue.
Listener registration fix
…ther than an entity flag. Added documentation for methods. Updated ManasSkillInstance to support this new process. Removed the depricated entity tag.
…ucing lag of checking if entities have skills to tick.
Skills Cooldown/Temporary skill tick rework
Added new module which adds parallel mod loading fixes for the Neoforge version of manascore.
…age. Make heldTicks server authoratative.
…ication if it works.
…#handleSkillRelease
Race, Skill, and Arch updates
setCooldowns didn't mark cooldown tick properly.
6 participants
Description
List of changes
Type of Change
Summary by CodeRabbit
New Features
Chores