Kitsune traits #2

ShchMax-sudo · 2023-06-29T13:32:02Z

No description provided.

feat: added the ability to add or remove multiple traits simultaneously

Sigmarik · 2023-06-30T09:54:04Z

src/main/java/net/sigmarik/abilitymod/mixin/PlayerEntityMixin.java

+            int lightLevel = world.getLightLevel(LightType.SKY, this.getBlockPos());
+            if (!(lightLevel <= 11 ||
+                    !world.isSkyVisible(this.getBlockPos()) || this.isTouchingWaterOrRain() || this.isInLava() ||
+                    this.hasStatusEffect(StatusEffect.byRawId(AbilityMod.FIRE_RESISTANCE_STATUS_EFFECT)) ||


hasStatusEffect(StatusEffects.FIRE_RESISTANCE);
There is no need to add magical numbers that can change in the next update of the game. You can just use existing values.

Sigmarik · 2023-06-30T09:55:59Z

src/main/java/net/sigmarik/abilitymod/AbilityMod.java

@@ -33,10 +40,99 @@ public class AbilityMod implements DedicatedServerModInitializer {

 	public static final double BOAT_ATTRACTION_FACTOR = 0.1;

+	public static final int FIRE_RESISTANCE_STATUS_EFFECT = 12;


Unnecessary magical number. See the comment at line 96 of PlayerEntityMixin.

Sigmarik · 2023-06-30T10:02:35Z

src/main/java/net/sigmarik/abilitymod/AbilityMod.java

@@ -33,10 +40,99 @@ public class AbilityMod implements DedicatedServerModInitializer {

 	public static final double BOAT_ATTRACTION_FACTOR = 0.1;

+	public static final int FIRE_RESISTANCE_STATUS_EFFECT = 12;
+
+	public static final int FIRING_CHANCE_PER_TICK = 55;


No linkage to the trait this parameter corresponds to. Change the name or move it somewhere to make it obvious that it affects "DAMAGED_BY_DAYLIGHT" trait effect.

And also... Why is catching flame on daylight has a probability to happen? It makes sense for zombies, as they need to vary their flames a little so they wouldn't take damage at the exact same ticks. But I highly doubt there will every be a crowd of players with this trait purposefully syncing their flaming ticks.

Sigmarik · 2023-06-30T10:09:21Z

src/main/java/net/sigmarik/abilitymod/AbilityMod.java

+			EntityType.TRADER_LLAMA
+	));
+
+	public static final double FAST_COEFFICIENT = 0.3;


Well, these names are a little awkward... May be move these coefficients to the place they are used at?
Even though it s hell of a wrong code practice, we need it to follow it on order to preserve consistency with the Minecraft code.

Sigmarik · 2023-06-30T10:12:54Z

src/main/java/net/sigmarik/abilitymod/AbilityMod.java

+	));
+
+	public static final double FAST_COEFFICIENT = 0.3;
+	public static final UUID FAST_MODIFIER_UUID = new UUID(Random.createLocal().nextLong(), Random.createLocal().nextLong());


Using random numbers as unique identifiers is a practice you use when you want your game to crash in random ways at random moments.

Please see if there is a way to generate truly unique UUID without affecting other important game systems, such as player account identification.

Sigmarik · 2023-06-30T10:16:36Z

src/main/java/net/sigmarik/abilitymod/command/TraitCommand.java

@@ -22,11 +26,12 @@ public static void register(CommandDispatcher<ServerCommandSource> dispatcher) {
                literal("trait").requires(source -> source.hasPermissionLevel(2))
                .then(literal("add")
                        .then(argument("players", EntityArgumentType.players())
-                        .then(argument("trait", StringArgumentType.word()).suggests(SuggestionProviders.ALL_RECIPES)
+                        .then(argument("trait", StringArgumentType.greedyString())
+                                .suggests(SuggestionProviders.ALL_RECIPES)


These arguments are not recipes.

Should I remind who made this code line?

If you noticed an inconvenience and decided to blindly copy it, that's on you, even though I was the one who forgot to remove the test line from the code.

Sigmarik · 2023-06-30T10:21:10Z

src/main/java/net/sigmarik/abilitymod/command/TraitCommand.java

+
+                if (trait.equals(AbilityMod.TRAIT_FAST)) {
+                    EntityAttributeInstance speed = player.getAttributes().getCustomInstance(EntityAttributes.GENERIC_MOVEMENT_SPEED);
+                    EntityAttributeModifier speedModifier = new EntityAttributeModifier(AbilityMod.FAST_MODIFIER_UUID, "Fast", AbilityMod.FAST_COEFFICIENT, EntityAttributeModifier.Operation.MULTIPLY_BASE);


Take a look at this long boiiiiiiiiiii

Sigmarik · 2023-06-30T10:21:23Z

src/main/java/net/sigmarik/abilitymod/command/TraitCommand.java

+                }
+                if (trait.equals(AbilityMod.TRAIT_STRONG)) {
+                    EntityAttributeInstance strength = player.getAttributes().getCustomInstance(EntityAttributes.GENERIC_ATTACK_DAMAGE);
+                    EntityAttributeModifier strengthModifier = new EntityAttributeModifier(AbilityMod.STRONG_MODIFIER_UUID, "Strong", AbilityMod.STRONG_COEFFICIENT, EntityAttributeModifier.Operation.MULTIPLY_BASE);


Long boiiiiiiiiii

Sigmarik · 2023-06-30T10:22:18Z

src/main/java/net/sigmarik/abilitymod/command/TraitCommand.java

+
+                if (trait.equals(AbilityMod.TRAIT_FAST)) {
+                    EntityAttributeInstance speed = player.getAttributes().getCustomInstance(EntityAttributes.GENERIC_MOVEMENT_SPEED);
+                    EntityAttributeModifier speedModifier = new EntityAttributeModifier(AbilityMod.FAST_MODIFIER_UUID, "Fast", AbilityMod.FAST_COEFFICIENT, EntityAttributeModifier.Operation.MULTIPLY_BASE);


Wow, I really should have got a wide monitor for this review...

Anyway, there is another LONG BOIIIIIIIIII

Sigmarik · 2023-06-30T10:24:13Z

src/main/java/net/sigmarik/abilitymod/command/TraitCommand.java

+                }
+                if (trait.equals(AbilityMod.TRAIT_STRONG)) {
+                    EntityAttributeInstance strength = player.getAttributes().getCustomInstance(EntityAttributes.GENERIC_ATTACK_DAMAGE);
+                    EntityAttributeModifier strengthModifier = new EntityAttributeModifier(AbilityMod.STRONG_MODIFIER_UUID, "Strong", AbilityMod.STRONG_COEFFICIENT, EntityAttributeModifier.Operation.MULTIPLY_BASE);


static long long boiiii this_line = ". . ."

Sigmarik · 2023-06-30T10:32:51Z

src/main/java/net/sigmarik/abilitymod/mixin/PlayerEntityMixin.java

+    private void tickIronBurn() {
+        if (ServerState.hasTrait((PlayerEntity)(Object)this, AbilityMod.TRAIT_HOT_IRON)) {
+            boolean doDamage = false;
+            for (int i = 0; i < 41; ++i) {


It is time to start a witch hunt and burn every magical number.

You can use this.getInventory().size() to get the size of the player's inventory.

And also, even though it is a little controversial, it would be cool if you renamed iterator to something like stackId instead of single-character i.

Sigmarik

Fix the issues highlighted in comments.

Sigmarik · 2023-06-30T13:05:06Z

src/main/java/net/sigmarik/abilitymod/AbilityMod.java

+			Items.WATER_BUCKET
+	));
+
+	public static final HashSet<EntityType> NEUTRAL_MOBS = new HashSet<>(List.of(


There is a way to implement "hated" trait without this list.

refactor: refactored some bad-looking code refactor: now per-tick functions stop immediately, if it's need feat: added correct suggestions to /trait command

Sigmarik · 2023-06-30T16:08:21Z

src/main/java/net/sigmarik/abilitymod/AbilityMod.java

+			Items.WATER_BUCKET
+	));
+
+	public static final double MOVEMENT_SPEED_BASE_MULTIPLY_COEFFICIENT = 0.3;


THIS_IS_HELL_OF_A_LONG_VARIABLE_NAME
And even the name above is shorter than the name of the variable in the code.

He doesn't like obscure names, he doesn't like understandable names...

The very reason why I asked you to change the variable name was that it didn't represent the trait it relates to.
Now let's take a look at you 40-character long (holy ****) variable name. May be I am a dummy, after all, and you really need that many words to describe what is going on.
How does the word "BASE" contribute to the variable name? Is this variable the root of some obscure tree? Or is it some value that will be gradually changed later? But why is it final, then?
That's because it is not, it's just a constant you need to multiply the player's movement at one point in the code. So the word "BASE" is unnecessary at best and misleading at worst.
"MULTIPLY_COEFFICIENT". It's called "multiplication coefficient" if you really want to preserve that Shakespearian vibe you are going with in your code. Programmers usually call this "multiplier", unless it occurs in some form of series or equations with other parameters, where it should be called "coefficient". In this case the word "multiplier" is more than enough.
"MOVEMENT_SPEED". There are not many parameters related to "movement". At least not many you would ever want to change. Just the word "movement" or "speed" should be enough.

So, we end up with "SPEED_MULTIPLIER" or "MOVEMENT_SPEED_MULTIPLIER", if you don't like the last transformation. These names are shorter than the original and less misleading than the original. Yet, neither them, nor the original 40-ch. long name describe where this multiplier is applied. It could relate to cobweb speed reduction (I mean, this coefficient is less than 1, so it should relate to slowing down something), it could relate to speed potion effect change, it could just mean that every player is now 3 times slower than they originally were. This extremely long misleading nonsensical variable name does not do ANYTHING to describe the very thing I asked for in the original review. So what's the purpose of all this circus?

Sigmarik · 2023-06-30T16:08:59Z

src/main/java/net/sigmarik/abilitymod/AbilityMod.java

+
+	public static final double MOVEMENT_SPEED_BASE_MULTIPLY_COEFFICIENT = 0.3;
+
+	public static final double ATTACK_DAMAGE_BASE_MULTIPLY_COEFFICIENT = 0.3;


WOW_THIS_NAME_IS_ALSO_SUPER_LONG

Sigmarik · 2023-06-30T16:13:39Z

src/main/java/net/sigmarik/abilitymod/command/TraitCommand.java

+                            "Fast", AbilityMod.MOVEMENT_SPEED_BASE_MULTIPLY_COEFFICIENT,
+                            EntityAttributeModifier.Operation.MULTIPLY_BASE);
+                    if (!speed.hasModifier(speedModifier)) {
+                        speed.addPersistentModifier(speedModifier);


There is, like, 5-6 tabs before the start of the line. I know, this can be classified as a java syntax, but at least it is a hint to wrap this part of the code into function.
Who knows, may be you will even be able to generalize the logic and not repeat the same code again and again.

Sigmarik · 2023-06-30T16:20:25Z

src/main/java/net/sigmarik/abilitymod/command/TraitCommand.java

@@ -87,4 +149,30 @@ private static int printTraits(CommandContext<ServerCommandSource> context) thro

        return 1;
    }
+
+    private static class TraitsSuggestionProvider implements SuggestionProvider<ServerCommandSource> {


Is that... is that a subclass declared in the middle of nowhere, lost in the forest of method definitions?
First I thought you created a completely new class when you used it inside register() function.

Subclasses deserve their place on top of the class definition, they are not part of the all structurally insignificant code written inside method definitions.

Sigmarik · 2023-06-30T16:24:53Z

src/main/java/net/sigmarik/abilitymod/mixin/PlayerEntityMixin.java

        }
    }

    private void tickBoatMagnet() {
-        if (ServerState.hasTrait((PlayerEntity)(Object)this, AbilityMod.TRAIT_BOAT_MAGNET) && hasVehicle() &&
+        if (!ServerState.hasTrait((PlayerEntity)(Object)this, AbilityMod.TRAIT_BOAT_MAGNET) && hasVehicle() &&


You did not inverse the condition. The code above (and a few lines below) do not represent the same logic there was before.
This code just does not work correctly.

Sigmarik · 2023-06-30T17:32:25Z

src/main/java/net/sigmarik/abilitymod/mixin/PlayerEntityMixin.java

+        }
+    }
+
+    private void tickLightBurn() {


tickLightDamage? It corelates a bit more with the trait name.

It's just a suggestion, though.

Sigmarik · 2023-06-30T17:33:11Z

src/main/java/net/sigmarik/abilitymod/mixin/PlayerEntityMixin.java

            }
        }
    }

+    private void tickIronBurn() {


tickHotIron?

Sigmarik · 2023-06-30T17:35:10Z

src/main/java/net/sigmarik/abilitymod/mixin/PlayerEntityMixin.java

+                this.hasStatusEffect(StatusEffects.FIRE_RESISTANCE) ||
+                this.getBlockStateAtPos().getBlock().equals(Blocks.COBWEB) || (time >= 12542 && time < 23460) ||
+                !world.getRegistryKey().getValue().equals(DimensionTypes.OVERWORLD_ID))) {
+            if (world.random.nextInt(AbilityMod.DAYLIGHT_BURNING_CHANCE_PER_TICK) == 0) {


Nested if-statements.

ShchMax-sudo added 2 commits June 28, 2023 17:51

feat: last zombie-like trait

6a10faa

feat: 3 kitsune traits

4a14a38

feat: added the ability to add or remove multiple traits simultaneously

ShchMax-sudo requested a review from Sigmarik June 29, 2023 13:32

Sigmarik reviewed Jun 30, 2023

View reviewed changes

Sigmarik requested changes Jun 30, 2023

View reviewed changes

Sigmarik reviewed Jun 30, 2023

View reviewed changes

fix: now "fast" & "strong" traits work after stopping server

0f7ff0e

refactor: refactored some bad-looking code refactor: now per-tick functions stop immediately, if it's need feat: added correct suggestions to /trait command

ShchMax-sudo closed this Jun 30, 2023

Sigmarik reviewed Jun 30, 2023

View reviewed changes

ShchMax-sudo reopened this Jun 30, 2023

ShchMax-sudo requested a review from Sigmarik June 30, 2023 16:35

Sigmarik reviewed Jun 30, 2023

View reviewed changes

src/main/java/net/sigmarik/abilitymod/mixin/PlayerEntityMixin.java

}

}

}

private void tickIronBurn() {

Copy link

Owner

Sigmarik Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tickHotIron?

Sigmarik reviewed Jun 30, 2023

View reviewed changes

Base automatically changed from traits to master July 1, 2023 13:24

Sigmarik closed this Jul 14, 2023

Sigmarik deleted the kitsune_traits branch August 10, 2023 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kitsune traits #2

Kitsune traits #2

ShchMax-sudo commented Jun 29, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

ShchMax-sudo Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023 •

edited

Loading

Sigmarik Jun 30, 2023

Sigmarik left a comment

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

ShchMax-sudo Jun 30, 2023

Sigmarik Jul 1, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

Sigmarik Jun 30, 2023

		@@ -33,10 +40,99 @@ public class AbilityMod implements DedicatedServerModInitializer {

		public static final double BOAT_ATTRACTION_FACTOR = 0.1;

		public static final int FIRE_RESISTANCE_STATUS_EFFECT = 12;


		public static final double MOVEMENT_SPEED_BASE_MULTIPLY_COEFFICIENT = 0.3;

		public static final double ATTACK_DAMAGE_BASE_MULTIPLY_COEFFICIENT = 0.3;

Kitsune traits #2

Kitsune traits #2

Conversation

ShchMax-sudo commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sigmarik Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sigmarik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sigmarik Jun 30, 2023 •

edited

Loading